Re: [PATCH 4/7] misc: Add a driver to expose U-Boot environment variable data

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, May 27, 2019 at 01:18:50PM -0700, Andrey Smirnov wrote:
> Add a driver to expose U-Boot environment variable data as a single
> mmap-able device. Not very useful on its own, it is a crucial
> low-level plumbing needed by filesystem driver introduced in the
> following commit.

It wasn't clear to me why we need this driver at all, so it might be
worth adding a comment that this is for handling redundant env.

> +static int ubootvar_flush(struct cdev *cdev)
> +{
> +	struct device_d *dev = cdev->dev;
> +	struct ubootvar_data *ubdata = dev->priv;
> +	const char *path = ubdata->path[!ubdata->current];
> +	uint32_t crc = 0xffffffff;
> +	struct resource *mem;
> +	resource_size_t size;
> +	const void *data;
> +	int fd, ret = 0;
> +
> +	mem = dev_get_resource(dev, IORESOURCE_MEM, 0);
> +	if (IS_ERR(mem)) {
> +		dev_err(dev, "Failed to get associated memory resource\n");
> +		return PTR_ERR(mem);
> +	}

Resources of IORESOURCE_MEM are meant to describe an area in the
physical memory space. You shouldn't abuse it to describe some
arbitrarily allocated memory area. Why not simply put the pointers into
struct ubootvar_data?

> +
> +	fd = open(path, O_WRONLY);
> +	if (fd < 0) {
> +		dev_err(dev, "Failed to open %s\n", path);
> +		return -errno;
> +	}
> +	/*
> +	 * FIXME: This code needs to do a proper protect/unprotect and
> +	 * erase calls to work on MTD devices
> +	 */
> +
> +	/*
> +	 * Write a dummy CRC first as a way of invalidating the
> +	 * environment in case we fail mid-flushing
> +	 */
> +	if (write_full(fd, &crc, sizeof(crc)) != sizeof(crc)) {
> +		dev_err(dev, "Failed to write dummy CRC\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +
> +	if (ubdata->count > 1) {
> +		/*
> +		 * FIXME: This assumes FLAG_INCREMENTAL
> +		 */
> +		const uint8_t flag = ++ubdata->flag;
> +
> +		if (write_full(fd, &flag, sizeof(flag)) != sizeof(flag)) {
> +			dev_dbg(dev, "Failed to write flag\n");
> +			ret = -errno;
> +			goto close_fd;
> +		}
> +	}
> +
> +	data = (const void *)mem->start;
> +	size = resource_size(mem);
> +
> +	/*
> +	 * Write out and flush all of the new environement data
> +	 */

s/environement/environment/

> +	if (write_full(fd, data, size) != size) {
> +		dev_dbg(dev, "Failed to write data\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +
> +	if (flush(fd)) {
> +		dev_dbg(dev, "Failed to flush written data\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +	/*
> +	 * Now that all of the environment data is out, we can go back
> +	 * to the start of the block and write correct CRC, to finish
> +	 * the processs.
> +	 */
> +	if (lseek(fd, 0, SEEK_SET) != 0) {
> +		dev_dbg(dev, "lseek() failed\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +
> +	crc = crc32(0, data, size);
> +	if (write_full(fd, &crc, sizeof(crc)) != sizeof(crc)) {
> +		dev_dbg(dev, "Failed to write valid CRC\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +	/*
> +	 * Now that we've successfuly written new enviroment blob out,
> +	 * swtich current partition.

s/swtich/switch/

> +	 */
> +	ubdata->current = !ubdata->current;
> +
> +close_fd:
> +	close(fd);
> +	return ret;
> +}
> +
> +static struct cdev_operations ubootvar_ops = {
> +	.read = mem_read,
> +	.write = mem_write,
> +	.memmap = generic_memmap_rw,

Ok, now I understand this struct resource thingy, you want to reuse
mem_read and friends. Given that these functions are oneliners to
implement I still suggest implementing them.

> +	.flush = ubootvar_flush,
> +};
> +
> +static void ubootenv_info(struct device_d *dev)
> +{
> +	struct ubootvar_data *ubdata = dev->priv;
> +
> +	printf("Current environment copy: device-path-%d\n", ubdata->current);
> +}
> +
> +static int ubootenv_probe(struct device_d *dev)
> +{
> +	struct ubootvar_data *ubdata;
> +	unsigned int crc_ok = 0;
> +	int ret, i, count = 0;
> +	uint32_t crc[2];
> +	uint8_t flag[2];
> +	size_t size[2];
> +	void *blob[2] = { NULL, NULL };
> +	uint8_t *data[2];
> +
> +	/*
> +	 * FIXME: Flag scheme is determined by the type of underlined
> +	 * non-volatible device, so it should probably come from
> +	 * Device Tree binding. Currently we just assume incremental
> +	 * scheme since that is what is used on SD/eMMC devices.
> +	 */
> +	enum ubootvar_flag_scheme flag_scheme = FLAG_INCREMENTAL;
> +
> +	ubdata = xzalloc(sizeof(*ubdata));
> +
> +	ret = of_find_path(dev->device_node, "device-path-0",
> +			   &ubdata->path[0],
> +			   OF_FIND_PATH_FLAGS_BB);

Maybe allow "device-path" without -x suffix when the U-Boot environment
is not redundant?

> +	if (ret) {
> +		dev_err(dev, "Failed to find first device\n");
> +		goto out;
> +	}
> +
> +	count++;
> +
> +	if (!of_find_path(dev->device_node, "device-path-1",
> +			  &ubdata->path[1],
> +			  OF_FIND_PATH_FLAGS_BB)) {
> +		count++;
> +	} else {
> +		/*
> +		 * If there's no redundant environment partition we
> +		 * configure both paths to point to the same device,
> +		 * so that writing logic could stay the same for both
> +		 * redundant and non-redundant cases
> +		 */
> +		ubdata->path[1] = ubdata->path[0];

ubdata->path[0] contains an allocated string, so we must be careful when
freeing this. You don't free it at all, so there's no problem currently.
I think you should either free it correctly in the error path or make
sure that both ubdata->path[0] and ubdata->path[1] contain allocated
strings.

> +	}
> +
> +	for (i = 0; i < count; i++) {
> +		data[i] = blob[i] = read_file(ubdata->path[i], &size[i]);
> +		if (!blob[i]) {
> +			dev_err(dev, "Failed to read U-Boot environment\n");
> +			ret = -EIO;
> +			goto out;
> +		}
> +
> +		crc[i] = *(uint32_t *)data[i];
> +
> +		size[i] -= sizeof(uint32_t);
> +		data[i] += sizeof(uint32_t);
> +
> +		if (count > 1) {
> +			/*
> +			 * When used in primary/redundant
> +			 * configuration, environment header has an
> +			 * additional "flag" byte
> +			 */
> +			flag[i] = *data[i];
> +			size[i] -= sizeof(uint8_t);
> +			data[i] += sizeof(uint8_t);
> +		}
> +
> +		crc_ok |= (crc32(0, data[i], size[i]) == crc[i]) << i;
> +	}
> +
> +	switch (crc_ok) {
> +	case 0b00:
> +		ret = -EINVAL;
> +		dev_err(dev, "Bad CRC, can't continue further\n");
> +		goto out;

So when there's no valid U-Boot env is found (erased for example) then
this driver errors out and we won't have a chance to ever write
something there. Is this intended?

> +	case 0b11:
> +		/*
> +		 * Both partition are valid, so we need to examine
> +		 * flags to determine which one to use as current
> +		 */
> +		switch (flag_scheme) {
> +		case FLAG_INCREMENTAL:
> +			if ((flag[0] == 0xff && flag[1] == 0) ||
> +			    (flag[1] == 0xff && flag[0] == 0)) {
> +				/*
> +				 * When flag overflow happens current
> +				 * partition is the one whose counter
> +				 * reached zero first. That is if
> +				 * flag[1] == 0 is true (1), then i
> +				 * would be 1 as well
> +				 */
> +				i = flag[1] == 0;
> +			} else {
> +				/*
> +				 * In no-overflow case the partition
> +				 * with higher flag value is
> +				 * considered current
> +				 */
> +				i = flag[1] > flag[0];
> +			}
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			dev_err(dev, "Unknown flag scheme %u\n", flag_scheme);
> +			goto out;
> +		}
> +		break;
> +	default:
> +		/*
> +		 * Only one partition is valid, so the choice of the
> +		 * current one is obvious
> +		 */
> +		i = __ffs(crc_ok);

'i' changes its meaning from a loop counter to the partition currently
used. This makes this code harder to read, could you use a separate
variable for the currently used partition?

> +		break;
> +	};
> +
> +	ret = device_add_resource(dev, "data", (resource_size_t)data[i],
> +				  size[i], IORESOURCE_MEM);
> +	if (ret) {
> +		dev_err(dev, "Failed to add resource\n");
> +		goto out;
> +	}
> +
> +	ubdata->cdev.name = basprintf("ubootvar%d",
> +				      cdev_find_free_index("ubootvar"));
> +	ubdata->cdev.size = size[i];
> +	ubdata->cdev.ops = &ubootvar_ops;
> +	ubdata->cdev.dev = dev;
> +	ubdata->cdev.filetype = filetype_ubootvar;
> +	ubdata->current = i;
> +	ubdata->count = count;
> +	ubdata->flag = flag[i];
> +
> +	dev->priv = ubdata;
> +
> +	ret = devfs_create(&ubdata->cdev);
> +	if (ret) {
> +		dev_err(dev, "Failed to create corresponding cdev\n");
> +		goto out;
> +	}
> +
> +	cdev_create_default_automount(&ubdata->cdev);
> +
> +	if (count > 1) {
> +		/*
> +		 * We won't be using read data from redundant
> +		 * parttion, so we may as well free at this point
> +		 */
> +		free(blob[!i]);
> +	}
> +
> +	dev->info = ubootenv_info;
> +
> +	return 0;
> +out:
> +	for (i = 0; i < count; i++)
> +		free(blob[i]);
> +
> +	free(ubdata);
> +
> +	return ret;
> +}
> +
> +static struct of_device_id ubootenv_dt_ids[] = {
> +	{
> +		.compatible = "barebox,uboot-environment",

Please add
Documentation/devicetree/bindings/barebox/barebox,uboot-environment.rst

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox