Date: Fri, 24 Apr 2020 11:23:56 -0500 From: Justin Hibbits <chmeeedalf@gmail.com> To: Andriy Gapon <avg@FreeBSD.org> Cc: Warner Losh <imp@bsdimp.com>, 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: <20200424112356.6b8a867e@titan.knownspace> 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, 24 Apr 2020 19:07:35 +0300 Andriy Gapon <avg@FreeBSD.org> wrote: > On 24/04/2020 18:11, Warner Losh wrote: > >=20 > >=20 > > On Fri, Apr 24, 2020 at 8:47 AM Andriy Gapon <avg@freebsd.org > > <mailto:avg@freebsd.org>> wrote: > >=20 > > On 24/04/2020 17:29, Warner Losh wrote: =20 > > > > > > > > > On Fri, Apr 24, 2020 at 1:49 AM Andriy Gapon <avg@freebsd.org > > > =20 > > <mailto:avg@freebsd.org> =20 > > > <mailto:avg@freebsd.org <mailto:avg@freebsd.org>>> wrote: > > > > > >=C2=A0 =C2=A0 =C2=A0Author: avg > > >=C2=A0 =C2=A0 =C2=A0Date: Fri Apr 24 07:49:21 2020 > > >=C2=A0 =C2=A0 =C2=A0New Revision: 360241 > > >=C2=A0 =C2=A0 =C2=A0URL: https://svnweb.freebsd.org/changeset/base= /360241 > > > > > >=C2=A0 =C2=A0 =C2=A0Log: > > >=C2=A0 =C2=A0 =C2=A0=C2=A0 ig4: ensure that drivers always attach = in correct order > > > > > >=C2=A0 =C2=A0 =C2=A0=C2=A0 Use DRIVER_MODULE_ORDERED(SI_ORDER_ANY)= so that ig4's > > >ACPI attachment happens after iicbus and acpi_iicbus drivers > > >are registered. > > > > > >=C2=A0 =C2=A0 =C2=A0=C2=A0 I have seen a problem where iicbus atta= ched under ig4 > > >instead of acpi_iicbus when ig4.ko was loaded with kldload.=C2=A0 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.=C2=A0 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. =20 > >=20 > > I think I listed them all: ig4iic with acpi attachment, iicbus > > and acpi_iicbus. acpi > > =C2=A0\--- ig4iic > > =C2=A0 =C2=A0 =C2=A0 =C2=A0\---- iicbus vs acpi_iicbus > >=20 > > I tried to explain the problem and the fix in the commit > > message.=C2=A0 If you want to discuss any specifics, let's continue with > > specifics.=C2=A0 If there is anything unclear in my explanation, I can > > clarify, but I need to understand what needs to be clarified. > >=20 > >=20 > > That won't fix the stated problem. =20 >=20 > It will. It does. You made me write an essay to explain why :) >=20 > > 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). > >=20 > > iicbus returns BUS_PROBE_GENERIC (-100) from its probe routine, > > while acpi_iicbus returns=C2=A0BUS_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). =20 >=20 > This driver, along with all of other I2C controller drivers, has a > particular quirk, so it can be called weird. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > > 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. =20 >=20 > Just for clarity, acpi_iicbus is compiled into iicbus.ko (on relevant > platforms, of course), same as iicbus. It's not a separate kld. >=20 Can you look at how ofw_iicbus does this? Everything works just fine with that, and it's compiled into the iicbus module as well. Perhaps you can pick some ideas from there. One thing I remember doing on the fsl_i2c driver was to just name the driver iichb and everything worked beautifully. Yes, it was sort of a cop-out vs adding another attachment, but it solved the problem, and does make sense. - Justin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20200424112356.6b8a867e>