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

[Warner Losh]

On Fri, Apr 24, 2020 at 8:47 AM Andriy Gapon <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>> 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. 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).

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.

Warner


>
> >       MFC after:    2 weeks
> >
> >     Modified:
> >       head/sys/dev/ichiic/ig4_acpi.c
> >
> >     Modified: head/sys/dev/ichiic/ig4_acpi.c
> >
>  ==============================================================================
> >     --- head/sys/dev/ichiic/ig4_acpi.c      Fri Apr 24 05:05:58 2020
>
> >     (r360240)
> >     +++ head/sys/dev/ichiic/ig4_acpi.c      Fri Apr 24 07:49:21 2020
>
> >     (r360241)
> >     @@ -192,5 +192,6 @@ static driver_t ig4iic_acpi_driver = {
> >             sizeof(struct ig4iic_softc),
> >      };
> >
> >     -DRIVER_MODULE(ig4iic, acpi, ig4iic_acpi_driver, ig4iic_devclass, 0,
> 0);
> >     +DRIVER_MODULE_ORDERED(ig4iic, acpi, ig4iic_acpi_driver,
> ig4iic_devclass, 0, 0,
> >     +    SI_ORDER_ANY);
> >      MODULE_DEPEND(ig4iic, acpi, 1, 1, 1);
> >
>
>
> --
> Andriy Gapon
>