Re: [PATCH] efi_driver: simplify efi_bl_bind()

2021-09-29 Thread Simon Glass
On Mon, 13 Sept 2021 at 00:02, AKASHI Takahiro
 wrote:
>
> blk_create_devicef() is what blk_create_device() +
> device_set_name_alloced() really does.
> The resultant name will be a bit changed.
>
> Signed-off-by: AKASHI Takahiro 
> ---
>  lib/efi_driver/efi_block_device.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
>

Reviewed-by: Simon Glass 


Re: [PATCH] efi_driver: simplify efi_bl_bind()

2021-09-13 Thread AKASHI Takahiro
On Mon, Sep 13, 2021 at 11:19:22AM +0200, Heinrich Schuchardt wrote:
> On 9/13/21 8:01 AM, AKASHI Takahiro wrote:
> > blk_create_devicef() is what blk_create_device() +
> > device_set_name_alloced() really does.
> > The resultant name will be a bit changed.
> 
> without the patch after a device has been generated by the UEFI subsystem:
> 
> => dm tree
> 
> Class Index  Probed  Driver  Name
> 
> -
> root  0  [ + ]   root_driver root_driver
> mmc   2  [ + ]   mmc_sandbox |-- mmc0
> 
> blk   2  [ + ]   mmc_blk |   `-- mmc0.blk
> blk   3  [ + ]   efi_blk `-- efiblk#0
> 
> with the patch:
> 
> => dm tree
> 
> Class Index  Probed  Driver  Name
> 
> -
> 
> root  0  [ + ]   root_driver root_driver
> mmc   2  [ + ]   mmc_sandbox |-- mmc0
> 
> blk   2  [ + ]   mmc_blk |   `-- mmc0.blk
> blk   3  [ + ]   efi_blk `-- root_driver.efiblk#0
> 
> Prepending 'root_driver.' to the name seems to not match the rest of the
> tree.

Is there anything wrong with the new "name"?

blk_create_devicef() creates a name with the parent's name + '.' +
a given name. In this case, you specifies the dm root object
as the device's parent. So "root_driver.efiblk#0" is a natural name
like mmc0.blk and others.

Another block device example:

 root  0  [ + ]   root_driver   root_driver
 pci   0  [ + ]   pci_generic_ecam  |-- pcie@1000
 pci_generi0  [   ]   pci_generic_drv   |   |-- pci_0:0.0
 virtio   32  [ + ]   virtio-pci.l  |   |-- virtio-pci.l#0
 ethernet  0  [ + ]   virtio-net|   |   `-- virtio-net#32
 ahci  0  [ + ]   ahci_pci  |   `-- ahci_pci
 scsi  0  [ + ]   ahci_scsi |   `-- ahci_scsi
 blk   0  [   ]   scsi_blk  |   |-- 
ahci_scsi.id0lun0
 blk   1  [   ]   scsi_blk  |   `-- 
ahci_scsi.id1lun0

