diff mbox series

[RFC,01/22] part: call part_init() in blk_get_device_by_str() only for MMC

Message ID 20211001050228.55183-2-takahiro.akashi@linaro.org
State Superseded
Headers show
Series efi_loader: more tightly integrate UEFI disks to device model | expand

Commit Message

AKASHI Takahiro Oct. 1, 2021, 5:01 a.m. UTC
In blk_get_device_by_str(), the comment says: "Updates the partition table
for the specified hw partition."
Since hw partition is supported only on MMC, it makes no sense to do so
for other devices.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

---
 disk/part.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.33.0

Comments

Heinrich Schuchardt Oct. 1, 2021, 6:41 a.m. UTC | #1
On 10/1/21 7:01 AM, AKASHI Takahiro wrote:
> In blk_get_device_by_str(), the comment says: "Updates the partition table

> for the specified hw partition."

> Since hw partition is supported only on MMC, it makes no sense to do so

> for other devices.

>

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> ---

>   disk/part.c | 3 ++-

>   1 file changed, 2 insertions(+), 1 deletion(-)

>

> diff --git a/disk/part.c b/disk/part.c

> index a6a8f7052bd3..b330103a5bc0 100644

> --- a/disk/part.c

> +++ b/disk/part.c

> @@ -427,7 +427,8 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,

>   	 * Always should be done, otherwise hw partition 0 will return stale

>   	 * data after displaying a non-zero hw partition.

>   	 */

> -	part_init(*dev_desc);

> +	if ((*dev_desc)->if_type == IF_TYPE_MMC)

> +		part_init(*dev_desc);


For an eMMC the following logical levels exist:

* device
* hardware partition
* software partition

Linux might show the following:

/dev/mmcblk0 - user data area
/dev/mmcblk0boot0 - boot hardware partition 0
/dev/mmcblk0boot1 - boot hardware partition 1
/dev/mmcblk0rpmb - replay protected memory block

How are the different hardware partition modeled in the UEFI device path?
Should each hardware partition be a separate udevice?

For NOR flash we also have an extra level:

=> setenv mtdparts
mtdparts=30bb0000.qspi:1m(U-Boot),512k(Env),512k(DTB),2m(User_FS),12m(Data_FS),4m(Factory_FS),34m(Ramdisk),10m(Linux)
=> mtd

device nor0 <30bb0000.qspi>, # parts = 8
  #: name                size            offset          mask_flags
  0: U-Boot              0x00100000      0x00000000      0
  1: Env                 0x00080000      0x00100000      0
  2: DTB                 0x00080000      0x00180000      0
  3: User_FS             0x00200000      0x00200000      0
  4: Data_FS             0x00c00000      0x00400000      0
  5: Factory_FS          0x00400000      0x01000000      0
  6: Ramdisk             0x02200000      0x01400000      0
  7: Linux               0x00a00000      0x03600000      0

active partition: nor0,0 - (U-Boot) 0x00100000 @ 0x00000000

Has part_info() to be called here too? What is the if_type?
What are the devicepaths for these partitions?

Best regards

Heinrich

>   #endif

>

>   cleanup:

>
Heinrich Schuchardt Oct. 1, 2021, 7:56 a.m. UTC | #2
On 10/1/21 08:41, Heinrich Schuchardt wrote:
>

>

> On 10/1/21 7:01 AM, AKASHI Takahiro wrote:

>> In blk_get_device_by_str(), the comment says: "Updates the partition

>> table

>> for the specified hw partition."

>> Since hw partition is supported only on MMC, it makes no sense to do so

>> for other devices.

>>

>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

>> ---

>>   disk/part.c | 3 ++-

>>   1 file changed, 2 insertions(+), 1 deletion(-)

>>

>> diff --git a/disk/part.c b/disk/part.c

>> index a6a8f7052bd3..b330103a5bc0 100644

>> --- a/disk/part.c

>> +++ b/disk/part.c

>> @@ -427,7 +427,8 @@ int blk_get_device_by_str(const char *ifname,

>> const char *dev_hwpart_str,

>>        * Always should be done, otherwise hw partition 0 will return

>> stale

>>        * data after displaying a non-zero hw partition.

>>        */

>> -    part_init(*dev_desc);

>> +    if ((*dev_desc)->if_type == IF_TYPE_MMC)

>> +        part_init(*dev_desc);

>

> For an eMMC the following logical levels exist:

>

> * device

> * hardware partition

> * software partition

>

> Linux might show the following:

>

> /dev/mmcblk0 - user data area

> /dev/mmcblk0boot0 - boot hardware partition 0

> /dev/mmcblk0boot1 - boot hardware partition 1

> /dev/mmcblk0rpmb - replay protected memory block

>

> How are the different hardware partition modeled in the UEFI device path?


This is a list of block devices with their Linux name and the device
path in EDK II as seen on my MacchicatoBIN board:

/dev/mmcblk0
VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00006EF00000000000)/eMMC(0x0)/Ctrl(0x0)

/dev/mmcblk0p1
VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00006EF00000000000)/eMMC(0x0)/Ctrl(0x0)/HD(1,GPT,C53259E6-9B1D-40CD-B4EF-39520011EF2B,0x2000,0x3FE001)

/dev/mmcblk0p2
VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00006EF00000000000)/eMMC(0x0)/Ctrl(0x0)/HD(2,GPT,A0AE9901-B847-4150-B947-6C8FF69529A7,0x400800,0xA8F7DF)

