On 10/11/21 16:54, Simon Glass wrote:
Hi Takahiro,

On Sun, 10 Oct 2021 at 20:29, AKASHI Takahiro
<takahiro.aka...@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.aka...@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.

Best regards

Heinrich

Reply via email to