| Message ID | 20190129025956.21870-2-takahiro.akashi@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | dm, efi: integrate efi_disk into DM | expand |
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 <takahiro.akashi@linaro.org> --- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
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
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@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
> On 29.01.2019, at 18:46, Sergey Kubushyn <ksi@koi8.net> 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.
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 <takahiro.akashi@linaro.org> > --- > 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 <dm/device-internal.h> > #include <dm/lists.h> > #include <dm/uclass-internal.h> > +#include <part.h> > +#include <string.h> > > 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 */ >
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 <takahiro.akashi@linaro.org> > > --- > > 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 <dm/device-internal.h> > > #include <dm/lists.h> > > #include <dm/uclass-internal.h> > > +#include <part.h> > > +#include <string.h> > > > > 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 */ > > >
Hi Takahiro, On Mon, 28 Jan 2019 at 19:59, AKASHI Takahiro <takahiro.akashi@linaro.org> 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 | 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 <dm/device-internal.h> > #include <dm/lists.h> > #include <dm/uclass-internal.h> > +#include <part.h> > +#include <string.h> > > 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
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 <dm/device-internal.h> #include <dm/lists.h> #include <dm/uclass-internal.h> +#include <part.h> +#include <string.h> 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 */
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 | 52 ++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + 2 files changed, 53 insertions(+)