Date: Wed, 6 Dec 2023 18:35:04 -0700 From: Warner Losh <imp@bsdimp.com> To: John Baldwin <jhb@freebsd.org> Cc: Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, "<dev-commits-src-main@freebsd.org>" <dev-commits-src-main@freebsd.org> Subject: Re: git: 773c13c686e4 - main - kldxref: skip .pkgsave files Message-ID: <CANCZdfpkEmPrTfU7bbw%2BauWuvvu=DuJf5LbepBzbZxM0_SGbGg@mail.gmail.com> In-Reply-To: <d6562b38-4770-4ae6-903d-dc00d557ea26@FreeBSD.org> References: <202302251737.31PHb2R8072300@gitrepo.freebsd.org> <e30bb1e4-11c7-4829-a7b6-21bfd590e558@FreeBSD.org> <CANCZdfp_Vz-K24%2BkBy1DZhMKoAjJ%2B-Z1m_9wB6DZ24=QTopBiA@mail.gmail.com> <a0746fe6-62b6-4a0a-99ad-d8a0b2e01d09@FreeBSD.org> <CANCZdfozE%2BB721S800eSKQ8eaUgdCC4BcyYXOf4BLEfbgL-34A@mail.gmail.com> <d6562b38-4770-4ae6-903d-dc00d557ea26@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Wed, Dec 6, 2023, 2:53 PM John Baldwin <jhb@freebsd.org> wrote: > On 12/6/23 1:41 PM, Warner Losh wrote: > > Hey John, > > > > On Wed, Dec 6, 2023 at 2:13 PM John Baldwin <jhb@freebsd.org> wrote: > > > >> On 12/6/23 1:02 PM, Warner Losh wrote: > >>> On Wed, Dec 6, 2023, 1:04 PM John Baldwin <jhb@freebsd.org> wrote: > >>> > >>>> On 2/25/23 9:37 AM, Warner Losh wrote: > >>>>> The branch main has been updated by imp: > >>>>> > >>>>> URL: > >>>> > >> > https://cgit.FreeBSD.org/src/commit/?id=773c13c686e4b6ae9dbbc150b342b82c3f47d73a > >>>>> > >>>>> commit 773c13c686e4b6ae9dbbc150b342b82c3f47d73a > >>>>> Author: Mina Galić <freebsd@igalic.co> > >>>>> AuthorDate: 2023-02-25 17:31:58 +0000 > >>>>> Commit: Warner Losh <imp@FreeBSD.org> > >>>>> CommitDate: 2023-02-25 17:35:43 +0000 > >>>>> > >>>>> kldxref: skip .pkgsave files > >>>>> > >>>>> This should help people transitioning from traditional setups > to > >>>> pkgbase > >>>>> experience a lot less friction. > >>>>> > >>>>> We do this by skipping all files containing two dots. > >>>>> > >>>>> Reviewed by: imp > >>>>> Pull Request: https://github.com/freebsd/freebsd-src/pull/661 > >>>>> Differential Revision: https://reviews.freebsd.org/D27959 > >>>> > >>>> This restriction is too broad and omits all of the modern wifi > firmware > >>>> klds from linker.hints, e.g. > >>>> > >>>> /boot/kernel/iwlwifi-3160-17.ucode.ko > >>>> /boot/kernel/iwlwifi-3168-29.ucode.ko > >>>> /boot/kernel/iwlwifi-7260-17.ucode.ko > >>>> /boot/kernel/iwlwifi-7265-17.ucode.ko > >>>> /boot/kernel/iwlwifi-7265D-29.ucode.ko > >>>> /boot/kernel/iwlwifi-8000C-36.ucode.ko > >>>> /boot/kernel/iwlwifi-8265-36.ucode.ko > >>>> /boot/kernel/iwlwifi-9000-pu-b0-jf-b0-46.ucode.ko > >>>> /boot/kernel/iwlwifi-9260-th-b0-jf-b0-46.ucode.ko > >>>> /boot/kernel/iwlwifi-Qu-b0-hr-b0-77.ucode.ko > >>>> /boot/kernel/iwlwifi-Qu-b0-jf-b0-77.ucode.ko > >>>> /boot/kernel/iwlwifi-Qu-c0-hr-b0-77.ucode.ko > >>>> /boot/kernel/iwlwifi-Qu-c0-jf-b0-77.ucode.ko > >>>> /boot/kernel/iwlwifi-QuZ-a0-hr-b0-77.ucode.ko > >>>> /boot/kernel/iwlwifi-QuZ-a0-jf-b0-77.ucode.ko > >>>> /boot/kernel/iwlwifi-cc-a0-77.ucode.ko > >>>> /boot/kernel/iwlwifi-so-a0-gf-a0-83.ucode.ko > >>>> /boot/kernel/iwlwifi-so-a0-gf-a0.pnvm.ko > >>>> /boot/kernel/iwlwifi-so-a0-gf4-a0-83.ucode.ko > >>>> /boot/kernel/iwlwifi-so-a0-gf4-a0.pnvm.ko > >>>> /boot/kernel/iwlwifi-so-a0-hr-b0-81.ucode.ko > >>>> /boot/kernel/iwlwifi-so-a0-jf-b0-77.ucode.ko > >>>> /boot/kernel/iwlwifi-ty-a0-gf-a0-83.ucode.ko > >>>> /boot/kernel/iwlwifi-ty-a0-gf-a0.pnvm.ko > >>>> /boot/kernel/rtw8723d_fw.bin.ko > >>>> /boot/kernel/rtw8821c_fw.bin.ko > >>>> /boot/kernel/rtw8822b_fw.bin.ko > >>>> /boot/kernel/rtw8822c_fw.bin.ko > >>>> /boot/kernel/rtw8822c_wow_fw.bin.ko > >>>> > >>>> all match this pattern and are skipped. > >>>> > >>>> I'm busy rewriting a bunch of kldxref to be a cross tool using libelf, > >>>> but I think here you want to probably revert this and just add pkgsave > >>>> to the list of "known bad" suffixes. > >>>> > >>> > >>> Sure. Any reason to not just require .ko? Or do we have to index the > >> kernel > >>> too? > >> > >> We do index the kernel as well, yes. However, we could probably get by > >> with "kernel" and ends in ".ko" as a valid set of files. This would > also > >> avoid bogusly warning about linker.hints not being a valid ELF file on > >> re-runs if you use -v. > >> > > > > Yea, that sounds good. I'll code it up and add you to the review. > > > > But why does it matter for these? Firmware is usually loaded by filename > > and need not be elf... or are these wrapped in elf sections... > > > > I haven't noticed it breaking my linuxkpi wifi driver that have > autoloaded > > firmware... > > Hmm, afaik firmwares are loaded by "module name" where a firmware .ko > contains > one or more of the firmware modules. We happen today to generally only > store one module in a single .ko (and with the same name), and in that case > kern_linker.c may fallback to just trying to load "foo".ko if it doesn't > find > an entry in linker.hints, but if that is why it is working that is > certainly > by happy accident. > > I only found this by comparing klxref output btw on a stale i386 VM between > the native kldxref in the VM (before this change) and my cross-arch version > of kldxref. > Ok. That all makes sense. I'll update my working tree tomorrow with the revert and the replacement. Since it "works" today, I'll push the revert and the fix at the same time unless the review takes too long. Warner -- > John Baldwin > > [-- Attachment #2 --] <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Dec 6, 2023, 2:53 PM John Baldwin <<a href="mailto:jhb@freebsd.org">jhb@freebsd.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 12/6/23 1:41 PM, Warner Losh wrote:<br> > Hey John,<br> > <br> > On Wed, Dec 6, 2023 at 2:13 PM John Baldwin <<a href="mailto:jhb@freebsd.org" target="_blank" rel="noreferrer">jhb@freebsd.org</a>> wrote:<br> > <br> >> On 12/6/23 1:02 PM, Warner Losh wrote:<br> >>> On Wed, Dec 6, 2023, 1:04 PM John Baldwin <<a href="mailto:jhb@freebsd.org" target="_blank" rel="noreferrer">jhb@freebsd.org</a>> wrote:<br> >>><br> >>>> On 2/25/23 9:37 AM, Warner Losh wrote:<br> >>>>> The branch main has been updated by imp:<br> >>>>><br> >>>>> URL:<br> >>>><br> >> <a href="https://cgit.FreeBSD.org/src/commit/?id=773c13c686e4b6ae9dbbc150b342b82c3f47d73a" rel="noreferrer noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=773c13c686e4b6ae9dbbc150b342b82c3f47d73a</a><br>; >>>>><br> >>>>> commit 773c13c686e4b6ae9dbbc150b342b82c3f47d73a<br> >>>>> Author: Mina Galić <<a href="mailto:freebsd@igalic.co" target="_blank" rel="noreferrer">freebsd@igalic.co</a>><br> >>>>> AuthorDate: 2023-02-25 17:31:58 +0000<br> >>>>> Commit: Warner Losh <imp@FreeBSD.org><br> >>>>> CommitDate: 2023-02-25 17:35:43 +0000<br> >>>>><br> >>>>> kldxref: skip .pkgsave files<br> >>>>><br> >>>>> This should help people transitioning from traditional setups to<br> >>>> pkgbase<br> >>>>> experience a lot less friction.<br> >>>>><br> >>>>> We do this by skipping all files containing two dots.<br> >>>>><br> >>>>> Reviewed by: imp<br> >>>>> Pull Request: <a href="https://github.com/freebsd/freebsd-src/pull/661" rel="noreferrer noreferrer" target="_blank">https://github.com/freebsd/freebsd-src/pull/661</a><br>; >>>>> Differential Revision: <a href="https://reviews.freebsd.org/D27959" rel="noreferrer noreferrer" target="_blank">https://reviews.freebsd.org/D27959</a><br>; >>>><br> >>>> This restriction is too broad and omits all of the modern wifi firmware<br> >>>> klds from linker.hints, e.g.<br> >>>><br> >>>> /boot/kernel/iwlwifi-3160-17.ucode.ko<br> >>>> /boot/kernel/iwlwifi-3168-29.ucode.ko<br> >>>> /boot/kernel/iwlwifi-7260-17.ucode.ko<br> >>>> /boot/kernel/iwlwifi-7265-17.ucode.ko<br> >>>> /boot/kernel/iwlwifi-7265D-29.ucode.ko<br> >>>> /boot/kernel/iwlwifi-8000C-36.ucode.ko<br> >>>> /boot/kernel/iwlwifi-8265-36.ucode.ko<br> >>>> /boot/kernel/iwlwifi-9000-pu-b0-jf-b0-46.ucode.ko<br> >>>> /boot/kernel/iwlwifi-9260-th-b0-jf-b0-46.ucode.ko<br> >>>> /boot/kernel/iwlwifi-Qu-b0-hr-b0-77.ucode.ko<br> >>>> /boot/kernel/iwlwifi-Qu-b0-jf-b0-77.ucode.ko<br> >>>> /boot/kernel/iwlwifi-Qu-c0-hr-b0-77.ucode.ko<br> >>>> /boot/kernel/iwlwifi-Qu-c0-jf-b0-77.ucode.ko<br> >>>> /boot/kernel/iwlwifi-QuZ-a0-hr-b0-77.ucode.ko<br> >>>> /boot/kernel/iwlwifi-QuZ-a0-jf-b0-77.ucode.ko<br> >>>> /boot/kernel/iwlwifi-cc-a0-77.ucode.ko<br> >>>> /boot/kernel/iwlwifi-so-a0-gf-a0-83.ucode.ko<br> >>>> /boot/kernel/iwlwifi-so-a0-gf-a0.pnvm.ko<br> >>>> /boot/kernel/iwlwifi-so-a0-gf4-a0-83.ucode.ko<br> >>>> /boot/kernel/iwlwifi-so-a0-gf4-a0.pnvm.ko<br> >>>> /boot/kernel/iwlwifi-so-a0-hr-b0-81.ucode.ko<br> >>>> /boot/kernel/iwlwifi-so-a0-jf-b0-77.ucode.ko<br> >>>> /boot/kernel/iwlwifi-ty-a0-gf-a0-83.ucode.ko<br> >>>> /boot/kernel/iwlwifi-ty-a0-gf-a0.pnvm.ko<br> >>>> /boot/kernel/rtw8723d_fw.bin.ko<br> >>>> /boot/kernel/rtw8821c_fw.bin.ko<br> >>>> /boot/kernel/rtw8822b_fw.bin.ko<br> >>>> /boot/kernel/rtw8822c_fw.bin.ko<br> >>>> /boot/kernel/rtw8822c_wow_fw.bin.ko<br> >>>><br> >>>> all match this pattern and are skipped.<br> >>>><br> >>>> I'm busy rewriting a bunch of kldxref to be a cross tool using libelf,<br> >>>> but I think here you want to probably revert this and just add pkgsave<br> >>>> to the list of "known bad" suffixes.<br> >>>><br> >>><br> >>> Sure. Any reason to not just require .ko? Or do we have to index the<br> >> kernel<br> >>> too?<br> >><br> >> We do index the kernel as well, yes. However, we could probably get by<br> >> with "kernel" and ends in ".ko" as a valid set of files. This would also<br> >> avoid bogusly warning about linker.hints not being a valid ELF file on<br> >> re-runs if you use -v.<br> >><br> > <br> > Yea, that sounds good. I'll code it up and add you to the review.<br> > <br> > But why does it matter for these? Firmware is usually loaded by filename<br> > and need not be elf... or are these wrapped in elf sections...<br> > <br> > I haven't noticed it breaking my linuxkpi wifi driver that have autoloaded<br> > firmware...<br> <br> Hmm, afaik firmwares are loaded by "module name" where a firmware .ko contains<br> one or more of the firmware modules. We happen today to generally only<br> store one module in a single .ko (and with the same name), and in that case<br> kern_linker.c may fallback to just trying to load "foo".ko if it doesn't find<br> an entry in linker.hints, but if that is why it is working that is certainly<br> by happy accident.<br> <br> I only found this by comparing klxref output btw on a stale i386 VM between<br> the native kldxref in the VM (before this change) and my cross-arch version<br> of kldxref.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Ok. That all makes sense. I'll update my working tree tomorrow with the revert and the replacement. Since it "works" today, I'll push the revert and the fix at the same time unless the review takes too long.</div><div dir="auto"><br></div><div dir="auto">Warner </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -- <br> John Baldwin<br> <br> </blockquote></div></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpkEmPrTfU7bbw%2BauWuvvu=DuJf5LbepBzbZxM0_SGbGg>