From nobody Tue Jun 14 17:00:24 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 191BF83B95E for ; Tue, 14 Jun 2022 17:00:37 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-vs1-xe32.google.com (mail-vs1-xe32.google.com [IPv6:2607:f8b0:4864:20::e32]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4LMvnw1CW4z3Kt1 for ; Tue, 14 Jun 2022 17:00:36 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-vs1-xe32.google.com with SMTP id i186so9544202vsc.9 for ; Tue, 14 Jun 2022 10:00:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fFYGpBKo2sQtFUbZGSYA6lT3bt5vY9eUQiyzdeofFOQ=; b=R7kFvu0/F3jrvqXrpldxg2ZfmY9bAa+U3kxqv3LMbVBL1cm1hCda3w+94QR+McW9Hj /9JVA0ZqWBBXuskICeWp51XrNMak6PvG0VZoeDeZB+EzPB+WhzdFURI1FrAFzsYM3grg udWFm3u6lAYqib5zynQQ5qjU257fp4kOHP9Ik6mQEn+t3v4L7EAYYIPDVh0ATg/laOYh vMPcC+9n/IhTfD+BjUWpqi1zkRR0TtN+HdzwAGTWLZ6K+pUy+eSKeSqNni+2vflDKumV eeEZqFL6xcjwoaciRKvS1dWiR305JiwKAXIbTJMGhcwHrnxGyaO8oGGH5I7mZLtsruhR Jd3Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=fFYGpBKo2sQtFUbZGSYA6lT3bt5vY9eUQiyzdeofFOQ=; b=NZ7mkt80yj2SV9DrnoDwNbwu8ka5O8THomfrePuFAcYPmXtNHY0ZquLIUqhc5tidUe 6dZ8E13DMi3Pf2ca7V4bHGFcdzBvSE+3NtQPbHrbO0zYGm55nbZ9tk1QdM+Oy/rLISkz LCHMb/3cnxi8uReMUFMtcyZ0MUngYc23FUi2/M869FqoF1Q7m1oRo4FyPz6SL/5i4yOi thDJyiLdIcjS2I9GpUPE8FsBTnYDvKYHNko5MOHo4YjyFTlw+L+O923lI+ttpavgT5/7 uoBkqyoGbL/RsW64R0eY8oJpALJxtWCD0uGmeBRWdxl81gB9Z5ZTTaroAfzWlWsh7aYi LR6Q== X-Gm-Message-State: AJIora8sO9Cmn9EwEwfv6Om3MpYinqt7BUFCL3Qpq609UpbWDRW4X/VU 7aLh0XSBDwmipJT+9XLl2YMaq+TdrOgs52qkCPttBw== X-Google-Smtp-Source: AGRyM1sUFj2BIthaz61cT1PW+AZxaOFCAMBjwn4ppSEBWIjhWn4fsUCbLkxtiD11FeU78oaOHcY/fxknGN6YeukUTPU= X-Received: by 2002:a05:6102:3bc5:b0:34c:bfaa:1d31 with SMTP id a5-20020a0561023bc500b0034cbfaa1d31mr2732664vsv.3.1655226035305; Tue, 14 Jun 2022 10:00:35 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202206121843.25CIhcLr014633@gitrepo.freebsd.org> <0e2172f8-21c1-afb1-9d2b-03ef14a4edf5@FreeBSD.org> In-Reply-To: <0e2172f8-21c1-afb1-9d2b-03ef14a4edf5@FreeBSD.org> From: Warner Losh Date: Tue, 14 Jun 2022 11:00:24 -0600 Message-ID: Subject: Re: git: 0f7b9777f8f3 - main - rtw88: split driver up into a core and pci part To: John Baldwin Cc: "Bjoern A. Zeeb" , src-committers , "" , dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="000000000000db92f905e16b57af" X-Rspamd-Queue-Id: 4LMvnw1CW4z3Kt1 X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20210112.gappssmtp.com header.s=20210112 header.b="R7kFvu0/"; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::e32) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-2.98 / 15.00]; RCVD_TLS_ALL(0.00)[]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20210112.gappssmtp.com:s=20210112]; NEURAL_HAM_MEDIUM(-0.98)[-0.977]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[dev-commits-src-all@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; RCPT_COUNT_FIVE(0.00)[5]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20210112.gappssmtp.com:+]; NEURAL_HAM_SHORT(-1.00)[-1.000]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::e32:from]; MLMMJ_DEST(0.00)[dev-commits-src-all]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; R_SPF_NA(0.00)[no SPF record]; MIME_TRACE(0.00)[0:+,1:+,2:~]; RCVD_COUNT_TWO(0.00)[2]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com] X-ThisMailContainsUnwantedMimeParts: N --000000000000db92f905e16b57af Content-Type: text/plain; charset="UTF-8" On Tue, Jun 14, 2022, 10:42 AM John Baldwin wrote: > On 6/13/22 10:37 AM, Bjoern A. Zeeb wrote: > > On Mon, 13 Jun 2022, Warner Losh wrote: > > > >> On Mon, Jun 13, 2022, 8:28 AM John Baldwin wrote: > >> > >>> On 6/12/22 11:43 AM, Bjoern A. Zeeb wrote: > >>>> The branch main has been updated by bz: > >>>> > >>>> URL: > >>> > https://cgit.FreeBSD.org/src/commit/?id=0f7b9777f8f39fbc230b3e1de2f844d9f839adea > >>>> > >>>> commit 0f7b9777f8f39fbc230b3e1de2f844d9f839adea > >>>> Author: Bjoern A. Zeeb > >>>> AuthorDate: 2022-06-12 18:35:58 +0000 > >>>> Commit: Bjoern A. Zeeb > >>>> CommitDate: 2022-06-12 18:35:58 +0000 > >>>> > >>>> rtw88: split driver up into a core and pci part > >>>> > >>>> Split the driver up into two modules (if_rtw88_pci.ko and > >>> rtw88_core.ko). > >>>> This is in preparation for the hopefully eventually upcoming USB > >>> support > >>>> using the same driver core. > >>>> > >>>> Note: this changes the module name to load to if_rtw88_pci.ko > >>> instead of > >>>> if_rtw88.ko. If using devmatch(8) everything should stay the > same > >>> as > >>>> the driver name (used for net.wlan.devices) stays rtw88. If > using > >>>> kld_list in rc.conf or loader.conf you will need to adjust the > name. > >>>> Update man page for this. > >>>> > >>>> MFC after: 3 days > >>> > >>> This sort of split in a .ko is kind of rare for drivers in the tree > that > >>> support > >>> multiple bus attachments. Usually we just lump all the attachments > into > >>> the same > >>> .ko. It's true that with the death of ISA, etc. we no longer have as > many > >>> drivers > >>> with multiple bus attachments, but the norm has been to include them > all > >>> in a > >>> single .ko. Is there a reason you can't follow the normal practice > here? > > > > I am not oppsed to one big blob but I wonder if it is "normal practise" > > these days? > > > > I honestly didn't think much beyond PCI is in PCI and USB load where I > > plug USB in but I couldn't think of many times having both and more > > code exposure than needed. > > > > > > So you made me go and think a bit about it and look into it: > > > > rtwn(4) is split up as "predecessor"; probably also influenced my > > above thinking. Upstream has this one written in a way that it can > > be split up natively between buses. In theory I this could be split > > up into even per-chipset mostly apart from "core" and then one could > > even start bundling chipset+fw together as an entity but that didn't > > seem like "common practise". > > > > I think the main argument is that this isn't just a few lines of bus > > attachment but larger parts of the driver (PCI vs. USB to come) given the > > PCI per-chipset files contain tables etc. and is way bigger than the > > actual common code (and USB will be fairly small I believe). > > > > -r-xr-xr-x 1 root wheel 308536 Jun 13 13:33 /boot/kernel/rtw88_core.ko > > -r-xr-xr-x 1 root wheel 998712 Jun 13 13:33 > /boot/kernel/if_rtw88_pci.ko > > > > And I don't neccessarily want to pull in two bus dependencies into the > > same driver even if that seems the historical way of thinking; "MINIMAL" > > is still a goal we once set out for and a way of thinking and autoloading > > does sort things out these days without users having to fiddle anything > > in the default case. > > > > Why do I need to load 1M file for PCI on a machine w/o PCI? > > Even many SoC boards have PCI, and anything approaching desktop class > will have PCI, so lack of PCI is quite specious. That said, historically > per-bus attachment code was indeed much smaller. OTOH, you can also > selectively include files in the .ko at build-time, e.g. based on > whether or not the base kernel included 'device pci' by checking for > DEV_PCI in KERN_OPTS, something like: > > .if ${KERN_OPTS:MDEV_PCI} > SRCS+= if_rtw88_pci.c > .endif > > This more closely matches what happens in the kernel where you would have > the sys/conf/files line be 'if_rtw88_pci.c optional rtw88 pci' > Yes, but is it worth the hassle and complications? In the past the savings hasn't been worth the extra effort. I'd really rather not start growing these things wholesale. We've always balanced ease of use over minimal loading size, mostly because to do it generally requires many things know about things like if_foo.ko, which this breaks. > Why do we have separate parts for ACPI vs. FDT? > > Separate modules just for foo_acpi.c and foo_fdt.c? Those are probably > bugs. > We don't have these as modules in general. > Why don't we have NTB support for AMD and Intel and ... together in > > one .ko? Why is mac-* not one module? Why are geom part > > implementations not one, or why are congestion control protocols, or > > ipfw modules separate? > > Why do I not get Panasonic, Fujitsu, Toshiba and HP ACPI modules on my > > Thinkpad (even same bus attachments)? > > This is not the same as these are differently named drivers in most > parts with different user interfaces. However, if you are going to have > the > 'foo0' interface, we assume that it loaded by 'if_foo.ko'. ifconfig(8) > even has this knowledge baked in. > Yea, I wasn't sure how to respond to what seemed like a non sequitur here, but I like your reply... It points out a large reason we did this: ifconfig ed0 loads if_ed automatically and if we had if_ed_isa.ko, if_ed_eisa.ho, etc then this would break. > Or why do we not put all firmware blobs for the same driver in one > > file (given firmware 9 can)? Because there is no point in loading > > 12 firmware images into memory when 1 suffices? > > Firmware is not user facing in that it doesn't show up in the UI. > > > Isn't it actually more common to put things apart these days for > > various reasons? > > Not for user-facing things. Note that urtwn and rtwn use different > driver names. I'm not sure that was really the best decision and > it is probably worth revisiting it in this case if it's really the > same hardware just with different bus attachments. > Agreed. It silently breaks autoloading as I mentioned. > I'd tend to argue that time has moved on and making things self-contained > > smaller and simpler is a good thing in a too complex world. > > So the prior history is that I think ath(4) did this once and it was > a POLA nightmare due to things like ifconfig(8), etc. > Yes. And for ath(8) i thought we had two .kos: one for mips where size mattered and one for x86 that was all the options... Warner -- > John Baldwin > --000000000000db92f905e16b57af Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, Jun 14, 2022, 10:42 AM John Baldwin <jhb@freebsd.org> wrote:
On 6/13/22 10:37 AM, Bjoern A. Zeeb wrote:
> On Mon, 13 Jun 2022, Warner Losh wrote:
>
>> On Mon, Jun 13, 2022, 8:28 AM John Baldwin <jhb@freebsd.org>= ; wrote:
>>
>>> On 6/12/22 11:43 AM, Bjoern A. Zeeb wrote:
>>>> The branch main has been updated by bz:
>>>>
>>>> URL:
>>> https://cgit.FreeBSD.org/src/commit/?id=3D0f7b9777f8f39fbc230b3e1de2f= 844d9f839adea
>>>>
>>>> commit 0f7b9777f8f39fbc230b3e1de2f844d9f839adea
>>>> Author:=C2=A0 =C2=A0 =C2=A0Bjoern A. Zeeb <bz@FreeBSD.o= rg>
>>>> AuthorDate: 2022-06-12 18:35:58 +0000
>>>> Commit:=C2=A0 =C2=A0 =C2=A0Bjoern A. Zeeb <bz@FreeBSD.o= rg>
>>>> CommitDate: 2022-06-12 18:35:58 +0000
>>>>
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0rtw88: split driver up into a co= re and pci part
>>>>
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0Split the driver up into two mod= ules (if_rtw88_pci.ko and
>>> rtw88_core.ko).
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0This is in preparation for the h= opefully eventually upcoming USB
>>> support
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0using the same driver core.
>>>>
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0Note: this changes the module na= me to load to if_rtw88_pci.ko
>>> instead of
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0if_rtw88.ko.=C2=A0 If using devm= atch(8) everything should stay the same
>>> as
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0the driver name (used for net.wl= an.devices) stays rtw88.=C2=A0 If using
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0kld_list in rc.conf or loader.co= nf you will need to adjust the name.
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0Update man page for this.
>>>>
>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0MFC after:=C2=A0 =C2=A0 =C2=A0 3= days
>>>
>>> This sort of split in a .ko is kind of rare for drivers in the= tree that
>>> support
>>> multiple bus attachments.=C2=A0 Usually we just lump all the a= ttachments into
>>> the same
>>> .ko.=C2=A0 It's true that with the death of ISA, etc. we n= o longer have as many
>>> drivers
>>> with multiple bus attachments, but the norm has been to includ= e them all
>>> in a
>>> single .ko.=C2=A0 Is there a reason you can't follow the n= ormal practice here?
>
> I am not oppsed to one big blob but I wonder if it is "normal pra= ctise"
> these days?
>
> I honestly didn't think much beyond PCI is in PCI and USB load whe= re I
> plug USB in but I couldn't think of many times having both and mor= e
> code exposure than needed.
>
>
> So you made me go and think a bit about it and look into it:
>
> rtwn(4) is split up as "predecessor";=C2=A0 probably also in= fluenced my
> above thinking.=C2=A0 =C2=A0Upstream has this one written in a way tha= t it can
> be split up natively between buses.=C2=A0 In theory I this could be sp= lit
> up into even per-chipset mostly apart from "core" and then o= ne could
> even start bundling chipset+fw together as an entity but that didn'= ;t
> seem like "common practise".
>
> I think the main argument is that this isn't just a few lines of b= us
> attachment but larger parts of the driver (PCI vs. USB to come) given = the
> PCI per-chipset files contain tables etc. and is way bigger than the > actual common code (and USB will be fairly small I believe).
>
> -r-xr-xr-x=C2=A0 1 root=C2=A0 wheel=C2=A0 308536 Jun 13 13:33 /boot/ke= rnel/rtw88_core.ko
> -r-xr-xr-x=C2=A0 1 root=C2=A0 wheel=C2=A0 998712 Jun 13 13:33 /boot/ke= rnel/if_rtw88_pci.ko
>
> And I don't neccessarily want to pull in two bus dependencies into= the
> same driver even if that seems the historical way of thinking; "M= INIMAL"
> is still a goal we once set out for and a way of thinking and autoload= ing
> does sort things out these days without users having to fiddle anythin= g
> in the default case.
>
> Why do I need to load 1M file for PCI on a machine w/o PCI?