/dev/mmcblk0boot0
VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00006EF00000000000)/eMMC(0x0)/Ctrl(0x1)

/dev/mmcblk0boot1
VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00006EF00000000000)/eMMC(0x0)/Ctrl(0x2)

> Should each hardware partition be a separate udevice?


We need a udevice matching the hardware partition. These child devices
should be created when the eMMC device is probed.

Best regards

Heinrich

>

> For NOR flash we also have an extra level:

>

> => setenv mtdparts

> mtdparts=30bb0000.qspi:1m(U-Boot),512k(Env),512k(DTB),2m(User_FS),12m(Data_FS),4m(Factory_FS),34m(Ramdisk),10m(Linux)

>

> => mtd

>

> device nor0 <30bb0000.qspi>, # parts = 8

>   #: name                size            offset          mask_flags

>   0: U-Boot              0x00100000      0x00000000      0

>   1: Env                 0x00080000      0x00100000      0

>   2: DTB                 0x00080000      0x00180000      0

>   3: User_FS             0x00200000      0x00200000      0

>   4: Data_FS             0x00c00000      0x00400000      0

>   5: Factory_FS          0x00400000      0x01000000      0

>   6: Ramdisk             0x02200000      0x01400000      0

>   7: Linux               0x00a00000      0x03600000      0

>

> active partition: nor0,0 - (U-Boot) 0x00100000 @ 0x00000000

>

> Has part_info() to be called here too? What is the if_type?

> What are the devicepaths for these partitions?

>

> Best regards

>

> Heinrich

>

>>   #endif

>>

>>   cleanup:

>>
Peter Robinson Oct. 1, 2021, 11:48 a.m. UTC | #3
On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>

> In blk_get_device_by_str(), the comment says: "Updates the partition table

> for the specified hw partition."

> Since hw partition is supported only on MMC, it makes no sense to do so

> for other devices.


Is it not also supported on UFS, and I believe it may also be an
option in the NVME spec too.

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> ---

>  disk/part.c | 3 ++-

>  1 file changed, 2 insertions(+), 1 deletion(-)

>

> diff --git a/disk/part.c b/disk/part.c

> index a6a8f7052bd3..b330103a5bc0 100644

> --- a/disk/part.c

> +++ b/disk/part.c

> @@ -427,7 +427,8 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,

>          * Always should be done, otherwise hw partition 0 will return stale

>          * data after displaying a non-zero hw partition.

>          */

> -       part_init(*dev_desc);

> +       if ((*dev_desc)->if_type == IF_TYPE_MMC)

> +               part_init(*dev_desc);

>  #endif

>

>  cleanup:

> --

> 2.33.0

>
AKASHI Takahiro Oct. 4, 2021, 3:13 a.m. UTC | #4
Heinrich,

On Fri, Oct 01, 2021 at 08:41:52AM +0200, Heinrich Schuchardt wrote:
> 

> 

> On 10/1/21 7:01 AM, AKASHI Takahiro wrote:

> > In blk_get_device_by_str(), the comment says: "Updates the partition table

> > for the specified hw partition."

> > Since hw partition is supported only on MMC, it makes no sense to do so

> > for other devices.

> > 

> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> > ---

> >   disk/part.c | 3 ++-

> >   1 file changed, 2 insertions(+), 1 deletion(-)

> > 

> > diff --git a/disk/part.c b/disk/part.c

> > index a6a8f7052bd3..b330103a5bc0 100644

> > --- a/disk/part.c

> > +++ b/disk/part.c

> > @@ -427,7 +427,8 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,

> >   	 * Always should be done, otherwise hw partition 0 will return stale

> >   	 * data after displaying a non-zero hw partition.

> >   	 */

> > -	part_init(*dev_desc);

> > +	if ((*dev_desc)->if_type == IF_TYPE_MMC)

> > +		part_init(*dev_desc);

> 

> For an eMMC the following logical levels exist:

> 

> * device

> * hardware partition

> * software partition

> 

> Linux might show the following:

> 

> /dev/mmcblk0 - user data area

> /dev/mmcblk0boot0 - boot hardware partition 0

> /dev/mmcblk0boot1 - boot hardware partition 1

> /dev/mmcblk0rpmb - replay protected memory block

> 

> How are the different hardware partition modeled in the UEFI device path?

> Should each hardware partition be a separate udevice?

> 

> For NOR flash we also have an extra level:

> 

> => setenv mtdparts

> mtdparts=30bb0000.qspi:1m(U-Boot),512k(Env),512k(DTB),2m(User_FS),12m(Data_FS),4m(Factory_FS),34m(Ramdisk),10m(Linux)

> => mtd

> 

> device nor0 <30bb0000.qspi>, # parts = 8

>  #: name                size            offset          mask_flags

>  0: U-Boot              0x00100000      0x00000000      0

>  1: Env                 0x00080000      0x00100000      0

>  2: DTB                 0x00080000      0x00180000      0

>  3: User_FS             0x00200000      0x00200000      0

>  4: Data_FS             0x00c00000      0x00400000      0

>  5: Factory_FS          0x00400000      0x01000000      0

>  6: Ramdisk             0x02200000      0x01400000      0

>  7: Linux               0x00a00000      0x03600000      0

> 

> active partition: nor0,0 - (U-Boot) 0x00100000 @ 0x00000000

