[Skiboot] [RFC PATCH 4/7] external/common: Add POWERPC code reenable building pflash for POWER

[Cyril Bur]

On Mon, 20 Jul 2015 14:28:13 +0930
Joel Stanley <joel at jms.id.au> wrote:

> On Fri, 2015-07-17 at 17:01 +1000, Cyril Bur wrote:
> > As per commit to create the external/common code, this commit introduces
> > the POWER arch compatibility flash reading code.
> > 
> > This commit actually should cause no functional change to pflash (it won't
> > be able to access the BMC flash if it ever could), however rather than
> > accessing the flash via the debug xscom interface it will access it though
> > the MTD interface provided by linux and skiboot.
> 

Thanks for the review! I'll go pflash some new firmware on a palmetto or two
just to be sure there isn't a massive regression but I can't see myself testing
all the corner cases...

> Did we ever use xscom? I think you mean LPC.

Maybe....

> 
> Although it happens to be ARM/Power, when writing your commit messages
> it would make sense to talk about it in terms of the method you're using
> to access it:
> 
> AHB is the bus that attaches PNOR to the BMC, which happens to be ARM
> (but could also be ColdFire - but lets not go there)
> 
> LPC is how the Power8 is attached to the PNOR.
> 
> When the PNOR is mapped on the LPC bus, Linux can talk to it using the:
> 
>  - mtd device, which uses opal_flash_{read,write,erase} 
>  - debugfs lpc, which use opal_lpc_{read,write}
> 

So this may be the best explaination I have of what used to be done... (i'm
going to run with it!), I'll edit the commit message, probs get you to proof
read it once more...