Even many SoC boards have PCI, and anything approaching desktop class
will have PCI, so lack of PCI is quite specious.=C2=A0 That said, historica= lly
per-bus attachment code was indeed much smaller.=C2=A0 OTOH, you can also selectively include files in the .ko at build-time, e.g. based on
whether or not the base kernel included 'device pci' by checking fo= r
DEV_PCI in KERN_OPTS, something like:

.if ${KERN_OPTS:MDEV_PCI}
SRCS+=3D=C2=A0 if_rtw88_pci.c
.endif

This more closely matches what happens in the kernel where you would have the sys/conf/files line be 'if_rtw88_pci.c=C2=A0 =C2=A0optional rtw88 p= ci'

Yes, but is it worth the hassle and complications? In the past the s= avings hasn't been worth the extra effort. I'd really rather not st= art growing these things wholesale. We've always balanced ease of use o= ver minimal loading size, mostly because to do it generally requires many t= hings know about things like if_foo.ko, which this breaks.

> Why do we have separate parts for ACPI vs. FDT?

Separate modules just for foo_acpi.c and foo_fdt.c?=C2=A0 Those are probabl= y bugs.

We don't have these as modules in general.

> Why don't we have NTB support for AMD and Intel and ... together i= n
> one .ko?=C2=A0 Why is mac-* not one module?=C2=A0 Why are geom part > implementations not one, or why are congestion control protocols, or > ipfw modules separate?
> Why do I not get Panasonic, Fujitsu, Toshiba and HP ACPI modules on my=
> Thinkpad (even same bus attachments)?

