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

[Warner Losh]

On Fri, Apr 24, 2020 at 10:07 AM 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 :)
>

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