On Fri, 24 Apr 2020 19:07:35 +0300 Andriy Gapon <a...@freebsd.org> wrote:
> On 24/04/2020 18:11, Warner Losh wrote: > > > > > > On Fri, Apr 24, 2020 at 8:47 AM Andriy Gapon <a...@freebsd.org > > <mailto:a...@freebsd.org>> wrote: > > > > On 24/04/2020 17:29, Warner Losh wrote: > > > > > > > > > On Fri, Apr 24, 2020 at 1:49 AM Andriy Gapon <a...@freebsd.org > > > > > <mailto:a...@freebsd.org> > > > <mailto:a...@freebsd.org <mailto:a...@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 :) > > > 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. > > 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. > > 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. > > > 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. > 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 _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
