On Wed, 2024-10-16 at 14:19 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2024-10-15 at 11:10 -0700, Adam Williamson wrote:
> >
> > 7b192ec4c rejigged the serial port detection code when no port
> > is explicitly specified. Before 7b192ec4c we did
> > grub_serial_find ("com0") in this case, which on *any* platform
> > would return a port called "com0" if one was found, otherwise it
> > fell through to the later blocks about ns8250 and ieee1275.
> > 7b192ec4c changed this so we do grub_serial_find ("auto"), and
> > added a block to handle the "auto" string which tries to detect
> > a port from the SPCR by calling grub_ns8250_spcr_init (), then
> > falls through to com0 if that doesn't work.
> >
> > However, that block was placed much later than a call with "com0"
> > would be handled, and was wrapped in two ifdefs that limit its
> > scope. This means that we would no longer default to com0 unless
> > the requirements of *both* ifdefs are met.
> >
> > To fix this, move the handling of "auto" up to happen right after
> > the exact port name match (and before the mips/x86/EMU/ARC ifdef
> > kicks in), and change the ifdef that is meant to block use of
> > grub_ns8250_spcr_init on inappropriate platforms to *only* wrap
> > the call to grub_ns8250_spcr_init, not *also* wrap the fallback
> > to com0.
>
> I am not entirely sure what the "moving up" changes in practice. The
> various top-level if () blocks in grub_serial_find() all operates on
> the name.
>
> You are basically moving the
>
> if (name is "auto")
>
> From after to before the
>
> if (name starts with "port")
>
> I don't think that in itself has any impact or changes anything other
> than being moved outside of the confines of the ifdef. So I assume
> that's what you are after.
>
> Correct me if I'm wrong but the only "issue" here is that if you are on
> a target for which:
>
> #if (defined(__i386__) || defined(__x86_64__)) &&
> !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
>
> Isn't true, then the new "default" of "auto" will fail to match an existing
> port in the list called "com0" right ?
Exactly, yeah. I moved it up to get it out of that ifdef.
> In which case a simpler patch would have been maybe to just add to
> the very top of the function insteadinstead
>
> if (grub_strcmp (name, "auto") == 0)
> {
> #if (defined(__i386__) || defined(__x86_64__)) &&
> !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
> /* Look for an SPCR if any. If not, default to com0. */
> port = grub_ns8250_spcr_init ();
> if (port != NULL)
> return port;
> #endif
> /* Legacy default */
> name = "com0";
> }
>
> Don't you think ?
Yeah, I actually thought about something along those lines later this
afternoon after I'd sent my patch - using the same FOR_SERIAL_PORTS
loop for both cases seems elegant. I agree with that idea.
--
Adam Williamson (he/him/his)
Fedora QA
Fedora Chat: @adamwill:fedora.im | Mastodon: @ad...@fosstodon.org
https://www.happyassassin.net
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
