| Message ID | 20180117191612.17108-15-xypron.glpk@gmx.de |
|---|---|
| State | Superseded, archived |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi_loader: enable EFI driver provided block device | expand |
On 17.01.18 20:16, Heinrich Schuchardt wrote: > U-Boot devices and EFI handles can be related, e.g. an > IDE disk relates to a handle with the EFI_BLOCK_IO_PROTOCOL. > Provide pointers to store these links. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > v2 > no change > --- > include/dm/device.h | 4 ++++ > include/efi_loader.h | 2 ++ > lib/efi_loader/efi_boottime.c | 1 + > 3 files changed, 7 insertions(+) > > diff --git a/include/dm/device.h b/include/dm/device.h > index 813e49f330..e5c54fe7b6 100644 > --- a/include/dm/device.h > +++ b/include/dm/device.h > @@ -11,6 +11,7 @@ > #ifndef _DM_DEVICE_H > #define _DM_DEVICE_H > > +#include <efi_loader.h> > #include <dm/ofnode.h> > #include <dm/uclass-id.h> > #include <fdtdec.h> > @@ -144,6 +145,9 @@ struct udevice { > uint32_t flags; > int req_seq; > int seq; > +#ifdef EFI_LOADER > + efi_handle_t handle; > +#endif I fail to find where you actually make use of the handle inside > #ifdef CONFIG_DEVRES > struct list_head devres_head; > #endif > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 4060348695..711c901eda 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -139,6 +139,8 @@ struct efi_object { > struct list_head protocols; > /* The object spawner can either use this for data or as identifier */ > void *handle; > + /* Device */ > + struct udevice *dev; > }; > > /** > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 5a3349ecb2..4b3b63e39a 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -362,6 +362,7 @@ efi_status_t efi_create_handle(efi_handle_t *handle) > (void **)&obj); > if (r != EFI_SUCCESS) > return r; > + obj->dev = NULL; How about we just zero initialize the whole struct? Alex > efi_add_handle(obj); > *handle = obj->handle; > return r; >
On 01/18/2018 05:09 PM, Alexander Graf wrote: > > > On 17.01.18 20:16, Heinrich Schuchardt wrote: >> U-Boot devices and EFI handles can be related, e.g. an >> IDE disk relates to a handle with the EFI_BLOCK_IO_PROTOCOL. >> Provide pointers to store these links. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> v2 >> no change >> --- >> include/dm/device.h | 4 ++++ >> include/efi_loader.h | 2 ++ >> lib/efi_loader/efi_boottime.c | 1 + >> 3 files changed, 7 insertions(+) >> >> diff --git a/include/dm/device.h b/include/dm/device.h >> index 813e49f330..e5c54fe7b6 100644 >> --- a/include/dm/device.h >> +++ b/include/dm/device.h >> @@ -11,6 +11,7 @@ >> #ifndef _DM_DEVICE_H >> #define _DM_DEVICE_H >> >> +#include <efi_loader.h> >> #include <dm/ofnode.h> >> #include <dm/uclass-id.h> >> #include <fdtdec.h> >> @@ -144,6 +145,9 @@ struct udevice { >> uint32_t flags; >> int req_seq; >> int seq; >> +#ifdef EFI_LOADER >> + efi_handle_t handle; >> +#endif > > I fail to find where you actually make use of the handle inside > >> #ifdef CONFIG_DEVRES >> struct list_head devres_head; >> #endif >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 4060348695..711c901eda 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -139,6 +139,8 @@ struct efi_object { >> struct list_head protocols; >> /* The object spawner can either use this for data or as identifier */ >> void *handle; >> + /* Device */ >> + struct udevice *dev; >> }; >> >> /** >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >> index 5a3349ecb2..4b3b63e39a 100644 >> --- a/lib/efi_loader/efi_boottime.c >> +++ b/lib/efi_loader/efi_boottime.c >> @@ -362,6 +362,7 @@ efi_status_t efi_create_handle(efi_handle_t *handle) >> (void **)&obj); >> if (r != EFI_SUCCESS) >> return r; >> + obj->dev = NULL; > > How about we just zero initialize the whole struct? All other fields are initialized in efi_add_handle(). So why invest more CPU cycles? Regards Heinrich > > Alex > >> efi_add_handle(obj); >> *handle = obj->handle; >> return r; >> >
On 18.01.18 17:18, Heinrich Schuchardt wrote: > On 01/18/2018 05:09 PM, Alexander Graf wrote: >> >> >> On 17.01.18 20:16, Heinrich Schuchardt wrote: >>> U-Boot devices and EFI handles can be related, e.g. an >>> IDE disk relates to a handle with the EFI_BLOCK_IO_PROTOCOL. >>> Provide pointers to store these links. >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> --- >>> v2 >>> no change >>> --- >>> include/dm/device.h | 4 ++++ >>> include/efi_loader.h | 2 ++ >>> lib/efi_loader/efi_boottime.c | 1 + >>> 3 files changed, 7 insertions(+) >>> >>> diff --git a/include/dm/device.h b/include/dm/device.h >>> index 813e49f330..e5c54fe7b6 100644 >>> --- a/include/dm/device.h >>> +++ b/include/dm/device.h >>> @@ -11,6 +11,7 @@ >>> #ifndef _DM_DEVICE_H >>> #define _DM_DEVICE_H >>> >>> +#include <efi_loader.h> >>> #include <dm/ofnode.h> >>> #include <dm/uclass-id.h> >>> #include <fdtdec.h> >>> @@ -144,6 +145,9 @@ struct udevice { >>> uint32_t flags; >>> int req_seq; >>> int seq; >>> +#ifdef EFI_LOADER >>> + efi_handle_t handle; >>> +#endif >> >> I fail to find where you actually make use of the handle inside Care to answer here too? :) >> >>> #ifdef CONFIG_DEVRES >>> struct list_head devres_head; >>> #endif >>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>> index 4060348695..711c901eda 100644 >>> --- a/include/efi_loader.h >>> +++ b/include/efi_loader.h >>> @@ -139,6 +139,8 @@ struct efi_object { >>> struct list_head protocols; >>> /* The object spawner can either use this for data or as identifier */ >>> void *handle; >>> + /* Device */ >>> + struct udevice *dev; >>> }; >>> >>> /** >>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >>> index 5a3349ecb2..4b3b63e39a 100644 >>> --- a/lib/efi_loader/efi_boottime.c >>> +++ b/lib/efi_loader/efi_boottime.c >>> @@ -362,6 +362,7 @@ efi_status_t efi_create_handle(efi_handle_t *handle) >>> (void **)&obj); >>> if (r != EFI_SUCCESS) >>> return r; >>> + obj->dev = NULL; >> >> How about we just zero initialize the whole struct? > > All other fields are initialized in efi_add_handle(). > So why invest more CPU cycles? I'm mostly concerned that we get into a situation where people don't fully grasp the flow of what gets initialized where and nasty bugs happen. So I guess we should either initialize obj->dev in efi_add_handle() or fully zero initialize obj, like we do for most other callers of efi_add_handle(). Alex
On 17.01.18 20:16, Heinrich Schuchardt wrote: > U-Boot devices and EFI handles can be related, e.g. an > IDE disk relates to a handle with the EFI_BLOCK_IO_PROTOCOL. > Provide pointers to store these links. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> You actually wouldn't need any of these changes I think. With a small change to the block driver, even the need for "dev" disappears. Alex diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c index 837787d563..71c752d107 100644 --- a/lib/efi_driver/efi_block_device.c +++ b/lib/efi_driver/efi_block_device.c @@ -91,19 +91,19 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, return blkcnt; } -static int efi_bl_bind_partions(efi_handle_t handle) +static int efi_bl_bind_partions(efi_object *obj, struct udevice *bdev) { struct efi_object *obj = efi_search_obj(handle); struct blk_desc *desc; const char *if_typename; - if (!obj || !obj->dev) + if (!obj || !bdev) return -ENOENT; - desc = dev_get_uclass_platdata(obj->dev); + desc = dev_get_uclass_platdata(bdev); if_typename = blk_get_if_type_name(desc->if_type); return efi_disk_create_partitions(handle, desc, if_typename, - desc->devnum, obj->dev->name); + desc->devnum, bdev->name); } /* @@ -137,11 +137,10 @@ static int efi_bl_bind(efi_handle_t handle, void *interface) return ret; EFI_PRINT("%s: block device '%s' created\n", __func__, bdev->name); bdev->platdata = interface; - obj->dev = bdev; ret = blk_prepare_device(bdev); - disks = efi_bl_bind_partions(handle); + disks = efi_bl_bind_partions(obj, bdev); EFI_PRINT("Found %d partions\n", disks); return 0;
On 01/18/2018 07:13 PM, Alexander Graf wrote: > > > On 18.01.18 17:18, Heinrich Schuchardt wrote: >> On 01/18/2018 05:09 PM, Alexander Graf wrote: >>> >>> >>> On 17.01.18 20:16, Heinrich Schuchardt wrote: >>>> U-Boot devices and EFI handles can be related, e.g. an >>>> IDE disk relates to a handle with the EFI_BLOCK_IO_PROTOCOL. >>>> Provide pointers to store these links. >>>> >>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>> --- >>>> v2 >>>> no change >>>> --- >>>> include/dm/device.h | 4 ++++ >>>> include/efi_loader.h | 2 ++ >>>> lib/efi_loader/efi_boottime.c | 1 + >>>> 3 files changed, 7 insertions(+) >>>> >>>> diff --git a/include/dm/device.h b/include/dm/device.h >>>> index 813e49f330..e5c54fe7b6 100644 >>>> --- a/include/dm/device.h >>>> +++ b/include/dm/device.h >>>> @@ -11,6 +11,7 @@ >>>> #ifndef _DM_DEVICE_H >>>> #define _DM_DEVICE_H >>>> >>>> +#include <efi_loader.h> >>>> #include <dm/ofnode.h> >>>> #include <dm/uclass-id.h> >>>> #include <fdtdec.h> >>>> @@ -144,6 +145,9 @@ struct udevice { >>>> uint32_t flags; >>>> int req_seq; >>>> int seq; >>>> +#ifdef EFI_LOADER >>>> + efi_handle_t handle; >>>> +#endif >>> >>> I fail to find where you actually make use of the handle inside > > Care to answer here too? :) The changes in include/dm/device.h are not needed. I will revert these. Maybe in future we will have to back link devices to handles but not now. Regards Heinrich > >>> >>>> #ifdef CONFIG_DEVRES >>>> struct list_head devres_head; >>>> #endif >>>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>>> index 4060348695..711c901eda 100644 >>>> --- a/include/efi_loader.h >>>> +++ b/include/efi_loader.h >>>> @@ -139,6 +139,8 @@ struct efi_object { >>>> struct list_head protocols; >>>> /* The object spawner can either use this for data or as identifier */ >>>> void *handle; >>>> + /* Device */ >>>> + struct udevice *dev; >>>> }; >>>> >>>> /** >>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >>>> index 5a3349ecb2..4b3b63e39a 100644 >>>> --- a/lib/efi_loader/efi_boottime.c >>>> +++ b/lib/efi_loader/efi_boottime.c >>>> @@ -362,6 +362,7 @@ efi_status_t efi_create_handle(efi_handle_t *handle) >>>> (void **)&obj); >>>> if (r != EFI_SUCCESS) >>>> return r; >>>> + obj->dev = NULL; >>> >>> How about we just zero initialize the whole struct? >> >> All other fields are initialized in efi_add_handle(). >> So why invest more CPU cycles? > > I'm mostly concerned that we get into a situation where people don't > fully grasp the flow of what gets initialized where and nasty bugs happen. > > So I guess we should either initialize obj->dev in efi_add_handle() or > fully zero initialize obj, like we do for most other callers of > efi_add_handle(). > > > Alex >
diff --git a/include/dm/device.h b/include/dm/device.h index 813e49f330..e5c54fe7b6 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -11,6 +11,7 @@ #ifndef _DM_DEVICE_H #define _DM_DEVICE_H +#include <efi_loader.h> #include <dm/ofnode.h> #include <dm/uclass-id.h> #include <fdtdec.h> @@ -144,6 +145,9 @@ struct udevice { uint32_t flags; int req_seq; int seq; +#ifdef EFI_LOADER + efi_handle_t handle; +#endif #ifdef CONFIG_DEVRES struct list_head devres_head; #endif diff --git a/include/efi_loader.h b/include/efi_loader.h index 4060348695..711c901eda 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -139,6 +139,8 @@ struct efi_object { struct list_head protocols; /* The object spawner can either use this for data or as identifier */ void *handle; + /* Device */ + struct udevice *dev; }; /** diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 5a3349ecb2..4b3b63e39a 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -362,6 +362,7 @@ efi_status_t efi_create_handle(efi_handle_t *handle) (void **)&obj); if (r != EFI_SUCCESS) return r; + obj->dev = NULL; efi_add_handle(obj); *handle = obj->handle; return r;
U-Boot devices and EFI handles can be related, e.g. an IDE disk relates to a handle with the EFI_BLOCK_IO_PROTOCOL. Provide pointers to store these links. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- v2 no change --- include/dm/device.h | 4 ++++ include/efi_loader.h | 2 ++ lib/efi_loader/efi_boottime.c | 1 + 3 files changed, 7 insertions(+)