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

Tom Rini trini at konsulko.com
Mon Aug 22 17:56:40 CEST 2016 

On Mon, Aug 22, 2016 at 12:22:58AM -0400, Alexander Graf wrote:
> 
> 
> > Am 19.08.2016 um 14:14 schrieb Tom Rini <trini at konsulko.com>:
> > 
> >> On Fri, Aug 12, 2016 at 08:19:42PM +0200, Alexander Graf wrote:
> >> 
> >> 
> >>> On 12.08.16 19:20, Simon Glass wrote:
> >>> Hi Alex,
> >>> 
> >>>> On 10 August 2016 at 13:01, Alexander Graf <agraf at suse.de> wrote:
> >>>> 
> >>>>> On 10 Aug 2016, at 18:25, Tom Rini <trini at konsulko.com> wrote:
> >>>>> 
> >>>>> On Wed, Aug 10, 2016 at 03:25:16PM +0200, Alexander Graf 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.
> >>>>> 
> >>>>> Seems like as good a place as any to jump in, of the boards that you
> >>>>> need EFI enabled on, what isn't converted today?
> >>>> 
> >>>> I want to make EFI the default boot path in openSUSE, which means any board that anyone out there wants to run openSUSE on would be on the list. Anything that keeps them from running EFI on a random board is a road block that I’d rather not have if I can avoid it.
> >>> 
> >>> Of course, I fully understand that. However as mentioned in this
> >>> patch, you are creating future problems.
> >> 
> >> I don't see how I am creating future problems, really. Passing a
> >> udevice* instead of the struct blk_desc* internally doesn't improve the
> >> code really at the end of the day.
> >> 
> >>> Can you address Tom's question? I take it that it boots on Raspberry
> >>> Pi (which I'd like to try actually - are there instructions
> >>> somewhere?). We can easily convert that over. Anything else?
> >> 
> >> For a list of currently available "upstream" openSUSE images, see
> >> https://build.opensuse.org/package/view_file/openSUSE:Factory:ARM/JeOS/pre_checkin.sh?expand=1
> >> 
> >> Every one of those is on the short-term list. Any other board that
> >> people want to run on is potentially on the mid-term to long-term list.
> > 
> > OK, that is a lot of boards and such.  And yes, I see both of these
> > features / projects as important to the long-term health of U-Boot.
> > 
> > So, Alex, when we've got storage converted fully to DM, you're willing
> > to do further clean-ups to make it DM-better, yes?  Thanks!
> 
> I don't understand the question. Eventually I agree with Simon that we
> should have only a single object list and type, but with the state dm
> is in, I definitely will not require any of the efi code on dm.
> 
> Having said that, I am definitely making sure things work with dm and
> as soon as a non-dm subsystem drops, I will happily integrate the efi
> onject hierarchy into the dm one for that class of objects.

OK, that's what I wanted.  To me the greatest problem (and why we've
pushed back on some other things that came in but didn't implement DM)
is non-DM code that won't have someone active to convert it later.  That
won't be the case here, so we'll address things here more later.
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160822/2f480823/attachment.sig>

More information about the U-Boot mailing list