[U-Boot] [PATCH] efi_loader: disk: Fix CONFIG_BLK breakage

[Alexander Graf]

> Am 10.08.2016 um 15:16 schrieb Simon Glass <sjg at chromium.org>:
> 
> Hi Alex,
> 
>> On 10 August 2016 at 07:02, Alexander Graf <agraf at suse.de> wrote:
>>> On 08/10/2016 02:56 PM, Simon Glass wrote:
>>> 
>>> +Tom
>>> 
>>> Hi Alex,
>>> 
>>> On 10 August 2016 at 01:47, Alexander Graf <agraf at suse.de> wrote:
>>>>> 
>>>>> On 08 Aug 2016, at 23:44, Simon Glass <sjg at chromium.org> wrote:
>>>>> 
>>>>> Hi Alexander,
>>>>> 
>>>>>> On 5 August 2016 at 06:49, Alexander Graf <agraf at suse.de> wrote:
>>>>>> 
>>>>>> When using CONFIG_BLK, there were 2 issues:
>>>>>> 
>>>>>>  1) The name we generate the device with has to match the
>>>>>>     name we set in efi_set_bootdev()
>>>>>> 
>>>>>>  2) The device we pass into our block functions was wrong,
>>>>>>     we should not rediscover it but just use the already known
>>>>>>     pointer.
>>>>>> 
>>>>>> This patch fixes both issues.
>>>>>> 
>>>>>> Signed-off-by: Alexander Graf <agraf at suse.de>
>>>>>> ---
>>>>>> cmd/bootefi.c             | 23 ++++++++++++++++++-----
>>>>>> lib/efi_loader/efi_disk.c | 18 +++++++++++-------
>>>>>> 2 files changed, 29 insertions(+), 12 deletions(-)
>>> [...]
>>> 
>>>>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>>>>> index c434c92..e00a747 100644
>>>>>> --- a/lib/efi_loader/efi_disk.c
>>>>>> +++ b/lib/efi_loader/efi_disk.c
>>>>>> @@ -31,6 +31,8 @@ struct efi_disk_obj {
>>>>>>        struct efi_device_path_file_path *dp;
>>>>>>        /* Offset into disk for simple partitions */
>>>>>>        lbaint_t offset;
>>>>>> +       /* Internal block device */
>>>>>> +       const struct blk_desc *desc;
>>>>> 
>>>>> Rather than storing this, can you store the udevice?
>>>> 
>>>> I could, but then I would diverge between the CONFIG_BLK and
>>>> non-CONFIG_BLK path again, which would turn the code into an #ifdef mess
>>>> (read: hard to maintain), because the whole device creation path relies on
>>>> struct blk_desc * today and doesn’t pass the udevice anywhere.
>>>> 
>>>> Do you feel strongly about this? To give you an idea how messy it gets,
>>>> the diff is below.
>>> 
>>> Actually I'd like to make this feature depend on CONFIG_BLK. If we add
>>> new features that don't use driver model, and then use the legacy data
>>> structures such that converting to driver model becomes harder, we'll
>>> never be done.
>>> 
>>> I did mention this at the beginning and it seems to have come to pass.
>>> 
>>> In order of preference from my side:
>>> 
>>> 1. Make EFI_LOADER depend on BLK
>> 
>> 
>> If we make EFI_LOADER depend on BLK, doesn't that break all systems that
>> need storage that isn't converted to device model today? Like the SATA
>> breakage on Xilinx systems, just at a much bigger scale?
> 
> No it just means that these platforms need to move to BLK before they
> can use the EFI loader. Given the embryonic nature of this feature,
> that seems reasonable, and the impact would be small. It will also
> encourage conversion and keep the code cleaner.

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Alex

> 
> Regards,
> Simon