| Message ID | 20220715144749.30564-2-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | enable menu-driven UEFI variable maintenance | expand |
On 7/15/22 16:47, Masahisa Kojima wrote: > This is a preparation patch to provide the unified method > to access udevice pointer associated with the block io device. > The EFI handles of both EFI block io driver implemented in > lib/efi_loader/efi_disk.c and EFI block io driver implemented > as EFI payload can posess the udevice pointer in the struct efi_object. > > We can use this udevice pointer to get the U-Boot friendly > block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > Newly created in v9 > > include/efi_loader.h | 8 ++++++++ > lib/efi_loader/efi_disk.c | 20 +++++++++++++------- > 2 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 3a63a1f75f..bba5ffd482 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); > #define EFI_CACHELINE_SIZE 128 > #endif > > +/** > + * efi_handle_to_udev - accessor to the DM device associated to the EFI handle > + * @handle: pointer to the EFI handle > + */ > +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) This conversion will hide errors if handle is not of type efi_handle_t. We should avoid the conversion and see build time errors instead. Please, remove the macro. For every handle of type efi_handle_t you can access the field handle->dev directly. For struct efi_disk_obj we can use disk->header.dev. > + > /* Key identifying current memory map */ > extern efi_uintn_t efi_memory_map_key; > > @@ -375,6 +381,7 @@ enum efi_object_type { > * @protocols: linked list with the protocol interfaces installed on this > * handle > * @type: image type if the handle relates to an image > + * @dev: pointer to the DM device which is associated with this EFI handle > * > * UEFI offers a flexible and expandable object model. The objects in the UEFI > * API are devices, drivers, and loaded images. struct efi_object is our storage > @@ -392,6 +399,7 @@ struct efi_object { > /* The list of protocols */ > struct list_head protocols; > enum efi_object_type type; > + struct udevice *dev; > }; > > enum efi_image_auth_status { > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 1d700b2a6b..a8e8521e3e 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -46,7 +46,6 @@ struct efi_disk_obj { > struct efi_device_path *dp; > unsigned int part; > struct efi_simple_file_system_protocol *volume; > - struct udevice *dev; /* TODO: move it to efi_object */ ok > }; > > /** > @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, > return EFI_BAD_BUFFER_SIZE; > > if (CONFIG_IS_ENABLED(PARTITIONS) && > - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > + device_get_uclass_id(efi_handle_to_udev(diskobj)) == UCLASS_PARTITION) { device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) { > if (direction == EFI_DISK_READ) > - n = dev_read(diskobj->dev, lba, blocks, buffer); > + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, buffer); dev_read(diskobj->header.hdev) > else > - n = dev_write(diskobj->dev, lba, blocks, buffer); > + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, buffer); dev_write(diskobj->header.hdev) > } else { > /* dev is a block device (UCLASS_BLK) */ > struct blk_desc *desc; > > - desc = dev_get_uclass_plat(diskobj->dev); > + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); dev_get_uclass(diskobj->header.hdev) > if (direction == EFI_DISK_READ) > n = blk_dread(desc, lba, blocks, buffer); > else > @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) > > return -1; > } > - disk->dev = dev; > + efi_handle_to_udev(disk) = dev; > if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > efi_free_pool(disk->dp); > efi_delete_handle(&disk->header); > @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) > log_err("Adding partition for %s failed\n", dev->name); > return -1; > } > - disk->dev = dev; > + efi_handle_to_udev(disk) = dev; disk->header.dev = dev; > if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > efi_free_pool(disk->dp); > efi_delete_handle(&disk->header); > @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event *event) > ret = efi_disk_create_raw(dev); > if (ret) > return -1; > + } else { > + efi_handle_t handle; > + > + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > + return -1; > + > + efi_handle_to_udev(handle) = dev; handle->dev = dev; Best regards Heinrich > } > > device_foreach_child(child, dev) {
On 7/17/22 10:09, Heinrich Schuchardt wrote: > On 7/15/22 16:47, Masahisa Kojima wrote: >> This is a preparation patch to provide the unified method >> to access udevice pointer associated with the block io device. >> The EFI handles of both EFI block io driver implemented in >> lib/efi_loader/efi_disk.c and EFI block io driver implemented >> as EFI payload can posess the udevice pointer in the struct efi_object. >> >> We can use this udevice pointer to get the U-Boot friendly >> block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. >> >> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >> --- >> Newly created in v9 >> >> include/efi_loader.h | 8 ++++++++ >> lib/efi_loader/efi_disk.c | 20 +++++++++++++------- >> 2 files changed, 21 insertions(+), 7 deletions(-) >> >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 3a63a1f75f..bba5ffd482 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); >> #define EFI_CACHELINE_SIZE 128 >> #endif >> >> +/** >> + * efi_handle_to_udev - accessor to the DM device associated to the >> EFI handle >> + * @handle: pointer to the EFI handle >> + */ >> +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) > > This conversion will hide errors if handle is not of type efi_handle_t. > We should avoid the conversion and see build time errors instead. > Please, remove the macro. > > For every handle of type efi_handle_t you can access the field > handle->dev directly. > > For struct efi_disk_obj we can use disk->header.dev. > >> + >> /* Key identifying current memory map */ >> extern efi_uintn_t efi_memory_map_key; >> >> @@ -375,6 +381,7 @@ enum efi_object_type { >> * @protocols: linked list with the protocol interfaces installed >> on this >> * handle >> * @type: image type if the handle relates to an image >> + * @dev: pointer to the DM device which is associated with this >> EFI handle >> * >> * UEFI offers a flexible and expandable object model. The objects >> in the UEFI >> * API are devices, drivers, and loaded images. struct efi_object is >> our storage >> @@ -392,6 +399,7 @@ struct efi_object { >> /* The list of protocols */ >> struct list_head protocols; >> enum efi_object_type type; >> + struct udevice *dev; >> }; >> >> enum efi_image_auth_status { >> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c >> index 1d700b2a6b..a8e8521e3e 100644 >> --- a/lib/efi_loader/efi_disk.c >> +++ b/lib/efi_loader/efi_disk.c >> @@ -46,7 +46,6 @@ struct efi_disk_obj { >> struct efi_device_path *dp; >> unsigned int part; >> struct efi_simple_file_system_protocol *volume; >> - struct udevice *dev; /* TODO: move it to efi_object */ > > ok > >> }; >> >> /** >> @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct >> efi_block_io *this, >> return EFI_BAD_BUFFER_SIZE; >> >> if (CONFIG_IS_ENABLED(PARTITIONS) && >> - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { >> + device_get_uclass_id(efi_handle_to_udev(diskobj)) == >> UCLASS_PARTITION) { > > device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) { > >> if (direction == EFI_DISK_READ) >> - n = dev_read(diskobj->dev, lba, blocks, buffer); >> + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, >> buffer); > > dev_read(diskobj->header.hdev) > >> else >> - n = dev_write(diskobj->dev, lba, blocks, buffer); >> + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, >> buffer); > > dev_write(diskobj->header.hdev) > >> } else { >> /* dev is a block device (UCLASS_BLK) */ >> struct blk_desc *desc; >> >> - desc = dev_get_uclass_plat(diskobj->dev); >> + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); > > dev_get_uclass(diskobj->header.hdev) > > >> if (direction == EFI_DISK_READ) >> n = blk_dread(desc, lba, blocks, buffer); >> else >> @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) >> >> return -1; >> } >> - disk->dev = dev; >> + efi_handle_to_udev(disk) = dev; >> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { >> efi_free_pool(disk->dp); >> efi_delete_handle(&disk->header); >> @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) >> log_err("Adding partition for %s failed\n", dev->name); >> return -1; >> } >> - disk->dev = dev; >> + efi_handle_to_udev(disk) = dev; > > disk->header.dev = dev; > >> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { >> efi_free_pool(disk->dp); >> efi_delete_handle(&disk->header); >> @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event >> *event) >> ret = efi_disk_create_raw(dev); >> if (ret) >> return -1; >> + } else { >> + efi_handle_t handle; >> + >> + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) Setting handle->dev can be done more easily in efi_bl_bind(): handle->dev = bdev; We can further remove the field handle from struct blk_create_device as it is now available in handle->dev. Best regards Heinrich >> + return -1; >> + >> + efi_handle_to_udev(handle) = dev; > > handle->dev = dev; > > Best regards > > Heinrich > >> } >> >> device_foreach_child(child, dev) { >
On Sun, Jul 17, 2022 at 10:09:42AM +0200, Heinrich Schuchardt wrote: > On 7/15/22 16:47, Masahisa Kojima wrote: > > This is a preparation patch to provide the unified method > > to access udevice pointer associated with the block io device. > > The EFI handles of both EFI block io driver implemented in > > lib/efi_loader/efi_disk.c and EFI block io driver implemented > > as EFI payload can posess the udevice pointer in the struct efi_object. > > > > We can use this udevice pointer to get the U-Boot friendly > > block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > Newly created in v9 > > > > include/efi_loader.h | 8 ++++++++ > > lib/efi_loader/efi_disk.c | 20 +++++++++++++------- > > 2 files changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index 3a63a1f75f..bba5ffd482 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); > > #define EFI_CACHELINE_SIZE 128 > > #endif > > > > +/** > > + * efi_handle_to_udev - accessor to the DM device associated to the EFI handle > > + * @handle: pointer to the EFI handle > > + */ > > +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) > > This conversion will hide errors if handle is not of type efi_handle_t. > We should avoid the conversion and see build time errors instead. > Please, remove the macro. I don't think we should remove the macro itself, but only the type casting. I think it is a good practice to hide an implementation how the relationship between udev and efi_object is maintained *behind* accessor macros. > For every handle of type efi_handle_t you can access the field > handle->dev directly. > > For struct efi_disk_obj we can use disk->header.dev. This is a good example for hiding the implementation from the rest of code. > > + > > /* Key identifying current memory map */ > > extern efi_uintn_t efi_memory_map_key; > > > > @@ -375,6 +381,7 @@ enum efi_object_type { > > * @protocols: linked list with the protocol interfaces installed on this > > * handle > > * @type: image type if the handle relates to an image > > + * @dev: pointer to the DM device which is associated with this EFI handle > > * > > * UEFI offers a flexible and expandable object model. The objects in the UEFI > > * API are devices, drivers, and loaded images. struct efi_object is our storage > > @@ -392,6 +399,7 @@ struct efi_object { > > /* The list of protocols */ > > struct list_head protocols; > > enum efi_object_type type; > > + struct udevice *dev; > > }; > > > > enum efi_image_auth_status { > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > index 1d700b2a6b..a8e8521e3e 100644 > > --- a/lib/efi_loader/efi_disk.c > > +++ b/lib/efi_loader/efi_disk.c > > @@ -46,7 +46,6 @@ struct efi_disk_obj { > > struct efi_device_path *dp; > > unsigned int part; > > struct efi_simple_file_system_protocol *volume; > > - struct udevice *dev; /* TODO: move it to efi_object */ > > ok > > > }; > > > > /** > > @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, > > return EFI_BAD_BUFFER_SIZE; > > > > if (CONFIG_IS_ENABLED(PARTITIONS) && > > - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > > + device_get_uclass_id(efi_handle_to_udev(diskobj)) == UCLASS_PARTITION) { > > device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) { > > > if (direction == EFI_DISK_READ) > > - n = dev_read(diskobj->dev, lba, blocks, buffer); > > + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, buffer); > > dev_read(diskobj->header.hdev) > > > else > > - n = dev_write(diskobj->dev, lba, blocks, buffer); > > + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, buffer); > > dev_write(diskobj->header.hdev) > > > } else { > > /* dev is a block device (UCLASS_BLK) */ > > struct blk_desc *desc; > > > > - desc = dev_get_uclass_plat(diskobj->dev); > > + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); > > dev_get_uclass(diskobj->header.hdev) > > > > if (direction == EFI_DISK_READ) > > n = blk_dread(desc, lba, blocks, buffer); > > else > > @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) > > > > return -1; > > } > > - disk->dev = dev; > > + efi_handle_to_udev(disk) = dev; > > if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > > efi_free_pool(disk->dp); > > efi_delete_handle(&disk->header); > > @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) > > log_err("Adding partition for %s failed\n", dev->name); > > return -1; > > } > > - disk->dev = dev; > > + efi_handle_to_udev(disk) = dev; > > disk->header.dev = dev; It's my preference, but I would suggest another accessor: efi_set_udev(handle, dev); to hide an implementation. -Takahiro Akashi > > > if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > > efi_free_pool(disk->dp); > > efi_delete_handle(&disk->header); > > @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event *event) > > ret = efi_disk_create_raw(dev); > > if (ret) > > return -1; > > + } else { > > + efi_handle_t handle; > > + > > + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > > + return -1; > > + > > + efi_handle_to_udev(handle) = dev; > > handle->dev = dev; > > Best regards > > Heinrich > > > } > > > > device_foreach_child(child, dev) { >
On Sun, Jul 17, 2022 at 01:23:41PM +0200, Heinrich Schuchardt wrote: > On 7/17/22 10:09, Heinrich Schuchardt wrote: > > On 7/15/22 16:47, Masahisa Kojima wrote: > > > This is a preparation patch to provide the unified method > > > to access udevice pointer associated with the block io device. > > > The EFI handles of both EFI block io driver implemented in > > > lib/efi_loader/efi_disk.c and EFI block io driver implemented > > > as EFI payload can posess the udevice pointer in the struct efi_object. > > > > > > We can use this udevice pointer to get the U-Boot friendly > > > block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > > --- > > > Newly created in v9 > > > > > > include/efi_loader.h | 8 ++++++++ > > > lib/efi_loader/efi_disk.c | 20 +++++++++++++------- > > > 2 files changed, 21 insertions(+), 7 deletions(-) > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > index 3a63a1f75f..bba5ffd482 100644 > > > --- a/include/efi_loader.h > > > +++ b/include/efi_loader.h > > > @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); > > > #define EFI_CACHELINE_SIZE 128 > > > #endif > > > > > > +/** > > > + * efi_handle_to_udev - accessor to the DM device associated to the > > > EFI handle > > > + * @handle: pointer to the EFI handle > > > + */ > > > +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) > > > > This conversion will hide errors if handle is not of type efi_handle_t. > > We should avoid the conversion and see build time errors instead. > > Please, remove the macro. > > > > For every handle of type efi_handle_t you can access the field > > handle->dev directly. > > > > For struct efi_disk_obj we can use disk->header.dev. > > > > > + > > > /* Key identifying current memory map */ > > > extern efi_uintn_t efi_memory_map_key; > > > > > > @@ -375,6 +381,7 @@ enum efi_object_type { > > > * @protocols: linked list with the protocol interfaces installed > > > on this > > > * handle > > > * @type: image type if the handle relates to an image > > > + * @dev: pointer to the DM device which is associated with this > > > EFI handle > > > * > > > * UEFI offers a flexible and expandable object model. The objects > > > in the UEFI > > > * API are devices, drivers, and loaded images. struct efi_object is > > > our storage > > > @@ -392,6 +399,7 @@ struct efi_object { > > > /* The list of protocols */ > > > struct list_head protocols; > > > enum efi_object_type type; > > > + struct udevice *dev; > > > }; > > > > > > enum efi_image_auth_status { > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > > index 1d700b2a6b..a8e8521e3e 100644 > > > --- a/lib/efi_loader/efi_disk.c > > > +++ b/lib/efi_loader/efi_disk.c > > > @@ -46,7 +46,6 @@ struct efi_disk_obj { > > > struct efi_device_path *dp; > > > unsigned int part; > > > struct efi_simple_file_system_protocol *volume; > > > - struct udevice *dev; /* TODO: move it to efi_object */ > > > > ok > > > > > }; > > > > > > /** > > > @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct > > > efi_block_io *this, > > > return EFI_BAD_BUFFER_SIZE; > > > > > > if (CONFIG_IS_ENABLED(PARTITIONS) && > > > - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > > > + device_get_uclass_id(efi_handle_to_udev(diskobj)) == > > > UCLASS_PARTITION) { > > > > device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) { > > > > > if (direction == EFI_DISK_READ) > > > - n = dev_read(diskobj->dev, lba, blocks, buffer); > > > + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, > > > buffer); > > > > dev_read(diskobj->header.hdev) > > > > > else > > > - n = dev_write(diskobj->dev, lba, blocks, buffer); > > > + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, > > > buffer); > > > > dev_write(diskobj->header.hdev) > > > > > } else { > > > /* dev is a block device (UCLASS_BLK) */ > > > struct blk_desc *desc; > > > > > > - desc = dev_get_uclass_plat(diskobj->dev); > > > + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); > > > > dev_get_uclass(diskobj->header.hdev) > > > > > > > if (direction == EFI_DISK_READ) > > > n = blk_dread(desc, lba, blocks, buffer); > > > else > > > @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) > > > > > > return -1; > > > } > > > - disk->dev = dev; > > > + efi_handle_to_udev(disk) = dev; > > > if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > > > efi_free_pool(disk->dp); > > > efi_delete_handle(&disk->header); > > > @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) > > > log_err("Adding partition for %s failed\n", dev->name); > > > return -1; > > > } > > > - disk->dev = dev; > > > + efi_handle_to_udev(disk) = dev; > > > > disk->header.dev = dev; > > > > > if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > > > efi_free_pool(disk->dp); > > > efi_delete_handle(&disk->header); > > > @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event > > > *event) > > > ret = efi_disk_create_raw(dev); > > > if (ret) > > > return -1; > > > + } else { > > > + efi_handle_t handle; > > > + > > > + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > > Setting handle->dev can be done more easily in efi_bl_bind(): I don't think so. "dev" field should be maintained in *one* place, i.e. efi_disk_probe(), which is uniquely called from probe event (even for efi_block_dev case). To make this clear, we'd better put the code in efi_disk_create_raw(): efi_disk_create_raw() if (desc->if_type == IF_TYPE_EFI_LOADER) { /* handle should exist now */ dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)); efi_set_udev(handle, dev); return 0; } /* normal block devices */ ... efi_set_udev(&disk->header, dev); ... -Takahiro Akashi > handle->dev = bdev; > > We can further remove the field handle from struct blk_create_device as > it is now available in handle->dev. > > Best regards > > Heinrich > > > > + return -1; > > > + > > > + efi_handle_to_udev(handle) = dev; > > > > handle->dev = dev; > > > > Best regards > > > > Heinrich > > > > > } > > > > > > device_foreach_child(child, dev) { > > >
On 7/20/22 07:23, Takahiro Akashi wrote: > On Sun, Jul 17, 2022 at 01:23:41PM +0200, Heinrich Schuchardt wrote: >> On 7/17/22 10:09, Heinrich Schuchardt wrote: >>> On 7/15/22 16:47, Masahisa Kojima wrote: >>>> This is a preparation patch to provide the unified method >>>> to access udevice pointer associated with the block io device. >>>> The EFI handles of both EFI block io driver implemented in >>>> lib/efi_loader/efi_disk.c and EFI block io driver implemented >>>> as EFI payload can posess the udevice pointer in the struct efi_object. >>>> >>>> We can use this udevice pointer to get the U-Boot friendly >>>> block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. >>>> >>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >>>> --- >>>> Newly created in v9 >>>> >>>> include/efi_loader.h | 8 ++++++++ >>>> lib/efi_loader/efi_disk.c | 20 +++++++++++++------- >>>> 2 files changed, 21 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>>> index 3a63a1f75f..bba5ffd482 100644 >>>> --- a/include/efi_loader.h >>>> +++ b/include/efi_loader.h >>>> @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); >>>> #define EFI_CACHELINE_SIZE 128 >>>> #endif >>>> >>>> +/** >>>> + * efi_handle_to_udev - accessor to the DM device associated to the >>>> EFI handle >>>> + * @handle: pointer to the EFI handle >>>> + */ >>>> +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) >>> >>> This conversion will hide errors if handle is not of type efi_handle_t. >>> We should avoid the conversion and see build time errors instead. >>> Please, remove the macro. >>> >>> For every handle of type efi_handle_t you can access the field >>> handle->dev directly. >>> >>> For struct efi_disk_obj we can use disk->header.dev. >>> >>>> + >>>> /* Key identifying current memory map */ >>>> extern efi_uintn_t efi_memory_map_key; >>>> >>>> @@ -375,6 +381,7 @@ enum efi_object_type { >>>> * @protocols: linked list with the protocol interfaces installed >>>> on this >>>> * handle >>>> * @type: image type if the handle relates to an image >>>> + * @dev: pointer to the DM device which is associated with this >>>> EFI handle >>>> * >>>> * UEFI offers a flexible and expandable object model. The objects >>>> in the UEFI >>>> * API are devices, drivers, and loaded images. struct efi_object is >>>> our storage >>>> @@ -392,6 +399,7 @@ struct efi_object { >>>> /* The list of protocols */ >>>> struct list_head protocols; >>>> enum efi_object_type type; >>>> + struct udevice *dev; >>>> }; >>>> >>>> enum efi_image_auth_status { >>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c >>>> index 1d700b2a6b..a8e8521e3e 100644 >>>> --- a/lib/efi_loader/efi_disk.c >>>> +++ b/lib/efi_loader/efi_disk.c >>>> @@ -46,7 +46,6 @@ struct efi_disk_obj { >>>> struct efi_device_path *dp; >>>> unsigned int part; >>>> struct efi_simple_file_system_protocol *volume; >>>> - struct udevice *dev; /* TODO: move it to efi_object */ >>> >>> ok >>> >>>> }; >>>> >>>> /** >>>> @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct >>>> efi_block_io *this, >>>> return EFI_BAD_BUFFER_SIZE; >>>> >>>> if (CONFIG_IS_ENABLED(PARTITIONS) && >>>> - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { >>>> + device_get_uclass_id(efi_handle_to_udev(diskobj)) == >>>> UCLASS_PARTITION) { >>> >>> device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) { >>> >>>> if (direction == EFI_DISK_READ) >>>> - n = dev_read(diskobj->dev, lba, blocks, buffer); >>>> + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, >>>> buffer); >>> >>> dev_read(diskobj->header.hdev) >>> >>>> else >>>> - n = dev_write(diskobj->dev, lba, blocks, buffer); >>>> + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, >>>> buffer); >>> >>> dev_write(diskobj->header.hdev) >>> >>>> } else { >>>> /* dev is a block device (UCLASS_BLK) */ >>>> struct blk_desc *desc; >>>> >>>> - desc = dev_get_uclass_plat(diskobj->dev); >>>> + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); >>> >>> dev_get_uclass(diskobj->header.hdev) >>> >>> >>>> if (direction == EFI_DISK_READ) >>>> n = blk_dread(desc, lba, blocks, buffer); >>>> else >>>> @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) >>>> >>>> return -1; >>>> } >>>> - disk->dev = dev; >>>> + efi_handle_to_udev(disk) = dev; >>>> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { >>>> efi_free_pool(disk->dp); >>>> efi_delete_handle(&disk->header); >>>> @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) >>>> log_err("Adding partition for %s failed\n", dev->name); >>>> return -1; >>>> } >>>> - disk->dev = dev; >>>> + efi_handle_to_udev(disk) = dev; >>> >>> disk->header.dev = dev; >>> >>>> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { >>>> efi_free_pool(disk->dp); >>>> efi_delete_handle(&disk->header); >>>> @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event >>>> *event) >>>> ret = efi_disk_create_raw(dev); >>>> if (ret) >>>> return -1; >>>> + } else { >>>> + efi_handle_t handle; >>>> + >>>> + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) >> >> Setting handle->dev can be done more easily in efi_bl_bind(): > > I don't think so. > "dev" field should be maintained in *one* place, i.e. efi_disk_probe(), > which is uniquely called from probe event (even for efi_block_dev case). > > To make this clear, we'd better put the code in efi_disk_create_raw(): > efi_disk_create_raw() > if (desc->if_type == IF_TYPE_EFI_LOADER) { > /* handle should exist now */ > dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)); > efi_set_udev(handle, dev); > return 0; > } > > /* normal block devices */ > ... > efi_set_udev(&disk->header, dev); > ... Linking devices and handles is not block device specific. If we would properly link all devices and handles, we would be able to generate device paths by walking the driver model device tree. This is the direction into which our future development should go. Can we have a wrapper around dev_tag_set_ptr(dev, DM_TAG_EFI, handle) which takes care of setting handle->dev and the tag replacing all current invocations: int efi_link_dev(efi_handle_t handle, udevice dev) { handle->dev = dev; return dev_tag_set_ptr(dev, DM_TAG_EFI, handle); } Best regards Heinrich > > > -Takahiro Akashi > >> handle->dev = bdev; >> >> We can further remove the field handle from struct blk_create_device as >> it is now available in handle->dev. >> >> Best regards >> >> Heinrich >> >>>> + return -1; >>>> + >>>> + efi_handle_to_udev(handle) = dev; >>> >>> handle->dev = dev; >>> >>> Best regards >>> >>> Heinrich >>> >>>> } >>>> >>>> device_foreach_child(child, dev) { >>> >>
On 7/20/22 01:56, Takahiro Akashi wrote: > On Sun, Jul 17, 2022 at 10:09:42AM +0200, Heinrich Schuchardt wrote: >> On 7/15/22 16:47, Masahisa Kojima wrote: >>> This is a preparation patch to provide the unified method >>> to access udevice pointer associated with the block io device. >>> The EFI handles of both EFI block io driver implemented in >>> lib/efi_loader/efi_disk.c and EFI block io driver implemented >>> as EFI payload can posess the udevice pointer in the struct efi_object. >>> >>> We can use this udevice pointer to get the U-Boot friendly >>> block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. >>> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >>> --- >>> Newly created in v9 >>> >>> include/efi_loader.h | 8 ++++++++ >>> lib/efi_loader/efi_disk.c | 20 +++++++++++++------- >>> 2 files changed, 21 insertions(+), 7 deletions(-) >>> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>> index 3a63a1f75f..bba5ffd482 100644 >>> --- a/include/efi_loader.h >>> +++ b/include/efi_loader.h >>> @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); >>> #define EFI_CACHELINE_SIZE 128 >>> #endif >>> >>> +/** >>> + * efi_handle_to_udev - accessor to the DM device associated to the EFI handle >>> + * @handle: pointer to the EFI handle >>> + */ >>> +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) >> >> This conversion will hide errors if handle is not of type efi_handle_t. >> We should avoid the conversion and see build time errors instead. >> Please, remove the macro. > > I don't think we should remove the macro itself, but only the type casting. > > I think it is a good practice to hide an implementation how the relationship > between udev and efi_object is maintained *behind* accessor macros. > >> For every handle of type efi_handle_t you can access the field >> handle->dev directly. >> >> For struct efi_disk_obj we can use disk->header.dev. > > This is a good example for hiding the implementation from the rest of code. Such a macro is pure code obfuscation. I won't take such a patch. Best regards Heinrich > >>> + >>> /* Key identifying current memory map */ >>> extern efi_uintn_t efi_memory_map_key; >>> >>> @@ -375,6 +381,7 @@ enum efi_object_type { >>> * @protocols: linked list with the protocol interfaces installed on this >>> * handle >>> * @type: image type if the handle relates to an image >>> + * @dev: pointer to the DM device which is associated with this EFI handle >>> * >>> * UEFI offers a flexible and expandable object model. The objects in the UEFI >>> * API are devices, drivers, and loaded images. struct efi_object is our storage >>> @@ -392,6 +399,7 @@ struct efi_object { >>> /* The list of protocols */ >>> struct list_head protocols; >>> enum efi_object_type type; >>> + struct udevice *dev; >>> }; >>> >>> enum efi_image_auth_status { >>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c >>> index 1d700b2a6b..a8e8521e3e 100644 >>> --- a/lib/efi_loader/efi_disk.c >>> +++ b/lib/efi_loader/efi_disk.c >>> @@ -46,7 +46,6 @@ struct efi_disk_obj { >>> struct efi_device_path *dp; >>> unsigned int part; >>> struct efi_simple_file_system_protocol *volume; >>> - struct udevice *dev; /* TODO: move it to efi_object */ >> >> ok >> >>> }; >>> >>> /** >>> @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, >>> return EFI_BAD_BUFFER_SIZE; >>> >>> if (CONFIG_IS_ENABLED(PARTITIONS) && >>> - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { >>> + device_get_uclass_id(efi_handle_to_udev(diskobj)) == UCLASS_PARTITION) { >> >> device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) { >> >>> if (direction == EFI_DISK_READ) >>> - n = dev_read(diskobj->dev, lba, blocks, buffer); >>> + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, buffer); >> >> dev_read(diskobj->header.hdev) >> >>> else >>> - n = dev_write(diskobj->dev, lba, blocks, buffer); >>> + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, buffer); >> >> dev_write(diskobj->header.hdev) >> >>> } else { >>> /* dev is a block device (UCLASS_BLK) */ >>> struct blk_desc *desc; >>> >>> - desc = dev_get_uclass_plat(diskobj->dev); >>> + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); >> >> dev_get_uclass(diskobj->header.hdev) >> >> >>> if (direction == EFI_DISK_READ) >>> n = blk_dread(desc, lba, blocks, buffer); >>> else >>> @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) >>> >>> return -1; >>> } >>> - disk->dev = dev; >>> + efi_handle_to_udev(disk) = dev; >>> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { >>> efi_free_pool(disk->dp); >>> efi_delete_handle(&disk->header); >>> @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) >>> log_err("Adding partition for %s failed\n", dev->name); >>> return -1; >>> } >>> - disk->dev = dev; >>> + efi_handle_to_udev(disk) = dev; >> >> disk->header.dev = dev; > > It's my preference, but I would suggest another accessor: > efi_set_udev(handle, dev); > to hide an implementation. > > -Takahiro Akashi > >> >>> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { >>> efi_free_pool(disk->dp); >>> efi_delete_handle(&disk->header); >>> @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event *event) >>> ret = efi_disk_create_raw(dev); >>> if (ret) >>> return -1; >>> + } else { >>> + efi_handle_t handle; >>> + >>> + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) >>> + return -1; >>> + >>> + efi_handle_to_udev(handle) = dev; >> >> handle->dev = dev; >> >> Best regards >> >> Heinrich >> >>> } >>> >>> device_foreach_child(child, dev) { >>
Hi Heinrich, Akashi-san, On Wed, 20 Jul 2022 at 16:42, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 7/20/22 07:23, Takahiro Akashi wrote: > > On Sun, Jul 17, 2022 at 01:23:41PM +0200, Heinrich Schuchardt wrote: > >> On 7/17/22 10:09, Heinrich Schuchardt wrote: > >>> On 7/15/22 16:47, Masahisa Kojima wrote: > >>>> This is a preparation patch to provide the unified method > >>>> to access udevice pointer associated with the block io device. > >>>> The EFI handles of both EFI block io driver implemented in > >>>> lib/efi_loader/efi_disk.c and EFI block io driver implemented > >>>> as EFI payload can posess the udevice pointer in the struct efi_object. > >>>> > >>>> We can use this udevice pointer to get the U-Boot friendly > >>>> block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. > >>>> > >>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > >>>> --- > >>>> Newly created in v9 > >>>> > >>>> include/efi_loader.h | 8 ++++++++ > >>>> lib/efi_loader/efi_disk.c | 20 +++++++++++++------- > >>>> 2 files changed, 21 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/include/efi_loader.h b/include/efi_loader.h > >>>> index 3a63a1f75f..bba5ffd482 100644 > >>>> --- a/include/efi_loader.h > >>>> +++ b/include/efi_loader.h > >>>> @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); > >>>> #define EFI_CACHELINE_SIZE 128 > >>>> #endif > >>>> > >>>> +/** > >>>> + * efi_handle_to_udev - accessor to the DM device associated to the > >>>> EFI handle > >>>> + * @handle: pointer to the EFI handle > >>>> + */ > >>>> +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) > >>> > >>> This conversion will hide errors if handle is not of type efi_handle_t. > >>> We should avoid the conversion and see build time errors instead. > >>> Please, remove the macro. > >>> > >>> For every handle of type efi_handle_t you can access the field > >>> handle->dev directly. > >>> > >>> For struct efi_disk_obj we can use disk->header.dev. > >>> > >>>> + > >>>> /* Key identifying current memory map */ > >>>> extern efi_uintn_t efi_memory_map_key; > >>>> > >>>> @@ -375,6 +381,7 @@ enum efi_object_type { > >>>> * @protocols: linked list with the protocol interfaces installed > >>>> on this > >>>> * handle > >>>> * @type: image type if the handle relates to an image > >>>> + * @dev: pointer to the DM device which is associated with this > >>>> EFI handle > >>>> * > >>>> * UEFI offers a flexible and expandable object model. The objects > >>>> in the UEFI > >>>> * API are devices, drivers, and loaded images. struct efi_object is > >>>> our storage > >>>> @@ -392,6 +399,7 @@ struct efi_object { > >>>> /* The list of protocols */ > >>>> struct list_head protocols; > >>>> enum efi_object_type type; > >>>> + struct udevice *dev; > >>>> }; > >>>> > >>>> enum efi_image_auth_status { > >>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > >>>> index 1d700b2a6b..a8e8521e3e 100644 > >>>> --- a/lib/efi_loader/efi_disk.c > >>>> +++ b/lib/efi_loader/efi_disk.c > >>>> @@ -46,7 +46,6 @@ struct efi_disk_obj { > >>>> struct efi_device_path *dp; > >>>> unsigned int part; > >>>> struct efi_simple_file_system_protocol *volume; > >>>> - struct udevice *dev; /* TODO: move it to efi_object */ > >>> > >>> ok > >>> > >>>> }; > >>>> > >>>> /** > >>>> @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct > >>>> efi_block_io *this, > >>>> return EFI_BAD_BUFFER_SIZE; > >>>> > >>>> if (CONFIG_IS_ENABLED(PARTITIONS) && > >>>> - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > >>>> + device_get_uclass_id(efi_handle_to_udev(diskobj)) == > >>>> UCLASS_PARTITION) { > >>> > >>> device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) { > >>> > >>>> if (direction == EFI_DISK_READ) > >>>> - n = dev_read(diskobj->dev, lba, blocks, buffer); > >>>> + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, > >>>> buffer); > >>> > >>> dev_read(diskobj->header.hdev) > >>> > >>>> else > >>>> - n = dev_write(diskobj->dev, lba, blocks, buffer); > >>>> + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, > >>>> buffer); > >>> > >>> dev_write(diskobj->header.hdev) > >>> > >>>> } else { > >>>> /* dev is a block device (UCLASS_BLK) */ > >>>> struct blk_desc *desc; > >>>> > >>>> - desc = dev_get_uclass_plat(diskobj->dev); > >>>> + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); > >>> > >>> dev_get_uclass(diskobj->header.hdev) > >>> > >>> > >>>> if (direction == EFI_DISK_READ) > >>>> n = blk_dread(desc, lba, blocks, buffer); > >>>> else > >>>> @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) > >>>> > >>>> return -1; > >>>> } > >>>> - disk->dev = dev; > >>>> + efi_handle_to_udev(disk) = dev; > >>>> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > >>>> efi_free_pool(disk->dp); > >>>> efi_delete_handle(&disk->header); > >>>> @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) > >>>> log_err("Adding partition for %s failed\n", dev->name); > >>>> return -1; > >>>> } > >>>> - disk->dev = dev; > >>>> + efi_handle_to_udev(disk) = dev; > >>> > >>> disk->header.dev = dev; > >>> > >>>> if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > >>>> efi_free_pool(disk->dp); > >>>> efi_delete_handle(&disk->header); > >>>> @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event > >>>> *event) > >>>> ret = efi_disk_create_raw(dev); > >>>> if (ret) > >>>> return -1; > >>>> + } else { > >>>> + efi_handle_t handle; > >>>> + > >>>> + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > >> > >> Setting handle->dev can be done more easily in efi_bl_bind(): > > > > I don't think so. > > "dev" field should be maintained in *one* place, i.e. efi_disk_probe(), > > which is uniquely called from probe event (even for efi_block_dev case). > > > > To make this clear, we'd better put the code in efi_disk_create_raw(): > > efi_disk_create_raw() > > if (desc->if_type == IF_TYPE_EFI_LOADER) { > > /* handle should exist now */ > > dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)); > > efi_set_udev(handle, dev); > > return 0; > > } > > > > /* normal block devices */ > > ... > > efi_set_udev(&disk->header, dev); > > ... > > Linking devices and handles is not block device specific. > > If we would properly link all devices and handles, we would be able to > generate device paths by walking the driver model device tree. This is > the direction into which our future development should go. > > Can we have a wrapper around dev_tag_set_ptr(dev, DM_TAG_EFI, handle) > which takes care of setting handle->dev and the tag replacing all > current invocations: > > int efi_link_dev(efi_handle_t handle, udevice dev) { > handle->dev = dev; > return dev_tag_set_ptr(dev, DM_TAG_EFI, handle); > } Thank you. I create efi_link_dev(). > >> We can further remove the field handle from struct blk_create_device as > >> it is now available in handle->dev. Do you mean struct efi_blk_plat? struct efi_blk_plat { >.......efi_handle_t>...>.......handle; >.......struct efi_block_io>....*io; }; Regards, Masahisa Kojima > > Best regards > > Heinrich > > > > > > > -Takahiro Akashi > > > >> handle->dev = bdev; > >> > >> We can further remove the field handle from struct blk_create_device as > >> it is now available in handle->dev. > >> > >> Best regards > >> > >> Heinrich > >> > >>>> + return -1; > >>>> + > >>>> + efi_handle_to_udev(handle) = dev; > >>> > >>> handle->dev = dev; > >>> > >>> Best regards > >>> > >>> Heinrich > >>> > >>>> } > >>>> > >>>> device_foreach_child(child, dev) { > >>> > >> >
On Wed, Jul 20, 2022 at 09:44:43AM +0200, Heinrich Schuchardt wrote: > On 7/20/22 01:56, Takahiro Akashi wrote: > > On Sun, Jul 17, 2022 at 10:09:42AM +0200, Heinrich Schuchardt wrote: > > > On 7/15/22 16:47, Masahisa Kojima wrote: > > > > This is a preparation patch to provide the unified method > > > > to access udevice pointer associated with the block io device. > > > > The EFI handles of both EFI block io driver implemented in > > > > lib/efi_loader/efi_disk.c and EFI block io driver implemented > > > > as EFI payload can posess the udevice pointer in the struct efi_object. > > > > > > > > We can use this udevice pointer to get the U-Boot friendly > > > > block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. > > > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > > > --- > > > > Newly created in v9 > > > > > > > > include/efi_loader.h | 8 ++++++++ > > > > lib/efi_loader/efi_disk.c | 20 +++++++++++++------- > > > > 2 files changed, 21 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > index 3a63a1f75f..bba5ffd482 100644 > > > > --- a/include/efi_loader.h > > > > +++ b/include/efi_loader.h > > > > @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); > > > > #define EFI_CACHELINE_SIZE 128 > > > > #endif > > > > > > > > +/** > > > > + * efi_handle_to_udev - accessor to the DM device associated to the EFI handle > > > > + * @handle: pointer to the EFI handle > > > > + */ > > > > +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) > > > > > > This conversion will hide errors if handle is not of type efi_handle_t. > > > We should avoid the conversion and see build time errors instead. > > > Please, remove the macro. > > > > I don't think we should remove the macro itself, but only the type casting. > > > > I think it is a good practice to hide an implementation how the relationship > > between udev and efi_object is maintained *behind* accessor macros. > > > > > For every handle of type efi_handle_t you can access the field > > > handle->dev directly. > > > > > > For struct efi_disk_obj we can use disk->header.dev. > > > > This is a good example for hiding the implementation from the rest of code. > > Such a macro is pure code obfuscation. I don't think so. It will help make it easier to read the code. If I follow your logic, why did you introduce/accept guidcpy/guidcmp()? -Takahiro Akashi > I won't take such a patch. > > Best regards > > Heinrich > > > > > > > + > > > > /* Key identifying current memory map */ > > > > extern efi_uintn_t efi_memory_map_key; > > > > > > > > @@ -375,6 +381,7 @@ enum efi_object_type { > > > > * @protocols: linked list with the protocol interfaces installed on this > > > > * handle > > > > * @type: image type if the handle relates to an image > > > > + * @dev: pointer to the DM device which is associated with this EFI handle > > > > * > > > > * UEFI offers a flexible and expandable object model. The objects in the UEFI > > > > * API are devices, drivers, and loaded images. struct efi_object is our storage > > > > @@ -392,6 +399,7 @@ struct efi_object { > > > > /* The list of protocols */ > > > > struct list_head protocols; > > > > enum efi_object_type type; > > > > + struct udevice *dev; > > > > }; > > > > > > > > enum efi_image_auth_status { > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > > > index 1d700b2a6b..a8e8521e3e 100644 > > > > --- a/lib/efi_loader/efi_disk.c > > > > +++ b/lib/efi_loader/efi_disk.c > > > > @@ -46,7 +46,6 @@ struct efi_disk_obj { > > > > struct efi_device_path *dp; > > > > unsigned int part; > > > > struct efi_simple_file_system_protocol *volume; > > > > - struct udevice *dev; /* TODO: move it to efi_object */ > > > > > > ok > > > > > > > }; > > > > > > > > /** > > > > @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, > > > > return EFI_BAD_BUFFER_SIZE; > > > > > > > > if (CONFIG_IS_ENABLED(PARTITIONS) && > > > > - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > > > > + device_get_uclass_id(efi_handle_to_udev(diskobj)) == UCLASS_PARTITION) { > > > > > > device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) { > > > > > > > if (direction == EFI_DISK_READ) > > > > - n = dev_read(diskobj->dev, lba, blocks, buffer); > > > > + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, buffer); > > > > > > dev_read(diskobj->header.hdev) > > > > > > > else > > > > - n = dev_write(diskobj->dev, lba, blocks, buffer); > > > > + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, buffer); > > > > > > dev_write(diskobj->header.hdev) > > > > > > > } else { > > > > /* dev is a block device (UCLASS_BLK) */ > > > > struct blk_desc *desc; > > > > > > > > - desc = dev_get_uclass_plat(diskobj->dev); > > > > + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); > > > > > > dev_get_uclass(diskobj->header.hdev) > > > > > > > > > > if (direction == EFI_DISK_READ) > > > > n = blk_dread(desc, lba, blocks, buffer); > > > > else > > > > @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) > > > > > > > > return -1; > > > > } > > > > - disk->dev = dev; > > > > + efi_handle_to_udev(disk) = dev; > > > > if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > > > > efi_free_pool(disk->dp); > > > > efi_delete_handle(&disk->header); > > > > @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) > > > > log_err("Adding partition for %s failed\n", dev->name); > > > > return -1; > > > > } > > > > - disk->dev = dev; > > > > + efi_handle_to_udev(disk) = dev; > > > > > > disk->header.dev = dev; > > > > It's my preference, but I would suggest another accessor: > > efi_set_udev(handle, dev); > > to hide an implementation. > > > > -Takahiro Akashi > > > > > > > > > if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { > > > > efi_free_pool(disk->dp); > > > > efi_delete_handle(&disk->header); > > > > @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event *event) > > > > ret = efi_disk_create_raw(dev); > > > > if (ret) > > > > return -1; > > > > + } else { > > > > + efi_handle_t handle; > > > > + > > > > + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > > > > + return -1; > > > > + > > > > + efi_handle_to_udev(handle) = dev; > > > > > > handle->dev = dev; > > > > > > Best regards > > > > > > Heinrich > > > > > > > } > > > > > > > > device_foreach_child(child, dev) { > > > >
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3a63a1f75f..bba5ffd482 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void); #define EFI_CACHELINE_SIZE 128 #endif +/** + * efi_handle_to_udev - accessor to the DM device associated to the EFI handle + * @handle: pointer to the EFI handle + */ +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev) + /* Key identifying current memory map */ extern efi_uintn_t efi_memory_map_key; @@ -375,6 +381,7 @@ enum efi_object_type { * @protocols: linked list with the protocol interfaces installed on this * handle * @type: image type if the handle relates to an image + * @dev: pointer to the DM device which is associated with this EFI handle * * UEFI offers a flexible and expandable object model. The objects in the UEFI * API are devices, drivers, and loaded images. struct efi_object is our storage @@ -392,6 +399,7 @@ struct efi_object { /* The list of protocols */ struct list_head protocols; enum efi_object_type type; + struct udevice *dev; }; enum efi_image_auth_status { diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 1d700b2a6b..a8e8521e3e 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -46,7 +46,6 @@ struct efi_disk_obj { struct efi_device_path *dp; unsigned int part; struct efi_simple_file_system_protocol *volume; - struct udevice *dev; /* TODO: move it to efi_object */ }; /** @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, return EFI_BAD_BUFFER_SIZE; if (CONFIG_IS_ENABLED(PARTITIONS) && - device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { + device_get_uclass_id(efi_handle_to_udev(diskobj)) == UCLASS_PARTITION) { if (direction == EFI_DISK_READ) - n = dev_read(diskobj->dev, lba, blocks, buffer); + n = dev_read(efi_handle_to_udev(diskobj), lba, blocks, buffer); else - n = dev_write(diskobj->dev, lba, blocks, buffer); + n = dev_write(efi_handle_to_udev(diskobj), lba, blocks, buffer); } else { /* dev is a block device (UCLASS_BLK) */ struct blk_desc *desc; - desc = dev_get_uclass_plat(diskobj->dev); + desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj)); if (direction == EFI_DISK_READ) n = blk_dread(desc, lba, blocks, buffer); else @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev) return -1; } - disk->dev = dev; + efi_handle_to_udev(disk) = dev; if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { efi_free_pool(disk->dp); efi_delete_handle(&disk->header); @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev) log_err("Adding partition for %s failed\n", dev->name); return -1; } - disk->dev = dev; + efi_handle_to_udev(disk) = dev; if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) { efi_free_pool(disk->dp); efi_delete_handle(&disk->header); @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event *event) ret = efi_disk_create_raw(dev); if (ret) return -1; + } else { + efi_handle_t handle; + + if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) + return -1; + + efi_handle_to_udev(handle) = dev; } device_foreach_child(child, dev) {
This is a preparation patch to provide the unified method to access udevice pointer associated with the block io device. The EFI handles of both EFI block io driver implemented in lib/efi_loader/efi_disk.c and EFI block io driver implemented as EFI payload can posess the udevice pointer in the struct efi_object. We can use this udevice pointer to get the U-Boot friendly block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- Newly created in v9 include/efi_loader.h | 8 ++++++++ lib/efi_loader/efi_disk.c | 20 +++++++++++++------- 2 files changed, 21 insertions(+), 7 deletions(-)