> (cc+benh so he can correct me where I'm wrong)
> > --- /dev/null
> > +++ b/external/common/arch_flash_powerpc.c
> 
> > +static int get_dev_attr(const char *dev, const char *attr_file, uint32_t *attr)
> > +{
> > +	char dev_path[PATH_MAX] = SYSFS_MTD_PATH;
> > +	/*
> > +	 * Needs to be large enough to hold at most uint32_t represented as a
> > +	 * string in hex with leading 0x
> > +	 */
> > +	char attr_buf[10];
> > +	int fd, rc;
> > +
> > +	/*
> > +	 * sizeof(dev_path) - (strlen(dev_path) + 1) is the remaining space in
> > +	 * dev_path, + 1 to account for the '\0'. As strncat could write n+1 bytes
> > +	 * to dev_path the correct calulcation for n is:
> > +	 * (sizeof(dev_path) - (strlen(dev_path) + 1) - 1)
> > +	 */
> > +	strncat(dev_path, dev, (sizeof(dev_path) - (strlen(dev_path) + 1) - 1));
> > +	strncat(dev_path, "/", (sizeof(dev_path) - (strlen(dev_path) + 1) - 1));
> > +	strncat(dev_path, attr_file, (sizeof(dev_path) - (strlen(dev_path) + 1) - 1));
> 
> I think this is more readable:
> 
> char *dev_path;
> rc = asprintf(&dev_path, "%s/%s/%s", FDT_FLASH_PATH, dev, attr_file);
> if (rc < 0)
>    goto out;

Very nice yes.

> 
> don't forget to free dev_path after reading it.
> 

I bet I did...

> > +	fd = open(dev_path, O_RDONLY);
> > +	if (fd == -1)
> > +		goto out;
> > +
> > +	rc = read(fd, attr_buf, sizeof(attr_buf));
> > +	close(fd);
> > +	if (rc == -1)
> > +		goto out;
> > +
> > +	if (attr)
> > +		*attr = strtol(attr_buf, NULL, 0);
> > +
> > +	return 0;
> > +
> > +out:
> > +	fprintf(stderr, "Couldn't get MTD device attribute '%s' from '%s'\n", dev, attr_file);
> > +	return -1;
> > +}
> > +
> > +static int get_dev_mtd(const char *fdt_flash_path, char **r_path)
> 
> r_path means something else to me. Perhaps a different name? fdt_path
> and mtd_path?
> 

Sure

> This function is searching for /dev/mtdN devices, and then looking in
> the dt for a matching entry?
> 

Yeah, I wasn't thrilled with this method of finding out which /dev/mtdN is the
flash but its the best I could come up with...

> > +{
> > +	struct dirent **namelist;
> > +	char fdt_node_path[PATH_MAX];
> > +	int count, i, rc, fd, done;
> > +
> > +	if (!fdt_flash_path)
> > +		return -1;
> > +
> > +	fd = open(fdt_flash_path, O_RDONLY);
> > +	if (fd == -1) {
> > +		fprintf(stderr, "Couldn't open '%s' FDT attribute to determine which flash device to use\n",
> > +				fdt_flash_path);
> > +		return -1;
> > +	}
> > +
> > +	rc = read(fd, fdt_node_path, sizeof(fdt_node_path));
> > +	close(fd);
> > +	if (rc == -1) {
> > +		fprintf(stderr, "Couldn't read flash FDT node from '%s'\n", fdt_flash_path);
> 
> We can give the user a hint that they might not have permissions
> (root?), or that their kernel/skiboot may not be new enough to have the
> flash node in the device tree.
> 

Yeah, hints are always nice.

> Is there a --force mode so the user can say "just use the flash device
> I've pointed you to?"

It's not really up to this layer to worry too much about that but it can be
done.

If you look at arch_flash_init(), what you would want is a tool which
passes through something in with *file, the gard tool is written that way
(although it doesn't use this infrastructure yet. pflash would need to be
updated --force or --file or whatever is what it needs... this is something I'm
planning on doing, it's basically a must for running pflash on x86, which, is
going to be nice!

> 
> > +		return -1;
> > +	}
> > +
> > +	count = scandir(SYSFS_MTD_PATH, &namelist, NULL, alphasort);
> > +	if (count == -1) {
> > +		fprintf(stderr, "Couldn't scan '%s' for MTD devices\n", SYSFS_MTD_PATH);
> 
> Again, give the user a hint to run as root?
> 
> > +		return -1;
> > +	}
> > +
> > +	rc = 0;
> > +	done = 0;
> > +	for (i = 0; i < count; i++) {
> > +		struct dirent *dirent;
> > +		char dev_path[PATH_MAX] = SYSFS_MTD_PATH;
> > +		char fdt_node_path_tmp[PATH_MAX];
> > +
> > +		dirent = namelist[i];
> > +		if (dirent->d_name[0] == '.' || rc || done) {
> > +			free(namelist[i]);
> > +			continue;
> > +		}
> > +
> > +		strncat(dev_path, dirent->d_name, sizeof(dev_path) - strlen(dev_path) - 2);
> > +		strncat(dev_path, "/device/of_node", sizeof(dev_path) - strlen(dev_path) - 2);
> 
> asprintf
> 
> > +
> > +		rc = readlink(dev_path, fdt_node_path_tmp, sizeof(fdt_node_path_tmp) - 1);
> > +		if (rc == -1) {
> > +			/*
> > +			 * This might fail because it could not exist if the system has flash
> > +			 * devices that present as mtd but don't have corresponding FDT
> > +			 * nodes, just continue silently.
> > +			 */
> > +			free(namelist[i]);
> > +			/* Should still try the next dir so reset rc */
> > +			rc = 0;
> > +			continue;
> > +		}
> > +		fdt_node_path_tmp[rc] = '\0';
> > +
> > +		if (strstr(fdt_node_path_tmp, fdt_node_path)) {
> > +			uint32_t flags, size;
> > +
> > +			/*
> > +			 * size and flags could perhaps have be gotten another way but this
> > +			 * method is super unlikely to fail so it will do.
> > +			 */
> > +
> > +			/* Check to see if device is writeable */
> > +			rc = get_dev_attr(dirent->d_name, "flags", &flags);
> > +			if (rc) {
> > +				free(namelist[i]);
> > +				continue;
> > +			}
> > +
> > +			/* Get the size of the mtd device while we're at it */
> > +			rc = get_dev_attr(dirent->d_name, "size", &size);
> > +			if (rc) {
> > +				free(namelist[i]);
> > +				continue;
> > +			}
> > +
> > +			strcpy(dev_path, "/dev/");
> > +			strncat(dev_path, dirent->d_name, sizeof(dev_path) - strlen(dev_path) - 2);
> > +			*r_path = strdup(dev_path);
> 
> asprintf
> 
> > +			done = 1;
> 
> true?

true!

> 
> > +		}
> > +		free(namelist[i]);
> > +	}
> > +	free(namelist);
> > +
> > +	if (!done)
> > +		fprintf(stderr, "Couldn't find '%s' corresponding MTD\n", fdt_flash_path);
> > +
> > +	/* explicit negative value so as to not return a libflash code */
> > +	return done ? rc : -1;
> > +}
> 
>