If you really don't like "root_driver." prefix, you can create
the "efi" object as the generic parent for EFI devices, and then
you will see "efi.blk#0".
(This is also the way[1] that I took in my previous integration patch.

But in either way, it's just a name. Who cares.

-Takahiro Akashi

[1] https://lists.denx.de/pipermail/u-boot/2019-February/357929.html

> Best regards
> 
> Heinrich
> 
> > 
> > Signed-off-by: AKASHI Takahiro 
> > ---
> >   lib/efi_driver/efi_block_device.c | 10 +++---
> >   1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/efi_driver/efi_block_device.c 
> > b/lib/efi_driver/efi_block_device.c
> > index 0937e3595a43..b81c75868eb4 100644
> > --- a/lib/efi_driver/efi_block_device.c
> > +++ b/lib/efi_driver/efi_block_device.c
> > @@ -159,15 +159,11 @@ static int efi_bl_bind(efi_handle_t handle, void 
> > *interface)
> > sprintf(name, "efiblk#%d", devnum);
> > 
> > /* Create driver model udevice for the EFI block io device */
> > -   ret = blk_create_device(parent, "efi_blk", name, IF_TYPE_EFI, devnum,
> > -   io->media->block_size,
> > -   (lbaint_t)io->media->last_block, );
> > +   ret = blk_create_devicef(parent, "efi_blk", name, IF_TYPE_EFI, devnum,
> > +io->media->block_size,
> > +(lbaint_t)io->media->last_block, );
> > if (ret)
> > return ret;
> > -   if (!bdev)
> > -   return -ENOENT;
> > -   /* Set the DM_FLAG_NAME_ALLOCED flag to avoid a memory leak */
> > -   device_set_name_alloced(bdev);
> > 
> > plat = dev_get_plat(bdev);
> > plat->handle = handle;
> > 
> 


Re: [PATCH] efi_driver: simplify efi_bl_bind()

2021-09-13 Thread Heinrich Schuchardt

On 9/13/21 8:01 AM, AKASHI Takahiro wrote:

blk_create_devicef() is what blk_create_device() +
device_set_name_alloced() really does.
The resultant name will be a bit changed.


without the patch after a device has been generated by the UEFI subsystem:

=> dm tree

Class Index  Probed  Driver  Name

-
root  0  [ + ]   root_driver root_driver
mmc   2  [ + ]   mmc_sandbox |-- mmc0

blk   2  [ + ]   mmc_blk |   `-- mmc0.blk
blk   3  [ + ]   efi_blk `-- efiblk#0

with the patch:

=> dm tree

Class Index  Probed  Driver  Name

-

root  0  [ + ]   root_driver root_driver
mmc   2  [ + ]   mmc_sandbox |-- mmc0

blk   2  [ + ]   mmc_blk |   `-- mmc0.blk
blk   3  [ + ]   efi_blk `-- root_driver.efiblk#0

Prepending 'root_driver.' to the name seems to not match the rest of the
tree.

Best regards

Heinrich



Signed-off-by: AKASHI Takahiro 
---
  lib/efi_driver/efi_block_device.c | 10 +++---
  1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/lib/efi_driver/efi_block_device.c 
b/lib/efi_driver/efi_block_device.c
index 0937e3595a43..b81c75868eb4 100644
--- a/lib/efi_driver/efi_block_device.c
+++ b/lib/efi_driver/efi_block_device.c
@@ -159,15 +159,11 @@ static int efi_bl_bind(efi_handle_t handle, void 
*interface)
sprintf(name, "efiblk#%d", devnum);

/* Create driver model udevice for the EFI block io device */
-   ret = blk_create_device(parent, "efi_blk", name, IF_TYPE_EFI, devnum,
-   io->media->block_size,
-   (lbaint_t)io->media->last_block, );
+   ret = blk_create_devicef(parent, "efi_blk", name, IF_TYPE_EFI, devnum,
+io->media->block_size,
+(lbaint_t)io->media->last_block, );
if (ret)
return ret;
-   if (!bdev)
-   return -ENOENT;
-   /* Set the DM_FLAG_NAME_ALLOCED flag to avoid a memory leak */
-   device_set_name_alloced(bdev);

plat = dev_get_plat(bdev);
plat->handle = handle;





[PATCH] efi_driver: simplify efi_bl_bind()

2021-09-13 Thread AKASHI Takahiro
blk_create_devicef() is what blk_create_device() +
device_set_name_alloced() really does.
The resultant name will be a bit changed.

Signed-off-by: AKASHI Takahiro 
---
 lib/efi_driver/efi_block_device.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/lib/efi_driver/efi_block_device.c 
b/lib/efi_driver/efi_block_device.c
index 0937e3595a43..b81c75868eb4 100644
--- a/lib/efi_driver/efi_block_device.c
+++ b/lib/efi_driver/efi_block_device.c
@@ -159,15 +159,11 @@ static int efi_bl_bind(efi_handle_t handle, void 
*interface)
sprintf(name, "efiblk#%d", devnum);
 
/* Create driver model udevice for the EFI block io device */
-   ret = blk_create_device(parent, "efi_blk", name, IF_TYPE_EFI, devnum,
-   io->media->block_size,
-   (lbaint_t)io->media->last_block, );
+   ret = blk_create_devicef(parent, "efi_blk", name, IF_TYPE_EFI, devnum,
+io->media->block_size,
+(lbaint_t)io->media->last_block, );
if (ret)
return ret;
-   if (!bdev)
-   return -ENOENT;
-   /* Set the DM_FLAG_NAME_ALLOCED flag to avoid a memory leak */
-   device_set_name_alloced(bdev);
 
plat = dev_get_plat(bdev);
plat->handle = handle;
-- 
2.33.0