[RFC 07/22] dm: blk: add UCLASS_PARTITION
AKASHI Takahiro
takahiro.akashi at linaro.org
Mon Oct 11 04:29:25 CEST 2021
Heinrich,
On Fri, Oct 08, 2021 at 10:23:52AM +0200, Heinrich Schuchardt wrote:
>
>
> On 10/8/21 02:51, AKASHI Takahiro wrote:
> > On Mon, Oct 04, 2021 at 12:27:59PM +0900, AKASHI Takahiro wrote:
> > > On Fri, Oct 01, 2021 at 11:30:37AM +0200, Heinrich Schuchardt wrote:
> > > >
> > > >
> > > > On 10/1/21 07:01, AKASHI Takahiro wrote:
> > > > > UCLASS_PARTITION device will be created as a child node of
> > > > > UCLASS_BLK device.
> > > > >
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > > > ---
> > > > > drivers/block/blk-uclass.c | 111 +++++++++++++++++++++++++++++++++++++
> > > > > include/blk.h | 9 +++
> > > > > include/dm/uclass-id.h | 1 +
> > > > > 3 files changed, 121 insertions(+)
> > > > >
> > > > > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> > > > > index 83682dcc181a..dd7f3c0fe31e 100644
> > > > > --- a/drivers/block/blk-uclass.c
> > > > > +++ b/drivers/block/blk-uclass.c
> > > > > @@ -12,6 +12,7 @@
> > > > > #include <log.h>
> > > > > #include <malloc.h>
> > > > > #include <part.h>
> > > > > +#include <string.h>
> > > > > #include <dm/device-internal.h>
> > > > > #include <dm/lists.h>
> > > > > #include <dm/uclass-internal.h>
> > > > > @@ -695,6 +696,44 @@ int blk_unbind_all(int if_type)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +int blk_create_partitions(struct udevice *parent)
> > > > > +{
> > > > > + int part, count;
> > > > > + struct blk_desc *desc = dev_get_uclass_plat(parent);
> > > > > + struct disk_partition info;
> > > > > + struct disk_part *part_data;
> > > > > + char devname[32];
> > > > > + struct udevice *dev;
> > > > > + int ret;
> > > > > +
> > > > > + if (!CONFIG_IS_ENABLED(PARTITIONS) ||
> > > > > + !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
> > > > > + return 0;
> > > > > +
> > > > > + /* Add devices for each partition */
> > > > > + for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > > > > + if (part_get_info(desc, part, &info))
> > > > > + continue;
> > > > > + snprintf(devname, sizeof(devname), "%s:%d", parent->name,
> > > > > + part);
> > > > > +
> > > > > + ret = device_bind_driver(parent, "blk_partition",
> > > > > + strdup(devname), &dev);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + part_data = dev_get_uclass_plat(dev);
> > > > > + part_data->partnum = part;
> > > > > + part_data->gpt_part_info = info;
> > > > > + count++;
> > > > > +
> > > > > + device_probe(dev);
> > > > > + }
> > > > > + debug("%s: %d partitions found in %s\n", __func__, count, parent->name);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > static int blk_post_probe(struct udevice *dev)
> > > > > {
> > > > > if (IS_ENABLED(CONFIG_PARTITIONS) &&
> > > > > @@ -713,3 +752,75 @@ UCLASS_DRIVER(blk) = {
> > > > > .post_probe = blk_post_probe,
> > > > > .per_device_plat_auto = sizeof(struct blk_desc),
> > > > > };
> > > > > +
> > > > > +static ulong blk_part_read(struct udevice *dev, lbaint_t start,
> > > > > + lbaint_t blkcnt, void *buffer)
> > > > > +{
> > > > > + struct udevice *parent;
> > > > > + struct disk_part *part;
> > > > > + const struct blk_ops *ops;
> > > > > +
> > > > > + parent = dev_get_parent(dev);
> > > >
> > > > What device type will the parent have if it is a eMMC hardware partition?
> > > >
> > > > > + ops = blk_get_ops(parent);
> > > > > + if (!ops->read)
> > > > > + return -ENOSYS;
> > > > > +
> > > > > + part = dev_get_uclass_plat(dev);
> > > >
> > > > You should check that we do not access the block device past the
> > > > partition end:
> > >
> > > Yes, I will fix all of checks.
> > >
> > > > struct blk_desc *desc = dev_get_uclass_plat(parent);
> > > > if ((start + blkcnt) * desc->blksz < part->gpt_part_info.blksz)
> > > > return -EFAULT.
> > > >
> > > > > + start += part->gpt_part_info.start;
> >
> > A better solution is:
> > if (start >= part->gpt_part_info.size)
> > return 0;
> >
> > if ((start + blkcnt) > part->gpt_part_info.size)
> > blkcnt = part->gpt_part_info.size - start;
> > start += part->gpt_part_info.start;
> > instead of returning -EFAULT.
> > (note that start and blkcnt are in "block".)
>
> What is your motivation to support an illegal access?
>
> We will implement the EFI_BLOCK_IO_PROTOCOL based on this function. The
> ReadBlocks() and WriteBlocks() services must return
> EFI_INVALID_PARAMETER if the read request contains LBAs that are not
> valid.
I interpreted that 'LBA' was the third parameter to ReadBlocks API,
and that if the starting block is out of partition region, we should
return an error (and if not, we still want to trim IO request to fit
into partition size as other OS's API like linux does).
Do you think it's incorrect?
-Takahiro Akashi
> So we should return an error code here that can be translated
> into EFI_INVALID_PARAMETER.
>
> Best regards
>
> Heinrich
>
> >
> > -Takahiro Akashi
> >
> > > > > +
> > > > > + return ops->read(parent, start, blkcnt, buffer);
> > > > > +}
> > > > > +
> > > > > +static ulong blk_part_write(struct udevice *dev, lbaint_t start,
> > > > > + lbaint_t blkcnt, const void *buffer)
> > > > > +{
> > > > > + struct udevice *parent;
> > > > > + struct disk_part *part;
> > > > > + const struct blk_ops *ops;
> > > > > +
> > > > > + parent = dev_get_parent(dev);
> > > > > + ops = blk_get_ops(parent);
> > > > > + if (!ops->write)
> > > > > + return -ENOSYS;
> > > > > +
> > > > > + part = dev_get_uclass_plat(dev);
> > > > > + start += part->gpt_part_info.start;
> > > >
> > > > here too
> > > >
> > > > > +
> > > > > + return ops->write(parent, start, blkcnt, buffer);
> > > > > +}
> > > > > +
> > > > > +static ulong blk_part_erase(struct udevice *dev, lbaint_t start,
> > > > > + lbaint_t blkcnt)
> > > > > +{
> > > > > + struct udevice *parent;
> > > > > + struct disk_part *part;
> > > > > + const struct blk_ops *ops;
> > > > > +
> > > > > + parent = dev_get_parent(dev);
> > > > > + ops = blk_get_ops(parent);
> > > > > + if (!ops->erase)
> > > > > + return -ENOSYS;
> > > > > +
> > > > > + part = dev_get_uclass_plat(dev);
> > > > > + start += part->gpt_part_info.start;
> > > >
> > > > here too
> > > >
> > > > Best regards
> > > >
> > > > Heinrich
> > > >
> > > > > +
> > > > > + return ops->erase(parent, start, blkcnt);
> > > > > +}
> > > > > +
> > > > > +static const struct blk_ops blk_part_ops = {
> > > > > + .read = blk_part_read,
> > > > > + .write = blk_part_write,
> > > > > + .erase = blk_part_erase,
> > > > > +};
> > > > > +
> > > > > +U_BOOT_DRIVER(blk_partition) = {
> > > > > + .name = "blk_partition",
> > > > > + .id = UCLASS_PARTITION,
> > > > > + .ops = &blk_part_ops,
> > > > > +};
> > > > > +
> > > > > +UCLASS_DRIVER(partition) = {
> > > > > + .id = UCLASS_PARTITION,
> > > > > + .per_device_plat_auto = sizeof(struct disk_part),
> > > > > + .name = "partition",
> > > > > +};
> > > > > diff --git a/include/blk.h b/include/blk.h
> > > > > index 19bab081c2cd..3d883eb1db64 100644
> > > > > --- a/include/blk.h
> > > > > +++ b/include/blk.h
> > > > > @@ -366,6 +366,15 @@ int blk_create_devicef(struct udevice *parent, const char *drv_name,
> > > > > const char *name, int if_type, int devnum, int blksz,
> > > > > lbaint_t lba, struct udevice **devp);
> > > > >
> > > > > +/**
> > > > > + * blk_create_partitions - Create block devices for disk partitions
> > > > > + *
> > > > > + * Create UCLASS_PARTITION udevices for each of disk partitions in @parent
> > > > > + *
> > > > > + * @parent: Whole disk device
> > > > > + */
> > > > > +int blk_create_partitions(struct udevice *parent);
> > > > > +
> > > > > /**
> > > > > * blk_unbind_all() - Unbind all device of the given interface type
> > > > > *
> > > > > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > > > > index e7edd409f307..30892d01ce13 100644
> > > > > --- a/include/dm/uclass-id.h
> > > > > +++ b/include/dm/uclass-id.h
> > > > > @@ -80,6 +80,7 @@ enum uclass_id {
> > > > > UCLASS_P2SB, /* (x86) Primary-to-Sideband Bus */
> > > > > UCLASS_PANEL, /* Display panel, such as an LCD */
> > > > > UCLASS_PANEL_BACKLIGHT, /* Backlight controller for panel */
> > > > > + UCLASS_PARTITION, /* Logical disk partition device */
> > > > > UCLASS_PCH, /* x86 platform controller hub */
> > > > > UCLASS_PCI, /* PCI bus */
> > > > > UCLASS_PCI_EP, /* PCI endpoint device */
> > > > >
More information about the U-Boot
mailing list