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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Hi Simon,

On Tue, Dec 26, 2023 at 09:47:03AM +0000, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, Dec 20, 2023 at 9:17???AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > Am 20. Dezember 2023 05:46:16 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > >Hi,
> > >
> > >On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro
> > ><takahiro.akashi@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@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@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.

Then, what is the difference between "record the device and partition
number (strictly it's not accurate because we also support tftp and others.)
and "call efi_set_bootdev()"?

The latter does the translation on the fly and saves the info
in bootefi_image_path and bootefi_device_path local variables.

I believe that both are essentially the same.

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

Always thank you for your review.

-Takahiro Akashi

> 
> >
> > 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