Date: Fri, 24 Apr 2020 10:57:19 -0600 From: Warner Losh <imp@bsdimp.com> To: Andriy Gapon <avg@freebsd.org> Cc: John Baldwin <jhb@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r360241 - head/sys/dev/ichiic Message-ID: <CANCZdfqVwfaOKNTqefpTUJ0%2BQ7i0iGO==AtHaXEvVB-uV2W51A@mail.gmail.com> In-Reply-To: <02449f05-6c43-9cb0-b427-34034a3c358f@FreeBSD.org> References: <202004240749.03O7nMSc066344@repo.freebsd.org> <CANCZdfok8rf3SWEtKA_ZHVAgPWHCzOM-95w2pP9UTTLn-9995w@mail.gmail.com> <32586115-2c82-2497-ba4c-b3471887518d@FreeBSD.org> <CANCZdfqJaBm=Jx43jU5%2BiB1KVx3f1aoSUTDB3U6z9QnPV_q6uQ@mail.gmail.com> <02449f05-6c43-9cb0-b427-34034a3c358f@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Apr 24, 2020 at 10:07 AM Andriy Gapon <avg@freebsd.org> wrote: > On 24/04/2020 18:11, Warner Losh wrote: > > > > > > On Fri, Apr 24, 2020 at 8:47 AM Andriy Gapon <avg@freebsd.org > > <mailto:avg@freebsd.org>> wrote: > > > > On 24/04/2020 17:29, Warner Losh wrote: > > > > > > > > > On Fri, Apr 24, 2020 at 1:49 AM Andriy Gapon <avg@freebsd.org > > <mailto:avg@freebsd.org> > > > <mailto:avg@freebsd.org <mailto:avg@freebsd.org>>> wrote: > > > > > > Author: avg > > > Date: Fri Apr 24 07:49:21 2020 > > > New Revision: 360241 > > > URL: https://svnweb.freebsd.org/changeset/base/360241 > > > > > > Log: > > > ig4: ensure that drivers always attach in correct order > > > > > > Use DRIVER_MODULE_ORDERED(SI_ORDER_ANY) so that ig4's ACPI > attachment > > > happens after iicbus and acpi_iicbus drivers are registered. > > > > > > I have seen a problem where iicbus attached under ig4 > instead of > > > acpi_iicbus when ig4.ko was loaded with kldload. I believe > that that > > > happened because ig4 driver was a first driver to register, > it attached > > > and created an iicbus child. Then iicbus driver was > registered and, > > > since it was the only driver that could attach to the iicbus > child > > > device, it did exactly that. After that acpi_iicbus driver > was > > > registered. It would be able to attach to the iicbus > device, but it was > > > already attached, so nothing happened. > > > > > > > > > Can you post more details of which devices are affected? From the > description > > > and the patch, I don't see how this could fix things. > > > > I think I listed them all: ig4iic with acpi attachment, iicbus and > acpi_iicbus. > > acpi > > \--- ig4iic > > \---- iicbus vs acpi_iicbus > > > > I tried to explain the problem and the fix in the commit message. > If you want > > to discuss any specifics, let's continue with specifics. If there > is anything > > unclear in my explanation, I can clarify, but I need to understand > what needs to > > be clarified. > > > > > > That won't fix the stated problem. > > It will. It does. You made me write an essay to explain why :) > Ah, it's a workaround for a newbus bug. OK. I'll work on fixing the newbus bug then. > > If changing the module order fixes something, > > it's the wrong fix. Which is why I asked to make sure I understood the > issue (it > > was unclear if it was at the level indicated, or for the children of the > iicbus). > > > > iicbus returns BUS_PROBE_GENERIC (-100) from its probe routine, while > > acpi_iicbus returns BUS_PROBE_DEFAULT (-20). This means that acpi_iicbus > should > > always win. So something else is going on. We don't specify order on > other > > devices except for some weird, special needs drivers (this is not such a > driver). > > This driver, along with all of other I2C controller drivers, has a > particular > quirk, so it can be called weird. > > It does not matter what the probe routines return if one of the drivers is > not > added to "newbus" at all (even if its code is loaded). Its probe method > is not > executed. And that's what I tried to explain in the commit message. > > Now, why this driver as well as all SMBus and I2C controller drivers are > somewhat weird... There is a multitude of drivers for SMBus and I2C > controllers. Those drivers have different names. So, using newbus speak, > smbus > and iicbus drivers can attach to [newbus] devices under many different > [newbus] > buses. For that reason, the corresponding DRIVER_MODULE declarations are > not > consolidated in smbus.c and iicbus.c; instead, they are spread over > individual > controller drivers. So, loading of smbus or iicbus module does not result > in a > call to devclass_add_driver() that would register these drivers under some > bus. > And that's an unusual thing comparing to the most straightforward drivers. > > Let's use ig4 as an example. > Across its source files we had the following DRIVER_MODULE declarations: > DRIVER_MODULE(ig4iic, acpi, ... -- in ig4_acpi.c > DRIVER_MODULE(iicbus, ig4iic, ... -- in ig4_iic.c > DRIVER_MODULE(acpi_iicbus, ig4iic, ... -- in ig4_iic.c > The first one is needed to register ig4iic driver under acpi bus. Other > two are > needed to register iicbus and acpi_iicbus drivers under ig4iic bus. > The order is not explicitly defined, so the corresponding declaration can > be > processed in any order when ig4.ko is loaded and so the corresponding > devclass_add_driver() can be called in any order. > It's a bug in newbus if that matters. I'll grant that it does today, but it shouldn't. In the past we've worked around this issue in a number of different ways (including having 3 different modules with the order encoded into those modules). It also indicates, perhaps, bugs in the iic acpi enumeration stuff, but that's a harder case to make without more careful study since I know that acpi_iic works around the bit of a mismatch between newbus' device model and ACPI's. > So, I observed in practice the scenario where the drivers were added in the > written order. First, ig4iic was added under acpi, it found the > corresponding > device, probed and attached to it. In its attach method it added a new > device > named "iicbus" as a child. Then, iicbus driver was added under ig4iic bus. > That driver found the child device and attached to it. Only then > acpi_iicbus > was added and it was too late to the party. > > So, this is really about an order in which DRIVER_MODULE-s (which > translate to > sysinit-s) _within a kld_ are processed. Essentially, about an order of > objects > in the corresponding section of a loadable ELF object. MODULE_DEPEND does > not > affect that order. > Yes. That's why we rarely put multiple modules into one .ko. One can have a debate about whether or not this is a bug in the loader or not... But we've been talking for a while about deferring the probe/attach phase of the driver registration until after everything is registered, but while the concept is simple, the details can be tricky. There's some support for this with devctl freeze/unfreeze, but maybe that should move into kldload itself. That too would solve this problem. > Just as an aside, for a contrast, gpiobus uses a different model. > There, all controller drivers must have "gpio" as their driver names > (driver->name). > So, a single DRIVER_MODULE(gpiobus, gpio, ...) in the gpiobus.c code is > sufficient for gpiobus driver to be added under all controllers. And that > registration always happens before a controller driver is added because of > MODULE_DEPEND between it and gpiobus. > Yes. This is one of the drivers I was thinking didn't co-locate things. > > Unless I'm mistaken, the real problem is that the following line is > missing instead. > > MODULE_DEPEND(ig4iic, acpi_iicbus, 1, 1, 1); > > which makes the module dependencies explicit. Can you add that to > ig4_acpi.c, > > revert your change and see if that works? It will ensure that the > acpi_iicbus > > driver is loaded first. If things break because it isn't, it sounds like > a hard > > dependency for ig4. Since we're already pulling in acpi, that doesn't > seem > > unreasonable to me. > > Just for clarity, acpi_iicbus is compiled into iicbus.ko (on relevant > platforms, > of course), same as iicbus. It's not a separate kld. > So I was mistaken about what your second message was trying to say. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqVwfaOKNTqefpTUJ0%2BQ7i0iGO==AtHaXEvVB-uV2W51A>