From nobody Fri Jan 10 14:05:12 2025 X-Original-To: dev-commits-src-main@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 4YV3MV4mLGz5kNrk for ; Fri, 10 Jan 2025 14:05:26 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-pj1-x1032.google.com (mail-pj1-x1032.google.com [IPv6:2607:f8b0:4864:20::1032]) (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 "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4YV3MV2kt2z4nWY for ; Fri, 10 Jan 2025 14:05:26 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-pj1-x1032.google.com with SMTP id 98e67ed59e1d1-2f4409fc8fdso3131318a91.1 for ; Fri, 10 Jan 2025 06:05:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20230601.gappssmtp.com; s=20230601; t=1736517925; x=1737122725; darn=freebsd.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=pdviZZG4ldewhivAz2NpV5j7amTAPOsLvuGcQi+bfSE=; b=pVbeO+zEqEyzX7GJUlzGbJ0ZvrCQOSGS1fgKzB2Rqy8hWygBW/fz3q8sm+/DcuvUs8 YUbih+ghIox+YFP3lPXUvZEpq1keuN7APojCuV2ymRe1uHgIyX6V1yt3DUKfwDxpQhu5 ic+gy0hUfEFyGUn/noZDWzjadlN9NJWDSa0TAigYbs0bu9R2CwOK09OLShb5dc20qJPB ISQNVoV35XXMuupFUeEFjJrU90pvPCnjtvJxLzCvStn4HaVt6C4FW3JFih7BF4Dr9zsN iPoIfqWIWAvkx7KbkgWsI4uL0NhuDrJk/dfVc2IZlEAULajbZSG5XpxR4FCXp1HXOHc3 YUHw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736517925; x=1737122725; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=pdviZZG4ldewhivAz2NpV5j7amTAPOsLvuGcQi+bfSE=; b=E8faKUAlEB4hFGexwwPScheTZYKzWk9SNqzhwulydAPIgDaInuZS+PDcPaoYT9dRuE 47vvRWGAGCtySxWB1dJFvDPXKuq/MZsh4MALtP6+0QvV3K+loDkBk2n8BqYYZd3UxJGL pV05XMpWRoQPvZuP4K5NMbAuzULjguVbjJX0s4sT2qw087VXIBAIO7i6wfrGhYTI8pFH rdFxhyMH6emFt6Z/XHH3WXY4dMEk6IOpoWGwpdvzgtamniscBbRAxtRTztdr07Kv3R+5 +WhUHe6hf2ZGXhDw5p4uyg9nB58bk9Fe7ryivj14+B6foHzVpPKUJcKOC8PIaay6UMT7 Lgqw== X-Forwarded-Encrypted: i=1; AJvYcCVp0MSvcmCuNo72FMuEupKVdffqT6UpqENvGBk9XwUltRARoHtACY7moWHhJjmPzfGoaHS7bIWBdbnXOk/L5p5RkNE7Sg==@freebsd.org X-Gm-Message-State: AOJu0Yx4hIlTnJ9uMw/wZ1SlWprs8Foop18vIe6GzJFcQp3SSOG09N/p Y6JCaRPbVWotUarA5KfjdxOLwCMW4TxxiuLMiVABLIIPoh5Uv6wbFYmcObg6diybgKUmJv4bTvV qGXa974ezkYMhinEsl42oM8mqSn7XOKl65eZY2Q== X-Gm-Gg: ASbGnctPj6lZ3rnN3CclfP2HA3ZNCR6is14t6+KAQmmPVehhanrnJR2YP7UAGkLatER vkgH0rKSsHdPWX5aaT6+QmIaSoz9yvKX8fv+OXw== X-Google-Smtp-Source: AGHT+IHuwlNdx3WMO0zVNI5bkJgQVQgTbHwMaM7qPXM2J7ZkGY/YdjTOwebl2tot4s+Rs9pngGzufffoMo4uXMTPb1A= X-Received: by 2002:a17:90b:2d0d:b0:2f2:e905:d5ff with SMTP id 98e67ed59e1d1-2f55833cee6mr9292547a91.6.1736517924622; Fri, 10 Jan 2025 06:05:24 -0800 (PST) List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 References: <202501092020.509KKt1U058876@gitrepo.freebsd.org> <9acd1878-2ee3-47c8-aab9-29d5be200081@FreeBSD.org> In-Reply-To: <9acd1878-2ee3-47c8-aab9-29d5be200081@FreeBSD.org> From: Warner Losh Date: Fri, 10 Jan 2025 07:05:12 -0700 X-Gm-Features: AbW1kvb64MA7KubMMTzIw1lFEjmDWMvoyEnhMOXR7A4Bu7wmJkAbe1f2-bekD1s Message-ID: Subject: Re: git: ccabc7c2e556 - main - DEVICE_IDENTIFY.9: Modernize description and use cases To: John Baldwin Cc: Konstantin Belousov , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="0000000000000b282d062b5a967c" X-Rspamd-Queue-Id: 4YV3MV2kt2z4nWY X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] --0000000000000b282d062b5a967c Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Jan 10, 2025 at 5:45=E2=80=AFAM John Baldwin wrot= e: > On 1/9/25 18:01, Konstantin Belousov wrote: > > On Thu, Jan 09, 2025 at 08:20:55PM +0000, John Baldwin wrote: > >> The branch main has been updated by jhb: > >> > >> URL: > https://cgit.FreeBSD.org/src/commit/?id=3Dccabc7c2e556ac0b14da9b682b706cc= af251c0fe > >> > >> commit ccabc7c2e556ac0b14da9b682b706ccaf251c0fe > >> Author: John Baldwin > >> AuthorDate: 2025-01-09 20:20:16 +0000 > >> Commit: John Baldwin > >> CommitDate: 2025-01-09 20:20:16 +0000 > >> > >> DEVICE_IDENTIFY.9: Modernize description and use cases > >> > >> Mention adding devices based on firmware tables and software-only > >> pseudo-devices as use cases for identify methods as those are mor= e > >> common than reading random I/O ports to identify a legacy ISA > device. > >> > >> Describe how device_find_chid can be used to avoid duplicates. > While > >> here, explicitly note that devices added in identify methods > typically > >> use a fixed device name. > >> > >> Trim the cross-references a bit. > >> > >> Reviewed by: ziaee, imp > >> Differential Revision: https://reviews.freebsd.org/D48367 > >> --- > >> share/man/man9/DEVICE_IDENTIFY.9 | 52 > +++++++++++++++++++--------------------- > >> 1 file changed, 25 insertions(+), 27 deletions(-) > >> > >> diff --git a/share/man/man9/DEVICE_IDENTIFY.9 > b/share/man/man9/DEVICE_IDENTIFY.9 > >> index d75c1a91ce4a..b10d94143050 100644 > >> --- a/share/man/man9/DEVICE_IDENTIFY.9 > >> +++ b/share/man/man9/DEVICE_IDENTIFY.9 > >> @@ -26,44 +26,46 @@ > >> .\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF TH= E > USE OF > >> .\" THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE= . > >> .\" > >> -.Dd January 15, 2017 > >> +.Dd January 9, 2025 > >> .Dt DEVICE_IDENTIFY 9 > >> .Os > >> .Sh NAME > >> .Nm DEVICE_IDENTIFY > >> -.Nd identify a device, register it > >> +.Nd identify new child devices and register them > >> .Sh SYNOPSIS > >> .In sys/param.h > >> .In sys/bus.h > >> .Ft void > >> .Fn DEVICE_IDENTIFY "driver_t *driver" "device_t parent" > > So what is the 'parent' for driver which creates devices based on the > > firmware tables? > > Hmmm, I could maybe try to clarify this further. In new-bus, drivers are > associated with a parent bus devclass. All of the drivers associated wit= h > a given parent bus are then eligible for use with children of any bus > devices for that bus. Thus, for example: > > DRIVER_MODULE(foo, bar, ....) > > Associates the "bar" driver with the bus "foo". For any fooX bus devices= , > the "bar" driver can attach to children of fooX. > > Most device_if.m methods operate on "child" devices, so device_probe, > device_attach, device_detach, etc. all operate on a given barX device > that is a child of a fooX. > > device_identify is different. Instead, each fooX bus device can call > bus_identify_children (formerly the somewhat misnamed bus_generic_probe) > during the fooX device_attach routine. bus_identify_children looks for > the devclass of foo and then walks all of the eligible device drivers > for potential children of foo invoking this method with fooX as the > parent. The idea being that device_identify will create "barX" children > of "fooX" explicitly using BUS_ADD_CHILD. > > In terms of which parent device, it's really about where a given "barX" > device should live. For system-wide "top-level" devices that aren't > behind some other bridge on an ACPI system, the pattern we use is to > hang those devices as children of acpi0, so you end up with > > DRIVER_MODULE(acpi, bar, ...) > > and the parent device at the time of DEVICE_IDENTIFY is acpi0. However, > we also use identify in some other places. nexus0 children all tend to > either be explicitly added in nexus_attach() or added via identify > routines that use nexus as the parent. legacy0 on x86 also uses identify > routines to add child devices like pcibX. > These are likely OK. It's arguably possible to model this as devices known in other places, but it gets super awkward and you'd have to do hand-stands that are worse in other ways. > Another case is the ipmi(4) device. Legacy ISA IPMI devices are describe= d > by an entry in the SMBIOS table. The ipmi(4) driver uses an identify > routine to add an ipmi0 device as a child of isa0 (since it has I/O ports > like a typical ISA device), but the identify routine is table-driven sinc= e > it depends on parsing the smbios table. > This is bogus, imho, but not worth fixing. We should have a isasmbb device that parses the child, creates a isasmb bus and then adds the children it finds from parsing the smbtable and moves on. It would be much cleaner. There's several other devices that kinda live here, but none worth supporting these days, so a rewrite is a waste of time. > cpufreqX is another odd case, and I'm not quite convinced it is correct. > Today we enumerate cpuX devices hung off of some nexus-like device (on > x86 cpuX are children of either acpi0 or legacy0). Each cpufreqX > driver then uses identify routines to add named children (p4tccX, estX, > hwpstateX, etc.). Those identify routines all have "cpu" as the parent > bus so that cpuX is the parent device (and they are called for each > instance of a cpuX device). For cpufreq I feel like we actually want > something a bit different on x86 at least. I think we want to create > an explicit cpufreqX device in cpu_attach, and that the various cpufreq > drivers that manage frequency should all be "cpufreq" drivers that bid > to attach to that device node instead of creating duplicate nodes that > try to duplicate work. Today the various identify routines try to > check for each other instead which is fragile. It may be that we'd > need/want two device_t nodes, one for P-states and one for throttling, > though it might be that we only want the throttling for certain P-state > drivers or some such. > This is definitely wrong in a big way. I have about 80-90% of the conversio= n to be proper children of cpu nodes, as appropriate. Warner > For your IOMMU case, you can use an ACPI table "anywhere" in the device > tree to enumerate device nodes if necessary (though PCI buses don't > currently call bus_identify_children today and don't have a BUS_ADD_CHILD > method), but if you want to be "ready" before other generic children like > PCI bridges are attached, you probably need to be a child of acpi0. > > -- > John Baldwin > > --0000000000000b282d062b5a967c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Fri, Jan 10,= 2025 at 5:45=E2=80=AFAM John Baldwin <jhb@freebsd.org> wrote:
On 1/9/25 18:01, Konstantin Belousov wrote:
> On Thu, Jan 09, 2025 at 08:20:55PM +0000, John Baldwin wrote:
>> The branch main has been updated by jhb:
>>
>> URL: http= s://cgit.FreeBSD.org/src/commit/?id=3Dccabc7c2e556ac0b14da9b682b706ccaf251c= 0fe
>>
>> commit ccabc7c2e556ac0b14da9b682b706ccaf251c0fe
>> Author:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.org> >> AuthorDate: 2025-01-09 20:20:16 +0000
>> Commit:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.org> >> CommitDate: 2025-01-09 20:20:16 +0000
>>
>>=C2=A0 =C2=A0 =C2=A0 DEVICE_IDENTIFY.9: Modernize description and u= se cases
>>=C2=A0 =C2=A0 =C2=A0
>>=C2=A0 =C2=A0 =C2=A0 Mention adding devices based on firmware table= s and software-only
>>=C2=A0 =C2=A0 =C2=A0 pseudo-devices as use cases for identify metho= ds as those are more
>>=C2=A0 =C2=A0 =C2=A0 common than reading random I/O ports to identi= fy a legacy ISA device.
>>=C2=A0 =C2=A0 =C2=A0
>>=C2=A0 =C2=A0 =C2=A0 Describe how device_find_chid can be used to a= void duplicates.=C2=A0 While
>>=C2=A0 =C2=A0 =C2=A0 here, explicitly note that devices added in id= entify methods typically
>>=C2=A0 =C2=A0 =C2=A0 use a fixed device name.
>>=C2=A0 =C2=A0 =C2=A0
>>=C2=A0 =C2=A0 =C2=A0 Trim the cross-references a bit.
>>=C2=A0 =C2=A0 =C2=A0
>>=C2=A0 =C2=A0 =C2=A0 Reviewed by:=C2=A0 =C2=A0 ziaee, imp
>>=C2=A0 =C2=A0 =C2=A0 Differential Revision:=C2=A0 https://= reviews.freebsd.org/D48367
>> ---
>>=C2=A0 =C2=A0share/man/man9/DEVICE_IDENTIFY.9 | 52 ++++++++++++++++= +++---------------------
>>=C2=A0 =C2=A01 file changed, 25 insertions(+), 27 deletions(-)
>>
>> diff --git a/share/man/man9/DEVICE_IDENTIFY.9 b/share/man/man9/DEV= ICE_IDENTIFY.9
>> index d75c1a91ce4a..b10d94143050 100644
>> --- a/share/man/man9/DEVICE_IDENTIFY.9
>> +++ b/share/man/man9/DEVICE_IDENTIFY.9
>> @@ -26,44 +26,46 @@
>>=C2=A0 =C2=A0.\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING I= N ANY WAY OUT OF THE USE OF
>>=C2=A0 =C2=A0.\" THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBI= LITY OF SUCH DAMAGE.
>>=C2=A0 =C2=A0.\"
>> -.Dd January 15, 2017
>> +.Dd January 9, 2025
>>=C2=A0 =C2=A0.Dt DEVICE_IDENTIFY 9
>>=C2=A0 =C2=A0.Os
>>=C2=A0 =C2=A0.Sh NAME
>>=C2=A0 =C2=A0.Nm DEVICE_IDENTIFY
>> -.Nd identify a device, register it
>> +.Nd identify new child devices and register them
>>=C2=A0 =C2=A0.Sh SYNOPSIS
>>=C2=A0 =C2=A0.In sys/param.h
>>=C2=A0 =C2=A0.In sys/bus.h
>>=C2=A0 =C2=A0.Ft void
>>=C2=A0 =C2=A0.Fn DEVICE_IDENTIFY "driver_t *driver" "= ;device_t parent"
> So what is the 'parent' for driver which creates devices based= on the
> firmware tables?

Hmmm, I could maybe try to clarify this further.=C2=A0 In new-bus, drivers = are
associated with a parent bus devclass.=C2=A0 All of the drivers associated = with
a given parent bus are then eligible for use with children of any bus
devices for that bus.=C2=A0 Thus, for example:

DRIVER_MODULE(foo, bar, ....)

Associates the "bar" driver with the bus "foo".=C2=A0 F= or any fooX bus devices,
the "bar" driver can attach to children of fooX.

Most device_if.m methods operate on "child" devices, so device_pr= obe,
device_attach, device_detach, etc. all operate on a given barX device
that is a child of a fooX.

device_identify is different.=C2=A0 Instead, each fooX bus device can call<= br> bus_identify_children (formerly the somewhat misnamed bus_generic_probe) during the fooX device_attach routine.=C2=A0 bus_identify_children looks fo= r
the devclass of foo and then walks all of the eligible device drivers
for potential children of foo invoking this method with fooX as the
parent.=C2=A0 The idea being that device_identify will create "barX&qu= ot; children
of "fooX" explicitly using BUS_ADD_CHILD.

In terms of which parent device, it's really about where a given "= barX"
device should live.=C2=A0 For system-wide "top-level" devices tha= t aren't
behind some other bridge on an ACPI system, the pattern we use is to
hang those devices as children of acpi0, so you end up with

DRIVER_MODULE(acpi, bar, ...)

and the parent device at the time of DEVICE_IDENTIFY is acpi0.=C2=A0 Howeve= r,
we also use identify in some other places.=C2=A0 nexus0 children all tend t= o
either be explicitly added in nexus_attach() or added via identify
routines that use nexus as the parent.=C2=A0 legacy0 on x86 also uses ident= ify
routines to add child devices like pcibX.

These are likely OK. It's arguably possible to model this as devices= known
in other places, but it gets super awkward and you'd h= ave to do hand-stands
that are worse in other ways.
=C2= =A0
Another case is the ipmi(4) device.=C2=A0 Legacy ISA IPMI devices are descr= ibed
by an entry in the SMBIOS table.=C2=A0 The ipmi(4) driver uses an identify<= br> routine to add an ipmi0 device as a child of isa0 (since it has I/O ports like a typical ISA device), but the identify routine is table-driven since<= br> it depends on parsing the smbios table.

This is bogus, imho, but not worth fixing. We should have a isasmbb device=
that parses the child, creates=C2=A0a isasmb bus and then adds t= he children it
finds from parsing the smbtable and moves on. It w= ould be much cleaner.
There's several other devices that kind= a live here, but none worth supporting
these days, so a rewrite i= s a waste of time.
=C2=A0
cpufreqX is another odd case, and I'm not quite convinced it is correct= .
Today we enumerate cpuX devices hung off of some nexus-like device (on
x86 cpuX are children of either acpi0 or legacy0).=C2=A0 Each cpufreqX
driver then uses identify routines to add named children (p4tccX, estX,
hwpstateX, etc.).=C2=A0 Those identify routines all have "cpu" as= the parent
bus so that cpuX is the parent device (and they are called for each
instance of a cpuX device).=C2=A0 For cpufreq I feel like we actually want<= br> something a bit different on x86 at least.=C2=A0 I think we want to create<= br> an explicit cpufreqX device in cpu_attach, and that the various cpufreq
drivers that manage frequency should all be "cpufreq" drivers tha= t bid
to attach to that device node instead of creating duplicate nodes that
try to duplicate work.=C2=A0 Today the various identify routines try to
check for each other instead which is fragile.=C2=A0 It may be that we'= d
need/want two device_t nodes, one for P-states and one for throttling,
though it might be that we only want the throttling for certain P-state
drivers or some such.

This is definitel= y wrong in a big way. I have about 80-90% of the conversion
to be= proper children of cpu nodes, as appropriate.

War= ner
=C2=A0
For your IOMMU case, you can use an ACPI table "anywhere" in the = device
tree to enumerate device nodes if necessary (though PCI buses don't
currently call bus_identify_children today and don't have a BUS_ADD_CHI= LD
method), but if you want to be "ready" before other generic child= ren like
PCI bridges are attached, you probably need to be a child of acpi0.


=C2=A0
--
John Baldwin

--0000000000000b282d062b5a967c--