> 

> Has part_info() to be called here too? What is the if_type?

> What are the devicepaths for these partitions?


You have good points.
MMC's hw partitions and mtd's "environment variable-based" partitions
are quite different from the rest of table-based software partitions
in terms of U-Boot block device implementation.
Both neither are handled by part_info() (under disk/* framework)
nor have dedicated "if_type's".

Even if we might have to address those issues at the end, I would like to
keep them out of my scope for now as it requires a lot of extra work.

# I wonder if we should support mtdparts partitions on U-Boot UEFI
# as it is a quite U-Boot specific feature.

-Takahiro Akashi


> Best regards

> 

> Heinrich

> 

> >   #endif

> > 

> >   cleanup:

> >
AKASHI Takahiro Oct. 4, 2021, 3:26 a.m. UTC | #5
Hi Peter,

On Fri, Oct 01, 2021 at 12:48:24PM +0100, Peter Robinson wrote:
> On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro

> <takahiro.akashi@linaro.org> wrote:

> >

> > In blk_get_device_by_str(), the comment says: "Updates the partition table

> > for the specified hw partition."

> > Since hw partition is supported only on MMC, it makes no sense to do so

> > for other devices.

> 

> Is it not also supported on UFS, and I believe it may also be an

> option in the NVME spec too.


Yeah, maybe.
But under the current implementation, IIUC, neither UFS nor NVME supports
hw partitions as both drivers do not provide a "select_hwpart" function
in blk_ops.
(UFS is seen as an instance of SCSI.)

-Takahiro Akashi


> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> > ---

> >  disk/part.c | 3 ++-

> >  1 file changed, 2 insertions(+), 1 deletion(-)

> >

> > diff --git a/disk/part.c b/disk/part.c

> > index a6a8f7052bd3..b330103a5bc0 100644

> > --- a/disk/part.c

> > +++ b/disk/part.c

> > @@ -427,7 +427,8 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,

> >          * Always should be done, otherwise hw partition 0 will return stale

> >          * data after displaying a non-zero hw partition.

> >          */

> > -       part_init(*dev_desc);

> > +       if ((*dev_desc)->if_type == IF_TYPE_MMC)

> > +               part_init(*dev_desc);

> >  #endif

> >

> >  cleanup:

> > --

> > 2.33.0

> >
Simon Glass Oct. 10, 2021, 2:14 p.m. UTC | #6
On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>

> In blk_get_device_by_str(), the comment says: "Updates the partition table

> for the specified hw partition."

> Since hw partition is supported only on MMC, it makes no sense to do so

> for other devices.

>

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> ---

>  disk/part.c | 3 ++-

>  1 file changed, 2 insertions(+), 1 deletion(-)


Reviewed-by: Simon Glass <sjg@chromium.org>
Heinrich Schuchardt Oct. 11, 2021, 10:07 a.m. UTC | #7
On 10/1/21 13:48, Peter Robinson wrote:
> On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro

> <takahiro.akashi@linaro.org> wrote:

>>

>> In blk_get_device_by_str(), the comment says: "Updates the partition table

>> for the specified hw partition."

>> Since hw partition is supported only on MMC, it makes no sense to do so

>> for other devices.

>

> Is it not also supported on UFS, and I believe it may also be an

> option in the NVME spec too.


An NVMe device may expose multiple namespaces. blk_create_devicef() is
called for each namespace.

A SCSI device may have multiple LUNs. blk_create_devicef() is called for
each LUN.

This is what the tree shown by 'dm tree' with on NVMe namespace and one LUN.

Class  Index Driver         Name
---------------------------------------------------------------------
root       0 root_driver    root_driver
simple_bus 0 simple_bus     |- soc
spi        1 sifive_spi     |  |- spi@10050000
mmc        0 mmc_spi        |  |  `- mmc@0
blk        0 mmc_blk        |  |     `- mmc@0.blk
pci        0 pcie_sifive    |  |- pcie@e00000000
pci        1 pci_bridge_drv |  |  `- pci_0:0.0
pci        2 pci_bridge_drv |  |     `- pci_1:0.0
pci        5 pci_bridge_drv |  |        |- pci_2:3.0
ahci       0 ahci_pci       |  |        |  `- ahci_pci
scsi       0 ahci_scsi      |  |        |     `- ahci_scsi
blk        2 scsi_blk       |  |        |        `- ahci_scsi.id0lun0
pci        6 pci_bridge_drv |  |        |- pci_2:4.0
nvme       0 nvme           |  |        |  `- nvme#0
blk        1 nvme-blk       |  |        |     `- nvme#0.blk#1

Namespaces and LUNs are modeled as block devices (class = 'blk').

Best regards

Heinrich

>

>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

>> ---

>>   disk/part.c | 3 ++-

>>   1 file changed, 2 insertions(+), 1 deletion(-)

>>

>> diff --git a/disk/part.c b/disk/part.c

>> index a6a8f7052bd3..b330103a5bc0 100644

>> --- a/disk/part.c

>> +++ b/disk/part.c

>> @@ -427,7 +427,8 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,

>>           * Always should be done, otherwise hw partition 0 will return stale

>>           * data after displaying a non-zero hw partition.

>>           */

>> -       part_init(*dev_desc);

>> +       if ((*dev_desc)->if_type == IF_TYPE_MMC)

>> +               part_init(*dev_desc);

>>   #endif

>>

>>   cleanup:

>> --

>> 2.33.0

>>
Simon Glass Oct. 11, 2021, 2:32 p.m. UTC | #8
Hi Heinrich,

On Mon, 11 Oct 2021 at 04:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>

>

>

> On 10/1/21 13:48, Peter Robinson wrote:

> > On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro

> > <takahiro.akashi@linaro.org> wrote:

> >>

> >> In blk_get_device_by_str(), the comment says: "Updates the partition table

> >> for the specified hw partition."

> >> Since hw partition is supported only on MMC, it makes no sense to do so

> >> for other devices.

> >

> > Is it not also supported on UFS, and I believe it may also be an

> > option in the NVME spec too.

>

> An NVMe device may expose multiple namespaces. blk_create_devicef() is

> called for each namespace.

>

> A SCSI device may have multiple LUNs. blk_create_devicef() is called for

> each LUN.

>

> This is what the tree shown by 'dm tree' with on NVMe namespace and one LUN.

>

> Class  Index Driver         Name

> ---------------------------------------------------------------------

> root       0 root_driver    root_driver

> simple_bus 0 simple_bus     |- soc

> spi        1 sifive_spi     |  |- spi@10050000

> mmc        0 mmc_spi        |  |  `- mmc@0

> blk        0 mmc_blk        |  |     `- mmc@0.blk

> pci        0 pcie_sifive    |  |- pcie@e00000000

> pci        1 pci_bridge_drv |  |  `- pci_0:0.0

> pci        2 pci_bridge_drv |  |     `- pci_1:0.0

> pci        5 pci_bridge_drv |  |        |- pci_2:3.0

> ahci       0 ahci_pci       |  |        |  `- ahci_pci

> scsi       0 ahci_scsi      |  |        |     `- ahci_scsi

> blk        2 scsi_blk       |  |        |        `- ahci_scsi.id0lun0

> pci        6 pci_bridge_drv |  |        |- pci_2:4.0

> nvme       0 nvme           |  |        |  `- nvme#0

> blk        1 nvme-blk       |  |        |     `- nvme#0.blk#1

>

> Namespaces and LUNs are modeled as block devices (class = 'blk').


So multiple block devices per NVMe device? I did not know that was supported.

We need a sandbox driver for NVMe as it has no tests at present. Since
it has no tests, I don't think we can expect people to know how to
maintain whatever functionality is there.

[..]

Regards,
Simon
Heinrich Schuchardt Oct. 11, 2021, 3:08 p.m. UTC | #9
On 10/11/21 16:32, Simon Glass wrote:
> Hi Heinrich,

>

> On Mon, 11 Oct 2021 at 04:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

>>

>>

>>

>> On 10/1/21 13:48, Peter Robinson wrote:

>>> On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro

>>> <takahiro.akashi@linaro.org> wrote:

>>>>

>>>> In blk_get_device_by_str(), the comment says: "Updates the partition table

>>>> for the specified hw partition."

>>>> Since hw partition is supported only on MMC, it makes no sense to do so

>>>> for other devices.

>>>

>>> Is it not also supported on UFS, and I believe it may also be an

>>> option in the NVME spec too.

>>

>> An NVMe device may expose multiple namespaces. blk_create_devicef() is

>> called for each namespace.

>>

>> A SCSI device may have multiple LUNs. blk_create_devicef() is called for

>> each LUN.

>>

>> This is what the tree shown by 'dm tree' with on NVMe namespace and one LUN.

>>

>> Class  Index Driver         Name

>> ---------------------------------------------------------------------

>> root       0 root_driver    root_driver

>> simple_bus 0 simple_bus     |- soc

>> spi        1 sifive_spi     |  |- spi@10050000

>> mmc        0 mmc_spi        |  |  `- mmc@0

>> blk        0 mmc_blk        |  |     `- mmc@0.blk

>> pci        0 pcie_sifive    |  |- pcie@e00000000

>> pci        1 pci_bridge_drv |  |  `- pci_0:0.0

>> pci        2 pci_bridge_drv |  |     `- pci_1:0.0

>> pci        5 pci_bridge_drv |  |        |- pci_2:3.0

>> ahci       0 ahci_pci       |  |        |  `- ahci_pci

>> scsi       0 ahci_scsi      |  |        |     `- ahci_scsi

>> blk        2 scsi_blk       |  |        |        `- ahci_scsi.id0lun0

>> pci        6 pci_bridge_drv |  |        |- pci_2:4.0

>> nvme       0 nvme           |  |        |  `- nvme#0

>> blk        1 nvme-blk       |  |        |     `- nvme#0.blk#1

>>

>> Namespaces and LUNs are modeled as block devices (class = 'blk').

>

> So multiple block devices per NVMe device? I did not know that was supported.

>

> We need a sandbox driver for NVMe as it has no tests at present. Since

> it has no tests, I don't think we can expect people to know how to

> maintain whatever functionality is there.


NVMe drives with multiple namespaces exist for servers but not for
consumer NVMe drives.

In QEMU you can define an NVMe device with multiple namespaces. Cf.
https://qemu.readthedocs.io/en/latest/system/devices/nvme.html?highlight=namespace#additional-namespaces

So for a first glimpse at the handling I suggest to use QEMU.

Best regards

Heinrich
Simon Glass Oct. 11, 2021, 4:14 p.m. UTC | #10
Hi Heinrich,

On Mon, 11 Oct 2021 at 09:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>

>

>

> On 10/11/21 16:32, Simon Glass wrote:

> > Hi Heinrich,

> >

> > On Mon, 11 Oct 2021 at 04:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> >>

> >>

> >>

> >> On 10/1/21 13:48, Peter Robinson wrote:

> >>> On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro

> >>> <takahiro.akashi@linaro.org> wrote:

> >>>>

> >>>> In blk_get_device_by_str(), the comment says: "Updates the partition table

> >>>> for the specified hw partition."

> >>>> Since hw partition is supported only on MMC, it makes no sense to do so

> >>>> for other devices.

> >>>

> >>> Is it not also supported on UFS, and I believe it may also be an

> >>> option in the NVME spec too.

> >>

> >> An NVMe device may expose multiple namespaces. blk_create_devicef() is

> >> called for each namespace.

> >>

> >> A SCSI device may have multiple LUNs. blk_create_devicef() is called for

> >> each LUN.

> >>

> >> This is what the tree shown by 'dm tree' with on NVMe namespace and one LUN.

> >>

> >> Class  Index Driver         Name

> >> ---------------------------------------------------------------------

> >> root       0 root_driver    root_driver

> >> simple_bus 0 simple_bus     |- soc

> >> spi        1 sifive_spi     |  |- spi@10050000

> >> mmc        0 mmc_spi        |  |  `- mmc@0

> >> blk        0 mmc_blk        |  |     `- mmc@0.blk

> >> pci        0 pcie_sifive    |  |- pcie@e00000000

> >> pci        1 pci_bridge_drv |  |  `- pci_0:0.0

> >> pci        2 pci_bridge_drv |  |     `- pci_1:0.0

> >> pci        5 pci_bridge_drv |  |        |- pci_2:3.0

> >> ahci       0 ahci_pci       |  |        |  `- ahci_pci

> >> scsi       0 ahci_scsi      |  |        |     `- ahci_scsi

> >> blk        2 scsi_blk       |  |        |        `- ahci_scsi.id0lun0

> >> pci        6 pci_bridge_drv |  |        |- pci_2:4.0

> >> nvme       0 nvme           |  |        |  `- nvme#0

> >> blk        1 nvme-blk       |  |        |     `- nvme#0.blk#1

> >>

> >> Namespaces and LUNs are modeled as block devices (class = 'blk').

> >

> > So multiple block devices per NVMe device? I did not know that was supported.

> >

> > We need a sandbox driver for NVMe as it has no tests at present. Since

> > it has no tests, I don't think we can expect people to know how to

> > maintain whatever functionality is there.

>

> NVMe drives with multiple namespaces exist for servers but not for

> consumer NVMe drives.

>

> In QEMU you can define an NVMe device with multiple namespaces. Cf.

> https://qemu.readthedocs.io/en/latest/system/devices/nvme.html?highlight=namespace#additional-namespaces

>

> So for a first glimpse at the handling I suggest to use QEMU.


Well that's fine, but every uclass must have a test and a sandbox
emulator as well.

Regards,
Simon
AKASHI Takahiro Oct. 12, 2021, 3:26 a.m. UTC | #11
Simon, Heinrich,

On Mon, Oct 11, 2021 at 10:14:02AM -0600, Simon Glass wrote:
> Hi Heinrich,

> 

> On Mon, 11 Oct 2021 at 09:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> >

> >

> >

> > On 10/11/21 16:32, Simon Glass wrote:

> > > Hi Heinrich,

> > >

> > > On Mon, 11 Oct 2021 at 04:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> > >>

> > >>

> > >>

> > >> On 10/1/21 13:48, Peter Robinson wrote:

> > >>> On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro

> > >>> <takahiro.akashi@linaro.org> wrote:

> > >>>>

> > >>>> In blk_get_device_by_str(), the comment says: "Updates the partition table

> > >>>> for the specified hw partition."

> > >>>> Since hw partition is supported only on MMC, it makes no sense to do so

> > >>>> for other devices.

> > >>>

> > >>> Is it not also supported on UFS, and I believe it may also be an

> > >>> option in the NVME spec too.

> > >>

> > >> An NVMe device may expose multiple namespaces. blk_create_devicef() is

> > >> called for each namespace.

> > >>

> > >> A SCSI device may have multiple LUNs. blk_create_devicef() is called for

> > >> each LUN.

> > >>

> > >> This is what the tree shown by 'dm tree' with on NVMe namespace and one LUN.

> > >>

> > >> Class  Index Driver         Name

> > >> ---------------------------------------------------------------------

> > >> root       0 root_driver    root_driver

> > >> simple_bus 0 simple_bus     |- soc

> > >> spi        1 sifive_spi     |  |- spi@10050000

> > >> mmc        0 mmc_spi        |  |  `- mmc@0

> > >> blk        0 mmc_blk        |  |     `- mmc@0.blk

> > >> pci        0 pcie_sifive    |  |- pcie@e00000000

> > >> pci        1 pci_bridge_drv |  |  `- pci_0:0.0

> > >> pci        2 pci_bridge_drv |  |     `- pci_1:0.0

> > >> pci        5 pci_bridge_drv |  |        |- pci_2:3.0

> > >> ahci       0 ahci_pci       |  |        |  `- ahci_pci

> > >> scsi       0 ahci_scsi      |  |        |     `- ahci_scsi

> > >> blk        2 scsi_blk       |  |        |        `- ahci_scsi.id0lun0

> > >> pci        6 pci_bridge_drv |  |        |- pci_2:4.0

> > >> nvme       0 nvme           |  |        |  `- nvme#0

> > >> blk        1 nvme-blk       |  |        |     `- nvme#0.blk#1

> > >>

> > >> Namespaces and LUNs are modeled as block devices (class = 'blk').

> > >

> > > So multiple block devices per NVMe device? I did not know that was supported.

> > >

> > > We need a sandbox driver for NVMe as it has no tests at present. Since

> > > it has no tests, I don't think we can expect people to know how to

> > > maintain whatever functionality is there.

> >

> > NVMe drives with multiple namespaces exist for servers but not for

> > consumer NVMe drives.

> >

> > In QEMU you can define an NVMe device with multiple namespaces. Cf.

> > https://qemu.readthedocs.io/en/latest/system/devices/nvme.html?highlight=namespace#additional-namespaces

> >

> > So for a first glimpse at the handling I suggest to use QEMU.

> 

> Well that's fine, but every uclass must have a test and a sandbox

> emulator as well.


Wait, it seems that you're discussing a different thing from my patch.

While I don't know whether NVMe namespaces are a kind of "HW partitions",
we don't care much here as long as any namespace can be handled simply
as a normal block device, like scsi LUN's, in terms of U-Boot driver model.

# 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).
# See do_mmcrpmb() in cmd/mmc.c

So I hope that *your* discussion doesn't make any difference to my patch.
Right?

-Takahiro Akashi


> Regards,

> Simon
Heinrich Schuchardt Oct. 12, 2021, 1:30 p.m. UTC | #12
On 10/12/21 05:26, AKASHI Takahiro wrote:
> Simon, Heinrich,

>

> On Mon, Oct 11, 2021 at 10:14:02AM -0600, Simon Glass wrote:

>> Hi Heinrich,

>>

>> On Mon, 11 Oct 2021 at 09:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

>>>

>>>

>>>

>>> On 10/11/21 16:32, Simon Glass wrote:

>>>> Hi Heinrich,

>>>>

>>>> On Mon, 11 Oct 2021 at 04:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

>>>>>

>>>>>

>>>>>

>>>>> On 10/1/21 13:48, Peter Robinson wrote:

>>>>>> On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro

>>>>>> <takahiro.akashi@linaro.org> wrote:

>>>>>>>

>>>>>>> In blk_get_device_by_str(), the comment says: "Updates the partition table

>>>>>>> for the specified hw partition."

>>>>>>> Since hw partition is supported only on MMC, it makes no sense to do so

>>>>>>> for other devices.

>>>>>>

>>>>>> Is it not also supported on UFS, and I believe it may also be an

>>>>>> option in the NVME spec too.

>>>>>

>>>>> An NVMe device may expose multiple namespaces. blk_create_devicef() is

>>>>> called for each namespace.

>>>>>

>>>>> A SCSI device may have multiple LUNs. blk_create_devicef() is called for

>>>>> each LUN.

>>>>>

>>>>> This is what the tree shown by 'dm tree' with on NVMe namespace and one LUN.

>>>>>

>>>>> Class  Index Driver         Name

>>>>> ---------------------------------------------------------------------

>>>>> root       0 root_driver    root_driver

>>>>> simple_bus 0 simple_bus     |- soc

>>>>> spi        1 sifive_spi     |  |- spi@10050000

>>>>> mmc        0 mmc_spi        |  |  `- mmc@0

>>>>> blk        0 mmc_blk        |  |     `- mmc@0.blk

>>>>> pci        0 pcie_sifive    |  |- pcie@e00000000

>>>>> pci        1 pci_bridge_drv |  |  `- pci_0:0.0

>>>>> pci        2 pci_bridge_drv |  |     `- pci_1:0.0

>>>>> pci        5 pci_bridge_drv |  |        |- pci_2:3.0

>>>>> ahci       0 ahci_pci       |  |        |  `- ahci_pci

>>>>> scsi       0 ahci_scsi      |  |        |     `- ahci_scsi

>>>>> blk        2 scsi_blk       |  |        |        `- ahci_scsi.id0lun0

>>>>> pci        6 pci_bridge_drv |  |        |- pci_2:4.0

>>>>> nvme       0 nvme           |  |        |  `- nvme#0

>>>>> blk        1 nvme-blk       |  |        |     `- nvme#0.blk#1

>>>>>

>>>>> Namespaces and LUNs are modeled as block devices (class = 'blk').

>>>>

>>>> So multiple block devices per NVMe device? I did not know that was supported.

>>>>

>>>> We need a sandbox driver for NVMe as it has no tests at present. Since

>>>> it has no tests, I don't think we can expect people to know how to

>>>> maintain whatever functionality is there.

>>>

>>> NVMe drives with multiple namespaces exist for servers but not for

>>> consumer NVMe drives.

>>>

>>> In QEMU you can define an NVMe device with multiple namespaces. Cf.

>>> https://qemu.readthedocs.io/en/latest/system/devices/nvme.html?highlight=namespace#additional-namespaces

>>>

>>> So for a first glimpse at the handling I suggest to use QEMU.

>>

>> Well that's fine, but every uclass must have a test and a sandbox

>> emulator as well.

>

> Wait, it seems that you're discussing a different thing from my patch.

>

> While I don't know whether NVMe namespaces are a kind of "HW partitions",

> we don't care much here as long as any namespace can be handled simply

> as a normal block device, like scsi LUN's, in terms of U-Boot driver model.

>

> # 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).

> # See do_mmcrpmb() in cmd/mmc.c


Each hardware partition should be a block device (class blk) which is
mirrored in the UEFI world by a CTRL() device. It is not necessary for
parent device to be a block device.

Best regards

Heinrich

>

> So I hope that *your* discussion doesn't make any difference to my patch.

> Right?

>

> -Takahiro Akashi

>

>

>> Regards,

>> Simon
Simon Glass Oct. 12, 2021, 8:31 p.m. UTC | #13
Hi Takahiro,

On Mon, 11 Oct 2021 at 21:26, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>

> Simon, Heinrich,

>

> On Mon, Oct 11, 2021 at 10:14:02AM -0600, Simon Glass wrote:

> > Hi Heinrich,

> >

> > On Mon, 11 Oct 2021 at 09:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> > >

> > >

> > >

> > > On 10/11/21 16:32, Simon Glass wrote:

> > > > Hi Heinrich,

> > > >

> > > > On Mon, 11 Oct 2021 at 04:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> > > >>

> > > >>

> > > >>

> > > >> On 10/1/21 13:48, Peter Robinson wrote:

> > > >>> On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro

> > > >>> <takahiro.akashi@linaro.org> wrote:

> > > >>>>

> > > >>>> In blk_get_device_by_str(), the comment says: "Updates the partition table

> > > >>>> for the specified hw partition."

> > > >>>> Since hw partition is supported only on MMC, it makes no sense to do so

> > > >>>> for other devices.

> > > >>>

> > > >>> Is it not also supported on UFS, and I believe it may also be an

> > > >>> option in the NVME spec too.

> > > >>

> > > >> An NVMe device may expose multiple namespaces. blk_create_devicef() is

> > > >> called for each namespace.

> > > >>

> > > >> A SCSI device may have multiple LUNs. blk_create_devicef() is called for

> > > >> each LUN.

> > > >>

> > > >> This is what the tree shown by 'dm tree' with on NVMe namespace and one LUN.

> > > >>

> > > >> Class  Index Driver         Name

> > > >> ---------------------------------------------------------------------

> > > >> root       0 root_driver    root_driver

> > > >> simple_bus 0 simple_bus     |- soc

> > > >> spi        1 sifive_spi     |  |- spi@10050000

> > > >> mmc        0 mmc_spi        |  |  `- mmc@0

> > > >> blk        0 mmc_blk        |  |     `- mmc@0.blk

> > > >> pci        0 pcie_sifive    |  |- pcie@e00000000

> > > >> pci        1 pci_bridge_drv |  |  `- pci_0:0.0

> > > >> pci        2 pci_bridge_drv |  |     `- pci_1:0.0

> > > >> pci        5 pci_bridge_drv |  |        |- pci_2:3.0

> > > >> ahci       0 ahci_pci       |  |        |  `- ahci_pci

> > > >> scsi       0 ahci_scsi      |  |        |     `- ahci_scsi

> > > >> blk        2 scsi_blk       |  |        |        `- ahci_scsi.id0lun0

> > > >> pci        6 pci_bridge_drv |  |        |- pci_2:4.0

> > > >> nvme       0 nvme           |  |        |  `- nvme#0

> > > >> blk        1 nvme-blk       |  |        |     `- nvme#0.blk#1

> > > >>

> > > >> Namespaces and LUNs are modeled as block devices (class = 'blk').

> > > >

> > > > So multiple block devices per NVMe device? I did not know that was supported.

> > > >

> > > > We need a sandbox driver for NVMe as it has no tests at present. Since

> > > > it has no tests, I don't think we can expect people to know how to

> > > > maintain whatever functionality is there.

> > >

> > > NVMe drives with multiple namespaces exist for servers but not for

> > > consumer NVMe drives.

> > >

> > > In QEMU you can define an NVMe device with multiple namespaces. Cf.

> > > https://qemu.readthedocs.io/en/latest/system/devices/nvme.html?highlight=namespace#additional-namespaces

> > >

> > > So for a first glimpse at the handling I suggest to use QEMU.

> >

> > Well that's fine, but every uclass must have a test and a sandbox

> > emulator as well.

>

> Wait, it seems that you're discussing a different thing from my patch.

>

> While I don't know whether NVMe namespaces are a kind of "HW partitions",

> we don't care much here as long as any namespace can be handled simply

> as a normal block device, like scsi LUN's, in terms of U-Boot driver model.

>

> # 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).

> # See do_mmcrpmb() in cmd/mmc.c

>

> So I hope that *your* discussion doesn't make any difference to my patch.

> Right?


From my POV the patch is fine, which is why I added the review tag.

We should continue the discussion on BLK versus PARTITION.

Regards,
Simon
AKASHI Takahiro Oct. 13, 2021, 1:50 a.m. UTC | #14
On Tue, Oct 12, 2021 at 03:30:26PM +0200, Heinrich Schuchardt wrote:
> 

> 

> On 10/12/21 05:26, AKASHI Takahiro wrote:

> > Simon, Heinrich,

> > 

> > On Mon, Oct 11, 2021 at 10:14:02AM -0600, Simon Glass wrote:

> > > Hi Heinrich,

> > > 

> > > On Mon, 11 Oct 2021 at 09:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> > > > 

> > > > 

> > > > 

> > > > On 10/11/21 16:32, Simon Glass wrote:

> > > > > Hi Heinrich,

> > > > > 

> > > > > On Mon, 11 Oct 2021 at 04:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> > > > > > 

> > > > > > 

> > > > > > 

> > > > > > On 10/1/21 13:48, Peter Robinson wrote:

> > > > > > > On Fri, Oct 1, 2021 at 6:03 AM AKASHI Takahiro

> > > > > > > <takahiro.akashi@linaro.org> wrote:

> > > > > > > > 

> > > > > > > > In blk_get_device_by_str(), the comment says: "Updates the partition table

> > > > > > > > for the specified hw partition."

> > > > > > > > Since hw partition is supported only on MMC, it makes no sense to do so

> > > > > > > > for other devices.

> > > > > > > 

> > > > > > > Is it not also supported on UFS, and I believe it may also be an

> > > > > > > option in the NVME spec too.

> > > > > > 

> > > > > > An NVMe device may expose multiple namespaces. blk_create_devicef() is

> > > > > > called for each namespace.

> > > > > > 

> > > > > > A SCSI device may have multiple LUNs. blk_create_devicef() is called for

> > > > > > each LUN.

> > > > > > 

> > > > > > This is what the tree shown by 'dm tree' with on NVMe namespace and one LUN.

> > > > > > 

> > > > > > Class  Index Driver         Name

> > > > > > ---------------------------------------------------------------------

> > > > > > root       0 root_driver    root_driver

> > > > > > simple_bus 0 simple_bus     |- soc

> > > > > > spi        1 sifive_spi     |  |- spi@10050000

> > > > > > mmc        0 mmc_spi        |  |  `- mmc@0

> > > > > > blk        0 mmc_blk        |  |     `- mmc@0.blk

> > > > > > pci        0 pcie_sifive    |  |- pcie@e00000000

> > > > > > pci        1 pci_bridge_drv |  |  `- pci_0:0.0

> > > > > > pci        2 pci_bridge_drv |  |     `- pci_1:0.0

> > > > > > pci        5 pci_bridge_drv |  |        |- pci_2:3.0

> > > > > > ahci       0 ahci_pci       |  |        |  `- ahci_pci

> > > > > > scsi       0 ahci_scsi      |  |        |     `- ahci_scsi

> > > > > > blk        2 scsi_blk       |  |        |        `- ahci_scsi.id0lun0

> > > > > > pci        6 pci_bridge_drv |  |        |- pci_2:4.0

> > > > > > nvme       0 nvme           |  |        |  `- nvme#0

> > > > > > blk        1 nvme-blk       |  |        |     `- nvme#0.blk#1

> > > > > > 

> > > > > > Namespaces and LUNs are modeled as block devices (class = 'blk').

> > > > > 

> > > > > So multiple block devices per NVMe device? I did not know that was supported.

> > > > > 

> > > > > We need a sandbox driver for NVMe as it has no tests at present. Since

> > > > > it has no tests, I don't think we can expect people to know how to

> > > > > maintain whatever functionality is there.

> > > > 

> > > > NVMe drives with multiple namespaces exist for servers but not for

> > > > consumer NVMe drives.

> > > > 

> > > > In QEMU you can define an NVMe device with multiple namespaces. Cf.

> > > > https://qemu.readthedocs.io/en/latest/system/devices/nvme.html?highlight=namespace#additional-namespaces

> > > > 

> > > > So for a first glimpse at the handling I suggest to use QEMU.

> > > 

> > > Well that's fine, but every uclass must have a test and a sandbox

> > > emulator as well.

> > 

> > Wait, it seems that you're discussing a different thing from my patch.

> > 

> > While I don't know whether NVMe namespaces are a kind of "HW partitions",

> > we don't care much here as long as any namespace can be handled simply

> > as a normal block device, like scsi LUN's, in terms of U-Boot driver model.

> > 

> > # 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).

> > # See do_mmcrpmb() in cmd/mmc.c

> 

> Each hardware partition should be a block device (class blk) which is

> mirrored in the UEFI world by a CTRL() device.


Yes, whether it is mirrored or not, a hw partition is to be
a separate udevice from its associated raw device.

> It is not necessary for

> parent device to be a block device.


I'm not sure what 'parent device' means here, but I guess that it is
the raw MMC device (as a controller handle in UEFI terminology which
is set to provide BLOCK_IO_PROTOCOL), isn't it?


-Takahiro Akashi

> Best regards

> 

> Heinrich

> 

> > 

> > So I hope that *your* discussion doesn't make any difference to my patch.

> > Right?

> > 

> > -Takahiro Akashi

> > 

> > 

> > > Regards,

> > > Simon
diff mbox series

Patch

diff --git a/disk/part.c b/disk/part.c
index a6a8f7052bd3..b330103a5bc0 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -427,7 +427,8 @@  int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,
 	 * Always should be done, otherwise hw partition 0 will return stale
 	 * data after displaying a non-zero hw partition.
 	 */
-	part_init(*dev_desc);
+	if ((*dev_desc)->if_type == IF_TYPE_MMC)
+		part_init(*dev_desc);
 #endif
 
 cleanup: