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

[Simon Glass]

Hi Alex,

On 10 August 2016 at 07:41, Alexander Graf <agraf at suse.de> wrote:
>
>> On 10 Aug 2016, at 15:33, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Alex,
>>
>> On 10 August 2016 at 07:25, Alexander Graf <agraf at suse.de> wrote:
>>>
>>>
>>>> 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.
>>
>> Yes that's right. But it is mostly just a simple case of enabling the
>> option. For a few boards there might be an MMC driver that needs
>> converting, but we can approach those one by one.
>
> That approach sounds terribly wrong tbh. If it’s so simple, why not just mark a flag day where CONFIG_BLK becomes mandatory and fix all fallout? Then you’ll have much more help at your disposal than a few percent of my overloaded days ;).

We all have plenty of things to do. I can't see how we can do a flag
day. Things need to convert bit by bit as people want the new features
that it enables. We can't just turn off all the non-complying boards.

If we add new features based on legacy code, there is no incentive for
people to switch. I had the same discussion on SPI flash. it really
won't save you are lot of time (you are overestimating the impact),
and it's much better for U-Boot as a whole.

Regards,
Simon