On Tue, May 28, 2019 at 2:34 AM Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:
>
> 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.
>
Was it before of after you read the Kconfig help that talks about it?
Asking to see if copying that text into commit would be enough, or if
should write a more detailed explanation.
> > +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.
>
OK, I'll have to make mem_copy() visible globally for them to remain
one-liners, but that should be doable.
> > + .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?
>
I was modelling this after "pinctrl" property in which always has
trailing "-0". I can add the support for "device-path" in v2, though.
> > + 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.
>
Oh, missed the fact that those strings are allocated. Will fix in v2.
> > + }
> > +
> > + 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?
>
The only use-case we have for this is to communicate data to already
existing U-Boot installation with a valid environment data, so the
scenario you are talking about never came up. I guess I can change the
code to zero the buffered data out and proceed, so that FS layer would
perceive this as an empty environment.
> > + 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?
>
Sure, will do in v2.
> > + 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
>
Will do.
Thanks,
Andrey Smirnov
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
