Re: [U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION
Hi Takahiro,
On Mon, 28 Jan 2019 at 19:59, AKASHI Takahiro
wrote:
>
> UCLASS_PARTITION device will be created as a child node of
> UCLASS_BLK device.
>
> Signed-off-by: AKASHI Takahiro
> ---
> drivers/block/blk-uclass.c | 52 ++
> include/dm/uclass-id.h | 1 +
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index baaf431e5e0c..d4ca30f23fc1 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -10,6 +10,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> static const char *if_typename_str[IF_TYPE_COUNT] = {
> [IF_TYPE_IDE] = "ide",
> @@ -654,3 +656,53 @@ UCLASS_DRIVER(blk) = {
> .post_probe = blk_post_probe,
> .per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
> };
> +
> +U_BOOT_DRIVER(blk_partition) = {
> + .name = "blk_partition",
> + .id = UCLASS_PARTITION,
> + .platdata_auto_alloc_size = sizeof(struct disk_part),
> +};
> +
> +UCLASS_DRIVER(partition) = {
> + .id = UCLASS_PARTITION,
> + .name = "partition",
> +};
> +
> +#if defined(CONFIG_PARTITIONS) && defined(CONFIG_HAVE_BLOCK_DEVICE)
> +int blk_create_partitions(struct udevice *parent)
> +{
> + int part;
> + struct blk_desc *desc = dev_get_uclass_platdata(parent);
> + disk_partition_t info;
> + struct disk_part *part_data;
> + char devname[32];
> + struct udevice *dev;
> + int disks = 0, ret;
> +
> + /* Add devices for each partition */
> + for (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_platdata(dev);
> + part_data->partnum = part;
> + part_data->gpt_part_info = info;
> +
> + disks++;
> + }
> +
> + return disks;
> +}
> +#else
> +int blk_create_partitions(struct udevice *dev)
> +{
> + return 0;
> +}
> +#endif
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index f3bafb3c6353..e02b5f8fda42 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -65,6 +65,7 @@ enum uclass_id {
> UCLASS_NVME,/* NVM Express device */
> 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_GENERIC, /* Generic PCI bus device */
> --
> 2.19.1
>
This looks OK to me. It does need a test in test/dm/partition.c for
the new uclass.
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION
On Tue, Jan 29, 2019 at 11:20:01PM +0100, Heinrich Schuchardt wrote:
> On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
> > UCLASS_PARTITION device will be created as a child node of
> > UCLASS_BLK device.
> >
> > Signed-off-by: AKASHI Takahiro
> > ---
> > drivers/block/blk-uclass.c | 52 ++
> > include/dm/uclass-id.h | 1 +
> > 2 files changed, 53 insertions(+)
> >
> > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> > index baaf431e5e0c..d4ca30f23fc1 100644
> > --- a/drivers/block/blk-uclass.c
> > +++ b/drivers/block/blk-uclass.c
> > @@ -10,6 +10,8 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> >
> > static const char *if_typename_str[IF_TYPE_COUNT] = {
> > [IF_TYPE_IDE] = "ide",
> > @@ -654,3 +656,53 @@ UCLASS_DRIVER(blk) = {
> > .post_probe = blk_post_probe,
> > .per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
> > };
> > +
> > +U_BOOT_DRIVER(blk_partition) = {
> > + .name = "blk_partition",
> > + .id = UCLASS_PARTITION,
> > + .platdata_auto_alloc_size = sizeof(struct disk_part),
> > +};
> > +
> > +UCLASS_DRIVER(partition) = {
> > + .id = UCLASS_PARTITION,
> > + .name = "partition",
> > +};
> > +
> > +#if defined(CONFIG_PARTITIONS) && defined(CONFIG_HAVE_BLOCK_DEVICE)
> > +int blk_create_partitions(struct udevice *parent)
> > +{
> > + int part;
> > + struct blk_desc *desc = dev_get_uclass_platdata(parent);
> > + disk_partition_t info;
> > + struct disk_part *part_data;
> > + char devname[32];
> > + struct udevice *dev;
> > + int disks = 0, ret;
> > +
> > + /* Add devices for each partition */
> > + for (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)
>
> This looks like a memory leak for the output of strdup().
Yes, I'm aware of that, but please note that this is a prototype
and so I haven't paid much attention to failure cases (error recovery).
First of all, even in the current implementation, we don't support
*unplugging* (or unbind in EFI jargon?) devices.
It's a more fundamental issue.
> > + return ret;
>
> Why would we leave here if one partition fails?
> Does this imply that all further partitions will fail?
> Should we use continue here?
Ditto. Please be patient for the time being :)
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> > +
> > + part_data = dev_get_uclass_platdata(dev);
> > + part_data->partnum = part;
> > + part_data->gpt_part_info = info;
> > +
> > + disks++;
> > + }
> > +
> > + return disks;
> > +}
> > +#else
> > +int blk_create_partitions(struct udevice *dev)
> > +{
> > + return 0;
> > +}
> > +#endif
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index f3bafb3c6353..e02b5f8fda42 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -65,6 +65,7 @@ enum uclass_id {
> > UCLASS_NVME,/* NVM Express device */
> > 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_GENERIC, /* Generic PCI bus device */
> >
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION
On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
> UCLASS_PARTITION device will be created as a child node of
> UCLASS_BLK device.
>
> Signed-off-by: AKASHI Takahiro
> ---
> drivers/block/blk-uclass.c | 52 ++
> include/dm/uclass-id.h | 1 +
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index baaf431e5e0c..d4ca30f23fc1 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -10,6 +10,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> static const char *if_typename_str[IF_TYPE_COUNT] = {
> [IF_TYPE_IDE] = "ide",
> @@ -654,3 +656,53 @@ UCLASS_DRIVER(blk) = {
> .post_probe = blk_post_probe,
> .per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
> };
> +
> +U_BOOT_DRIVER(blk_partition) = {
> + .name = "blk_partition",
> + .id = UCLASS_PARTITION,
> + .platdata_auto_alloc_size = sizeof(struct disk_part),
> +};
> +
> +UCLASS_DRIVER(partition) = {
> + .id = UCLASS_PARTITION,
> + .name = "partition",
> +};
> +
> +#if defined(CONFIG_PARTITIONS) && defined(CONFIG_HAVE_BLOCK_DEVICE)
> +int blk_create_partitions(struct udevice *parent)
> +{
> + int part;
> + struct blk_desc *desc = dev_get_uclass_platdata(parent);
> + disk_partition_t info;
> + struct disk_part *part_data;
> + char devname[32];
> + struct udevice *dev;
> + int disks = 0, ret;
> +
> + /* Add devices for each partition */
> + for (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)
This looks like a memory leak for the output of strdup().
> + return ret;
Why would we leave here if one partition fails?
Does this imply that all further partitions will fail?
Should we use continue here?
Best regards
Heinrich
> +
> + part_data = dev_get_uclass_platdata(dev);
> + part_data->partnum = part;
> + part_data->gpt_part_info = info;
> +
> + disks++;
> + }
> +
> + return disks;
> +}
> +#else
> +int blk_create_partitions(struct udevice *dev)
> +{
> + return 0;
> +}
> +#endif
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index f3bafb3c6353..e02b5f8fda42 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -65,6 +65,7 @@ enum uclass_id {
> UCLASS_NVME,/* NVM Express device */
> 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_GENERIC, /* Generic PCI bus device */
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION
> On 29.01.2019, at 18:46, Sergey Kubushyn wrote: > > On Tue, 29 Jan 2019, Alexander Graf wrote: > >> On 01/29/2019 04:17 AM, Sergey Kubushyn wrote: >>> On Tue, 29 Jan 2019, AKASHI Takahiro wrote: >>> >>> My $.25 -- IMHO, block device partitions in Device Tree is _NOT_ a very >>> good >>> idea. >> >> This is about device model, not device tree :). Device trees indeed should >> not contain partition information. Our internal object model however would >> do well if it had them. > > DM assumes using Device Tree. As there’s no compatible-string, this indeed is for the internal object model only. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION
On Tue, 29 Jan 2019, Alexander Graf wrote: On 01/29/2019 04:17 AM, Sergey Kubushyn wrote: On Tue, 29 Jan 2019, AKASHI Takahiro wrote: My $.25 -- IMHO, block device partitions in Device Tree is _NOT_ a very good idea. This is about device model, not device tree :). Device trees indeed should not contain partition information. Our internal object model however would do well if it had them. DM assumes using Device Tree. --- ** * KSI@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION
On 01/29/2019 04:17 AM, Sergey Kubushyn wrote: On Tue, 29 Jan 2019, AKASHI Takahiro wrote: My $.25 -- IMHO, block device partitions in Device Tree is _NOT_ a very good idea. This is about device model, not device tree :). Device trees indeed should not contain partition information. Our internal object model however would do well if it had them. Alex ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION
On Tue, 29 Jan 2019, AKASHI Takahiro wrote: My $.25 -- IMHO, block device partitions in Device Tree is _NOT_ a very good idea. What if one decided to re-partition his drive? Or just use bigger or smaller drive? Would he has to re-write DTS file and re-compile everything? And such change is not something extraordinary, it is a routine action. IMHO partitions do _NOT_ belong to Block Device. That's what partition tables are. It _MIGHT_ make sense for some particular devices such as MTD that don't have partition tables but _NOT_ for Block Devices in general. UCLASS_PARTITION device will be created as a child node of UCLASS_BLK device. Signed-off-by: AKASHI Takahiro --- ** * KSI@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [RFC 1/3] dm: blk: add UCLASS_PARTITION
UCLASS_PARTITION device will be created as a child node of
UCLASS_BLK device.
Signed-off-by: AKASHI Takahiro
---
drivers/block/blk-uclass.c | 52 ++
include/dm/uclass-id.h | 1 +
2 files changed, 53 insertions(+)
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index baaf431e5e0c..d4ca30f23fc1 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -10,6 +10,8 @@
#include
#include
#include
+#include
+#include
static const char *if_typename_str[IF_TYPE_COUNT] = {
[IF_TYPE_IDE] = "ide",
@@ -654,3 +656,53 @@ UCLASS_DRIVER(blk) = {
.post_probe = blk_post_probe,
.per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
};
+
+U_BOOT_DRIVER(blk_partition) = {
+ .name = "blk_partition",
+ .id = UCLASS_PARTITION,
+ .platdata_auto_alloc_size = sizeof(struct disk_part),
+};
+
+UCLASS_DRIVER(partition) = {
+ .id = UCLASS_PARTITION,
+ .name = "partition",
+};
+
+#if defined(CONFIG_PARTITIONS) && defined(CONFIG_HAVE_BLOCK_DEVICE)
+int blk_create_partitions(struct udevice *parent)
+{
+ int part;
+ struct blk_desc *desc = dev_get_uclass_platdata(parent);
+ disk_partition_t info;
+ struct disk_part *part_data;
+ char devname[32];
+ struct udevice *dev;
+ int disks = 0, ret;
+
+ /* Add devices for each partition */
+ for (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_platdata(dev);
+ part_data->partnum = part;
+ part_data->gpt_part_info = info;
+
+ disks++;
+ }
+
+ return disks;
+}
+#else
+int blk_create_partitions(struct udevice *dev)
+{
+ return 0;
+}
+#endif
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index f3bafb3c6353..e02b5f8fda42 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -65,6 +65,7 @@ enum uclass_id {
UCLASS_NVME,/* NVM Express device */
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_GENERIC, /* Generic PCI bus device */
--
2.19.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
