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

[Joel Stanley]

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.

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

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}

(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;

don't forget to free dev_path after reading it.

> +	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?

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

> +{
> +	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.

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

> +		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?

> +		}
> +		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;
> +}