[PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[Simon Glass]

Hi Heinrich,

On Wed, Dec 20, 2023 at 9:17 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> Am 20. Dezember 2023 05:46:16 MEZ schrieb Simon Glass <sjg at chromium.org>:
> >Hi,
> >
> >On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro
> ><takahiro.akashi at linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote:
> >> > Hi AKASHI,
> >> >
> >> > On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> >> > <takahiro.akashi at linaro.org> wrote:
> >> > >
> >> > > Now it is clear that the feature actually depends on efi interfaces,
> >> > > not "bootefi" command. efi_set_bootdev() will automatically be nullified
> >> > > if necessary efi component is disabled.
> >> > >
> >> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> >> > > ---
> >> > >  net/tftp.c | 10 ++++------
> >> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> >> > >
> >> >
> >> > I have the same comment here as the 'fs' patch.
> >> >
> >> > > diff --git a/net/tftp.c b/net/tftp.c
> >> > > index 88e71e67de35..2e335413492b 100644
> >> > > --- a/net/tftp.c
> >> > > +++ b/net/tftp.c
> >> > > @@ -302,12 +302,10 @@ static void tftp_complete(void)
> >> > >                         time_start * 1000, "/s");
> >> > >         }
> >> > >         puts("\ndone\n");
> >> > > -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
> >> >
> >> > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER
> >> > is not enabled?
> >>
> >> The trick is in efi_loader.h.
> >> If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined,
> >> this function gets voided.  See patch#1 in this version.
> >>
> >> I took this approach in order not to make users much worried about
> >> what config be used as they are not familiar with UEFI implementation.
> >
> >OK, but we really need to delete this function, so what is the plan
> >for that? The info that EFI needs should be passed to the bootefi()
> >function, not set in a global.
>
> Hello Simon,
>
> Bootstd is not the only way to boot. Please, do not forget the shell.
>
> The user loads a file with tftpboot. At some later moment the user executes bootefi.
>
> We need a place where we store the device from which the image was loaded.

Yes, agreed. See my other reply on that.

>
> In future we might have a register of loaded files. But that is beyond the scope of this patch series.

I believe we could just record the device and partition number (which
is itself a device these days, I suppose). Then for EFI can do the
translation at the start of the bootm cmd if not using bootstd.

Reviewed-by: Simon Glass <sjg at chromium.org>

>
> Best regards
>
> Heinrich
>
> >
> >
> >>
> >> -Takahiro Akashi
> >>
> >> > > -               if (!tftp_put_active)
> >> > > -                       efi_set_bootdev("Net", "", tftp_filename,
> >> > > -                                       map_sysmem(tftp_load_addr, 0),
> >> > > -                                       net_boot_file_size);
> >> > > -       }
> >> > > +       if (!tftp_put_active)
> >> > > +               efi_set_bootdev("Net", "", tftp_filename,
> >> > > +                               map_sysmem(tftp_load_addr, 0),
> >> > > +                               net_boot_file_size);
> >> > >         net_set_state(NETLOOP_SUCCESS);
> >> > >  }
> >> > >
> >> > > --
> >> > > 2.34.1
> >
> >Regards,

Regards,
Simon