Date: Fri, 5 Feb 2021 22:09:16 +0200 From: Toomas Soome <tsoome@me.com> To: Warner Losh <imp@bsdimp.com> Cc: Toomas Soome <tsoome@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-branches@freebsd.org Subject: Re: git: 0c839497c174 - stable/13 - loader.efi: There are systems without ConOut, also use ConOutDev Message-ID: <014891C8-7B1F-4908-9495-2ED1A5FAABCF@me.com> In-Reply-To: <CANCZdfpf6KAyPCnxJnxPAg2K4POK7okkp1m3O28VvAcrK1tzag@mail.gmail.com> References: <CANCZdfqXU7Syo4wvxJ17Gt%2B0E2Kw8yOU_nrTNA%2Beu=z-ZwTKow@mail.gmail.com> <CF59D855-228A-4BC1-AC4B-0C54417EF3BF@me.com> <CANCZdfrdpMXTxJbzY6w2NE_DtwZ1eBuMm%2B1rr6d0a22R2QKaCQ@mail.gmail.com> <97F5C09F-7AE3-4763-AD32-BFEA25101CE5@me.com> <CANCZdfpf6KAyPCnxJnxPAg2K4POK7okkp1m3O28VvAcrK1tzag@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 5. Feb 2021, at 21:43, Warner Losh <imp@bsdimp.com> wrote: >=20 >=20 >=20 > On Fri, Feb 5, 2021 at 10:24 AM Toomas Soome <tsoome@me.com = <mailto:tsoome@me.com>> wrote: >=20 >=20 >> On 5. Feb 2021, at 18:44, Warner Losh <imp@bsdimp.com = <mailto:imp@bsdimp.com>> wrote: >>=20 >>=20 >>=20 >> On Thu, Feb 4, 2021 at 11:38 PM Toomas Soome <tsoome@me.com = <mailto:tsoome@me.com>> wrote: >>=20 >>=20 >>> On 5. Feb 2021, at 01:56, Warner Losh <imp@bsdimp.com = <mailto:imp@bsdimp.com>> wrote: >>>=20 >>> =EF=BB=BF >>> And why the instaMFC? Changes are supposed to cook force days before = merging... I have questions about the wisdom of this change... >>>=20 >>> Warner=20 >>>=20 >>=20 >> Reason is in PR. There is someone with the system without ConOut but = ConOutDev is set. Instead of falling back to arbitrary device (which in = this case was totally wrong choice), we can try the possible devices = list. We do not change the ConOut parsing. >>=20 >> We could have the same effect defaulting to Video. This bug should = have been discussed / reviewed before it was committed. >=20 > How is is different from defaulting to serial, it is just as bad? we = can not guess there, thats why we do have ConOutDev list. >=20 >=20 >>=20 >> If it would appear, there are systems with unusable devices listed in = ConOutDev, then we need to think how to handle such case. >>=20 >> Yes. We fall back to the arbitrary device... It's just a flag that = can be overridden. We can easily fall back to video too. >=20 > We *should not* fall back on arbitrary devices when there is source = for alternate options. And that option is from specification: >=20 > "The ConInDev, ConOutDev, and ErrOutDev variables each contain an = EFI_DEVICE_PATH_PROTOCOL descriptor that defines all the possible = default devices to use on boot. These variables are volatile, and are = set dynamically on every boot. ConIn, ConOut, and ErrOut are always = proper subsets of ConInDev, ConOutDev, and ErrOutDev.=E2=80=9D >=20 > Right. Except they aren't a proper subset in this case. Since they = aren't a proper subset, can you count on them having any meaningful = meaning? In the cases you found they do, but it's just as arbitrary. Well, we can argue if empty set is or is not subset of (any) other set. = But, we do have specification. And we should not arbitrary pick what = part we are going to follow and what not. > =20 > UEFI Spec 2.8A, Page 82. >=20 > There may or may not be Video (or Serial) device listed. If not, there = are good chance we will be in trouble because the firmware internals may = not be set up properly and we can just as well end up in hung system. >=20 > For x86, there's almost always Video. For !x86 it gets more = troublesome. There is almost always ConOut as well. Except when there is not. And in = this case, there is not. I still do not get what it is we are arguing about. Do we have case = where ConOut is not set and ConOutDev does have garbage? rgds, toomas >=20 > The kernel shouldn't hang when we give it the wrong console because if = the device isn't there, it won't cninit won't work. >> Please seek more review is the point I'd hoped to make in the private = email. This could easily have been reviewed. There was no urgent rush = that required it to go in w/o review or even discussion. >>=20 >> However, you didn't answer my question: Why the instant MFC? There's = a 3 day minimum for changes in head... And there's nothing so urgent = that requires a short-circuit. >>=20 >> Warner >=20 >=20 > Because the issues is reported on 13. well, I guess You are right = there, could have waited a bit more. >=20 > Right. Exactly my point.... >=20 > Warner > =20 > thanks, > toomas >=20 >=20 >=20 >>=20 >> =20 >> Thanks, >> Toomas >>=20 >>> On Thu, Feb 4, 2021, 2:34 PM Toomas Soome <tsoome@freebsd.org = <mailto:tsoome@freebsd.org>> wrote: >>> The branch stable/13 has been updated by tsoome: >>>=20 >>> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D0c839497c174e961fc71f7d3329d05b1= 0ec5525b = <https://cgit.freebsd.org/src/commit/?id=3D0c839497c174e961fc71f7d3329d05b= 10ec5525b> >>>=20 >>> commit 0c839497c174e961fc71f7d3329d05b10ec5525b >>> Author: Toomas Soome <tsoome@FreeBSD.org = <mailto:tsoome@FreeBSD.org>> >>> AuthorDate: 2021-02-04 20:49:02 +0000 >>> Commit: Toomas Soome <tsoome@FreeBSD.org = <mailto:tsoome@FreeBSD.org>> >>> CommitDate: 2021-02-04 21:33:15 +0000 >>>=20 >>> loader.efi: There are systems without ConOut, also use ConOutDev >>>=20 >>> Conout does contian the default output device name. >>> ConOutDev does contain all possible output device names, so we = can >>> use it as fallback, when there is no ConOut. >>>=20 >>> PR: 253253 >>>=20 >>> (cherry picked from commit = 2bd4ff2d8911009283e4e615ca4aad35a845f48b) >>> --- >>> stand/efi/loader/main.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>>=20 >>> diff --git a/stand/efi/loader/main.c b/stand/efi/loader/main.c >>> index ca41cd4a2610..32b278950745 100644 >>> --- a/stand/efi/loader/main.c >>> +++ b/stand/efi/loader/main.c >>> @@ -735,6 +735,8 @@ parse_uefi_con_out(void) >>> how =3D 0; >>> sz =3D sizeof(buf); >>> rv =3D efi_global_getenv("ConOut", buf, &sz); >>> + if (rv !=3D EFI_SUCCESS) >>> + rv =3D efi_global_getenv("ConOutDev", buf, &sz); >>> if (rv !=3D EFI_SUCCESS) { >>> /* If we don't have any ConOut default to serial */ >>> how =3D RB_SERIAL;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?014891C8-7B1F-4908-9495-2ED1A5FAABCF>