[RFC 07/22] dm: blk: add UCLASS_PARTITION
Heinrich Schuchardt
xypron.glpk at gmx.de
Tue Oct 12 08:42:07 CEST 2021
Am 12. Oktober 2021 07:12:59 MESZ schrieb AKASHI Takahiro <takahiro.akashi at linaro.org>:
>Simon, Heinrich,
>
>On Mon, Oct 11, 2021 at 11:41:02AM -0600, Simon Glass wrote:
>> Hi Heinrich,,
>>
>> On Mon, 11 Oct 2021 at 10:53, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>> >
>> >
>> >
>> > On 10/11/21 18:14, Simon Glass wrote:
>> > > Hi Heinrich,
>> > >
>> > > On Mon, 11 Oct 2021 at 09:02, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>> > >>
>> > >>
>> > >>
>> > >> On 10/11/21 16:54, Simon Glass wrote:
>> > >>> Hi Takahiro,
>> > >>>
>> > >>> On Sun, 10 Oct 2021 at 20:29, AKASHI Takahiro
>> > >>> <takahiro.akashi at linaro.org> wrote:
>> > >>>>
>> > >>>> 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?
>> > >>>
>> > >>> [..]
>> > >>>
>> > >>> Related to this patch I think that the partition type should be really
>> > >>> be a child of the media device:
>> > >>>
>> > >>> - MMC
>> > >>> |- BLK
>> > >>> |- PARTITION
>> > >>> |- BLK
>> > >>> |- PARTITION
>> > >>> |- BLK
>> > >>> |- PARTITION
>> > >>> |- BLK
>> > >>>
>> > >>> It seems more natural to me that putting the partitions under the
>> > >>> top-level BLK device, so that BLK remains a 'terminal' device.
>> > >>>
>> > >>> The partition uclass is different from BLK, of course. It could
>> > >>> contain information about the partition such as its partition number
>> > >>> and UUID.
>> > >>
>> > >> Do you mean hardware partition here? Otherwise I would not know what BLK
>> > >> should model.
>> > >
>> > > I mean that (I think) we should not use BLK to model partitions. A BLK
>> > > should just be a block device.
>> >
>> > That is fine. But this implies that a software partition is the child of
>> > a block partition and not the other way round. So the tree should like:
>> >
>> > MMC
>> > |- BLK (user hardware partition)
>> > ||- PARTITION 1 (software partition)
>> > ||- PARTITION 2 (software partition)
>> > |...
>> > ||- PARTITION n (software partition)
>> > |- BLK (rpmb hardware partition)
>> > |- BLK (boot0 hardware partition)
>> > |- BLK (boot1 hardware partition)
>>
>> I presume you meant to include a BLK device under each PARTITION?
>
>I do understand that PARTITION is not BLK, but thinking that there is always
>one-to-one link between a PARTITION device and a BLK device, what benefit
>will we see in distinguishing one from the other?
>I'm afraid that this makes things a bit complicated.
>
>> But anyway, I was more thinking of this:
>>
>> MMC
>> | HWPARTITION rpmb
>> || BLK whole rpmb
>> || PARTITION 1
>> ||| BLK
>> || PARTITION 2
>> ||| BLK
>> || PARTITION 3
>> ||| BLK
>> | HWPARTITION boot0
>> || BLK
>> (maybe have PARTITION in here too?
>> | HWPARTITION boot1
>> (maybe have PARTITION in here too?
>> || BLK
>
>I simply wonder why not we see "HWPARTITION" as yet another block device
>[controller] (like scsi LUN's and/or NVME namespaces as discussed in
>a thread of "part: call part_init() in blk_get_device_by_str() only for
>MMC"?).
>
>> >
>> > >
>> > > I don't see any difference between a partition and a hardware
>> > > partition. We presumably end up with a hierarchy though. Do we need a
>> > > HWPARTITION uclass so we can handle the hardware partitions
>> > > differently?
>> >
>> > Software partitions are defined and discovered via partition tables.
>> > Hardware partitions are defined in a hardware specific way.
>> >
>> > All software partitions map to HD() device tree nodes in UEFI.
>> > An MMC device maps to an eMMC() node
>> > MMC hardware partitions are mapped to Ctrl() nodes by EDK II. We should
>> > do the same in U-Boot.
>> > An SD-card maps to an SD() node.
>> > An NVMe namespace maps to a NVMe() node.
>> > An SCSI LUN maps to a Scsi() node.
>> > SCSI channels of multiple channel controllers are mapped to Ctrl() nodes.
>>
>> I'm not quite sure about the terminology here. I'm not even talking
>> about UEFI, really, just how best to model this stuff in U-Boot.
>
>Anyhow, if we pursue the direction that you suggested here,
>we will end up with having one PARTITION *driver* per partition type,
>efi, dos or iso, under disk/?
We already have one driver per partition table type. They just need to be integrated into the DM model. Partitions are defined by first and last block. They don't depend on the type of the partition table. But you need separate drivers per filesystem.
The block device needs a pointer to the partition table driver.
The partition needs a pointer to the filesystem driver.
Best regards
Heinrich
>
># Oops, extra work :)
>
>Thanks,
>-Takahiro Akashi
>
>
>> In U-Boot, UCLASS_SCSI should be a SCSI controller, not a device,
>> right? I'm a little worried it is not modelled correctly. After all,
>> what is the parent of a SCSI device?
>>
>> >
>> > The simple file protocol is only provided by HD() nodes and not by nodes
>> > representing hardware partitions. If the whole hardware partition is
>> > formatted as a file system you would still create a HD() node with
>> > partition number 0.
>>
>> Regards,
>> Simon
More information about the U-Boot
mailing list