This is not the same as these are differently named drivers in most
parts with different user interfaces.=C2=A0 However, if you are going to ha= ve the
'foo0' interface, we assume that it loaded by 'if_foo.ko'.= =C2=A0 ifconfig(8)
even has this knowledge baked in.

Yea, I wasn't sure how to respond to w= hat seemed like a non sequitur here, but I like your reply...

It points out a large reason we did t= his: ifconfig ed0 loads if_ed automatically and if we had if_ed_isa.ko, if_= ed_eisa.ho, etc then this would break.

> Or why do we not put all firmware blobs for the same driver in one
> file (given firmware 9 can)?=C2=A0 Because there is no point in loadin= g
> 12 firmware images into memory when 1 suffices?

Firmware is not user facing in that it doesn't show up in the UI.

> Isn't it actually more common to put things apart these days for > various reasons?

Not for user-facing things.=C2=A0 Note that urtwn and rtwn use different driver names.=C2=A0 I'm not sure that was really the best decision and<= br> it is probably worth revisiting it in this case if it's really the
same hardware just with different bus attachments.

Agreed. It silently break= s autoloading as I mentioned.=C2=A0

> I'd tend to argue that time has moved on and making things self-co= ntained
> smaller and simpler is a good thing in a too complex world.

So the prior history is that I think ath(4) did this once and it was
a POLA nightmare due to things like ifconfig(8), etc.

Yes. And for ath(8) i = thought we had two .kos: one for mips where size mattered and one for x86 t= hat was all the options...

Warner=C2=A0

--
John Baldwin
--000000000000db92f905e16b57af--