Hi Shantur, On Sun, 19 Nov 2023 at 10:01, Shantur Rathore <i...@shantur.com> wrote: > > Hi Simon, > > On Sun, Nov 19, 2023 at 12:44 AM Simon Glass <s...@chromium.org> wrote: > > > > Hi Shantur, > > > > On Sat, 18 Nov 2023 at 14:17, Shantur Rathore <i...@shantur.com> wrote: > > > > > > Hi Simon, > > > > > > On Sat, Nov 18, 2023 at 5:11 PM Simon Glass <s...@chromium.org> wrote: > > > > > > > > +Ilias too as this involves a design decision > > > > > > > > Hi Shantur, > > > > > > > > On Fri, 17 Nov 2023 at 14:22, Shantur Rathore <i...@shantur.com> wrote: > > > > > > > > > > efi_set_bootdev is already called as part of tftp while doing > > > > > dhcp_run() > > > > > > > > Is that in tftp_complete() ? > > > > > > > > > Doing this again crashes U-boot and we don't need to call again. > > > > > > > > U-Boot > > > > > > > > The problem is that we might scan for 4 bootflows, then later decide > > > > which one to boot. So we cannot know which one it will be until that > > > > point. > > > > > > > > The hack is needed so that it actually tells EFI about the right one. > > > > > > > > The addition of EFI hacks when reading files (i.e. the code in > > > > tftp_complete()) is a real problem, unfortunately. I understand that > > > > it was a convenient hack, but with standard boot we really don't want > > > > it to do that. We end up calling it multiple times for bootflows that > > > > won't be used. > > > > > > > > So I believe the fix is to be able to disable those calls, leaving it > > > > to bootstd. > > > > > > > > I'm not sure of the best way to do that. Perhaps we should have a > > > > function like bootstd_scanning() which returns a bool indicating that > > > > bootstd is scanning? If that is true then no efi_set_bootdev() calls > > > > are made? We could have a flag in 'struct bootstd_priv' which is set > > > > before bootflow_check() is called and cleared afterwards. > > > > > > > > As to how this should be done with standard boot, we should create a > > > > function like efi_start_app() and pass it the information we need. > > > > That function should do the equivalent of the 'bootefi' command, > > > > except programmatically, with no prior state hanging around. > > > > > > > > As to when we might be able to do that, we need more refactoring of > > > > the bootm code. There is a start on this [1] but it likely needs 2-3 > > > > more series before it might be possible. > > > > > > > > So for now, I think bootstd_scanning() is the best approach. If you > > > > are not sure about that, I can send a patch. > > > > > > > > > > My apologies, the fix I submitted is incorrect. > > > After reading your explanation, I started thinking on why dhcp can > > > call this multiple times but bootmethod can't. > > > > > > There it was clear that the issue bflow->fname is null when we call > > > this function. > > > The correct fix should be below > > > > > > + /* bootfile should be setup by dhcp by now*/ > > > + bootfile_name = env_get("bootfile"); > > > + if (!bootfile_name) > > > + return log_msg_ret("bootfile_name", ret); > > > + bflow->fname = strdup(bootfile_name); > > > + > > > /* do the hideous EFI hack */ > > > efi_set_bootdev("Net", "", bflow->fname, map_sysmem(addr, 0), > > > bflow->size); > > > > > > I have a patch ready for this which I will send but I have some > > > queries regarding that. > > > As I sent this a series of patches out of which 2 were reviewed, > > > should I send a new version for the whole series even though 2 are not > > > going to be changed? > > > > Yes you normally send the whole series but include any review/tested > > tags that you received for each patch ('patman status' can help). > > > > patman... that's the missing piece in the puzzle. I was trying to find > out how everyone is managing their commits. > > Thanks for pointing it out to me.
:-) If you have ideas to improve it, please send patches. - Simon