svn commit: r360241 - head/sys/dev/ichiic

Fri Apr 24 16:24:07 UTC 2020

On Fri, 24 Apr 2020 19:07:35 +0300
Andriy Gapon <avg at FreeBSD.org> wrote:

> On 24/04/2020 18:11, Warner Losh wrote:
> > 
> > 
> > On Fri, Apr 24, 2020 at 8:47 AM Andriy Gapon <avg at freebsd.org
> > <mailto:avg at freebsd.org>> wrote:
> > 
> >     On 24/04/2020 17:29, Warner Losh wrote:  
> >     >
> >     >
> >     > On Fri, Apr 24, 2020 at 1:49 AM Andriy Gapon <avg at freebsd.org
> >     >  
> >     <mailto:avg at freebsd.org>  
> >     > <mailto:avg at freebsd.org <mailto:avg at 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