Hi Takahiro,
On Tue, 12 Oct 2021 at 19:32, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Oct 12, 2021 at 11:14:17AM -0400, Tom Rini wrote:
On Mon, Oct 11, 2021 at 10:14:00AM -0600, Simon Glass wrote:
Hi Heinrich,
On Mon, 11 Oct 2021 at 09:02, Heinrich Schuchardt xypron.glpk@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@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@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.
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?
Note that for eMMC devices, hardware partitions are different from partition-table partitions. If you boot a system with an eMMC device up in Linux you typically get mmcblkN, mmcblkNboot0, mmcblkNboot1 and mmcblkNrpmb, each of which are hardware partitions. It gets tricky in U-Boot in that you can access each of these with 'mmc dev N M' where M defaults to 0 and is the user partition (mmcblkN), 1/2 are boot0/boot1 and 3 is the rpmb area. The 'mmc' command also allows, when possible and implemented, configuring these partitions, again to the extent allowed, documented and implemented.
Thank you. That is exactly what I tried to mention in my reply at "part: call part_init() in blk_get_device_by_str() only for MMC"
OK so it sounds like we agree that hwpartition and partition are different things.
---8<--- # On the other hand, we have to explicitly switch "hw partitions" # with blk_select_hwpart_devnum() on MMC devices even though we use # the *same* udevice(blk_desc). --->8---
The problem with the current U-Boot driver model is that all of "mmcblkN, mmcblkNboot0, mmcblkNboot1 and mmcblkNrpmb" will be linked to the same udevice. We have to do "mmc dev N M" or call blk_select_hwpart[_devnum]() to distinguish them.
Here's our chance to rethink this. What should the device hierarchy be for an MMC device? I made a proposal further up the thread.
When it comes to UEFI, I hope we can currently support hw partitions in this way: => efidebug add boot -b 1 FOO mmc 0.1 /foo.bin "" (".1" is a key, I have never tried this syntax though.)
But probably its device path won't be properly formatted as expected as Heinrich suggested.
-Takahiro Akashi
In terms of modeling, this is akin to how if you use a USB card reader that supports 4 different form-factor cards, you can end up with 4 different devices showing up in Linux (if you have one of the nice card readers that supports multiple cards at once).
-- Tom
Regards, Simon