| Message ID | 20201204075041.44339-1-hui.wang@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI / bus: skip the primary physical pnp device in companion_match | expand |
On Fri, Dec 4, 2020 at 8:51 AM Hui Wang <hui.wang@canonical.com> wrote: > > We are working on some latest Thinkpad Yoga and Carbon laptops, the > touchscreen doesn't work on those machines. And we found the > touchscreen module is I2C wacom WACF2200 (056A:5276). > > The problem is in the acpi_pnp.c, the WACFXXX is in the > acpi_pnp_device_ids[], so a pnp device will be built and attach to the > acpi_dev as the 1st physical_node, later when I2C subsystem starts to > initialize, it will build an I2C_dev and attach to the acpi_dev as the > 2nd physical_node. When I2C bus needs to match the acpi_id_table, it > will call acpi_companion_match(), because the 1st physical_node is not > I2C_dev, it fails to match, then the i2c driver (hid_i2c) will not be > called. > > To fix it, adding a special treatment in the companion_match(): if the > 1st dev is on pnp bus and the device in question is not on pnp bus, > skip the 1st physical device, just use the device in question to > match. > > We could refer to the pnpacpi_add_device() in the > pnp/pnpacpi/core.c, pnp device will not be built if the acpi_dev > is already attached to a physical device, so a pnp device has > lower priority than other devices, it is safe to skip it in > the companion_match(). > > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > drivers/acpi/bus.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index 1682f8b454a2..8aa0a861ca29 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -582,6 +582,15 @@ bool acpi_device_is_first_physical_node(struct acpi_device *adev, > return !!acpi_primary_dev_companion(adev, dev); > } > > +/* Could move this function to linux/pnp.h in the future */ > +static bool acpi_dev_is_on_pnp_bus(const struct device *dev) > +{ > + if (dev->bus) > + return !strcmp(dev->bus->name, "pnp"); > + else Unnecessary else. > + return false; > +} > + > /* > * acpi_companion_match() - Can we match via ACPI companion device > * @dev: Device in question > @@ -597,7 +606,9 @@ bool acpi_device_is_first_physical_node(struct acpi_device *adev, > * companion. A typical case is an MFD device where all the sub-devices share > * the parent's ACPI companion. In such cases we can only allow the primary > * (first) physical device to be matched with the help of the companion's PNP > - * IDs. > + * IDs. And another case is a pnp device is attached to ACPI device first, then > + * other function devices are attached too, in this case, the primary physical > + * device (pnp) is ignored, just use the device in question to match. > * > * Additional physical devices sharing the ACPI companion can still use > * resources available from it but they will be matched normally using functions > @@ -605,7 +616,7 @@ bool acpi_device_is_first_physical_node(struct acpi_device *adev, > */ > struct acpi_device *acpi_companion_match(const struct device *dev) > { > - struct acpi_device *adev; > + struct acpi_device *adev, *radev; > > adev = ACPI_COMPANION(dev); > if (!adev) > @@ -614,7 +625,15 @@ struct acpi_device *acpi_companion_match(const struct device *dev) > if (list_empty(&adev->pnp.ids)) > return NULL; > > - return acpi_primary_dev_companion(adev, dev); > + radev = acpi_primary_dev_companion(adev, dev); > + if (radev == NULL) { > + const struct device *first_dev = acpi_get_first_physical_node(adev); > + > + if (acpi_dev_is_on_pnp_bus(first_dev) && !acpi_dev_is_on_pnp_bus(dev)) > + radev = adev; > + } > + > + return radev; > } This is too convoluted IMV. Would dropping the device ID in question from acpi_pnp_device_ids[] make the problem go away? If so, why don't you do just that?
On 12/7/20 9:11 PM, Rafael J. Wysocki wrote: > On Fri, Dec 4, 2020 at 8:51 AM Hui Wang <hui.wang@canonical.com> wrote: >> We are working on some latest Thinkpad Yoga and Carbon laptops, the >> touchscreen doesn't work on those machines. And we found the >> touchscreen module is I2C wacom WACF2200 (056A:5276). >> >> The problem is in the acpi_pnp.c, the WACFXXX is in the >> acpi_pnp_device_ids[], so a pnp device will be built and attach to the >> acpi_dev as the 1st physical_node, later when I2C subsystem starts to >> initialize, it will build an I2C_dev and attach to the acpi_dev as the >> 2nd physical_node. When I2C bus needs to match the acpi_id_table, it >> will call acpi_companion_match(), because the 1st physical_node is not >> I2C_dev, it fails to match, then the i2c driver (hid_i2c) will not be >> called. >> >> To fix it, adding a special treatment in the companion_match(): if the >> 1st dev is on pnp bus and the device in question is not on pnp bus, >> skip the 1st physical device, just use the device in question to >> match. >> >> We could refer to the pnpacpi_add_device() in the >> pnp/pnpacpi/core.c, pnp device will not be built if the acpi_dev >> is already attached to a physical device, so a pnp device has >> lower priority than other devices, it is safe to skip it in >> the companion_match(). >> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> drivers/acpi/bus.c | 26 ++++++++++++++++++++++---- >> 1 file changed, 22 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c >> index 1682f8b454a2..8aa0a861ca29 100644 >> --- a/drivers/acpi/bus.c >> +++ b/drivers/acpi/bus.c >> @@ -582,6 +582,15 @@ bool acpi_device_is_first_physical_node(struct acpi_device *adev, >> return !!acpi_primary_dev_companion(adev, dev); >> } >> >> +/* Could move this function to linux/pnp.h in the future */ >> +static bool acpi_dev_is_on_pnp_bus(const struct device *dev) >> +{ >> + if (dev->bus) >> + return !strcmp(dev->bus->name, "pnp"); >> + else > Unnecessary else. OK, got it. no need to check the bus, then no need to add the else. >> + return false; >> +} >> + >> /* >> * acpi_companion_match() - Can we match via ACPI companion device >> * @dev: Device in question >> @@ -597,7 +606,9 @@ bool acpi_device_is_first_physical_node(struct acpi_device *adev, >> * companion. A typical case is an MFD device where all the sub-devices share >> * the parent's ACPI companion. In such cases we can only allow the primary >> * (first) physical device to be matched with the help of the companion's PNP >> - * IDs. >> + * IDs. And another case is a pnp device is attached to ACPI device first, then >> + * other function devices are attached too, in this case, the primary physical >> + * device (pnp) is ignored, just use the device in question to match. >> * >> * Additional physical devices sharing the ACPI companion can still use >> * resources available from it but they will be matched normally using functions >> @@ -605,7 +616,7 @@ bool acpi_device_is_first_physical_node(struct acpi_device *adev, >> */ >> struct acpi_device *acpi_companion_match(const struct device *dev) >> { >> - struct acpi_device *adev; >> + struct acpi_device *adev, *radev; >> >> adev = ACPI_COMPANION(dev); >> if (!adev) >> @@ -614,7 +625,15 @@ struct acpi_device *acpi_companion_match(const struct device *dev) >> if (list_empty(&adev->pnp.ids)) >> return NULL; >> >> - return acpi_primary_dev_companion(adev, dev); >> + radev = acpi_primary_dev_companion(adev, dev); >> + if (radev == NULL) { >> + const struct device *first_dev = acpi_get_first_physical_node(adev); >> + >> + if (acpi_dev_is_on_pnp_bus(first_dev) && !acpi_dev_is_on_pnp_bus(dev)) >> + radev = adev; >> + } >> + >> + return radev; >> } > This is too convoluted IMV. > > Would dropping the device ID in question from acpi_pnp_device_ids[] > make the problem go away? > > If so, why don't you do just that? Yes, if remove "WACFXXX" from acpi_pnp_device_ids[], could fix this issue. I planned to do so, but I found the pnp_driver in the drivers/tty/serial/8250/8250_pnp.c still handle this ID, maybe it could introduce regression on old machines if removing it. Thanks.
On Tue, Dec 8, 2020 at 3:02 AM Hui Wang <hui.wang@canonical.com> wrote: > > > On 12/7/20 9:11 PM, Rafael J. Wysocki wrote: > > On Fri, Dec 4, 2020 at 8:51 AM Hui Wang <hui.wang@canonical.com> wrote: [cut] > > > > Would dropping the device ID in question from acpi_pnp_device_ids[] > > make the problem go away? > > > > If so, why don't you do just that? > > Yes, if remove "WACFXXX" from acpi_pnp_device_ids[], could fix this > issue. I planned to do so, but I found the pnp_driver in the > drivers/tty/serial/8250/8250_pnp.c still handle this ID, maybe it could > introduce regression on old machines if removing it. Well, "WACFXXX" is not a proper device ID, it is a wild card possibly matching too many devices. What device ID specifically is used in the ACPI tables for the device in question?
On 12/8/20 10:01 PM, Rafael J. Wysocki wrote: > On Tue, Dec 8, 2020 at 3:02 AM Hui Wang <hui.wang@canonical.com> wrote: >> >> On 12/7/20 9:11 PM, Rafael J. Wysocki wrote: >>> On Fri, Dec 4, 2020 at 8:51 AM Hui Wang <hui.wang@canonical.com> wrote: > [cut] > >>> Would dropping the device ID in question from acpi_pnp_device_ids[] >>> make the problem go away? >>> >>> If so, why don't you do just that? >> Yes, if remove "WACFXXX" from acpi_pnp_device_ids[], could fix this >> issue. I planned to do so, but I found the pnp_driver in the >> drivers/tty/serial/8250/8250_pnp.c still handle this ID, maybe it could >> introduce regression on old machines if removing it. > Well, "WACFXXX" is not a proper device ID, it is a wild card possibly > matching too many devices. > > What device ID specifically is used in the ACPI tables for the device > in question? It is "WACF2200", how about the change as below, is it safe to say the length of a pnp device id is 7? diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c index 4ed755a963aa..1e828378238c 100644 --- a/drivers/acpi/acpi_pnp.c +++ b/drivers/acpi/acpi_pnp.c @@ -336,6 +336,10 @@ static bool acpi_pnp_match(const char *idstr, const struct acpi_device_id **matc { const struct acpi_device_id *devid; + /* Expect the pnp device id has CCCdddd format (C character, d digital) */ + if (strlen(idstr) != 7) + return false; + for (devid = acpi_pnp_device_ids; devid->id[0]; devid++) if (matching_id(idstr, (char *)devid->id)) { if (matchid) Thanks.
On Wed, Dec 9, 2020 at 4:08 AM Hui Wang <hui.wang@canonical.com> wrote: > > > On 12/8/20 10:01 PM, Rafael J. Wysocki wrote: > > On Tue, Dec 8, 2020 at 3:02 AM Hui Wang <hui.wang@canonical.com> wrote: > >> > >> On 12/7/20 9:11 PM, Rafael J. Wysocki wrote: > >>> On Fri, Dec 4, 2020 at 8:51 AM Hui Wang <hui.wang@canonical.com> wrote: > > [cut] > > > >>> Would dropping the device ID in question from acpi_pnp_device_ids[] > >>> make the problem go away? > >>> > >>> If so, why don't you do just that? > >> Yes, if remove "WACFXXX" from acpi_pnp_device_ids[], could fix this > >> issue. I planned to do so, but I found the pnp_driver in the > >> drivers/tty/serial/8250/8250_pnp.c still handle this ID, maybe it could > >> introduce regression on old machines if removing it. > > Well, "WACFXXX" is not a proper device ID, it is a wild card possibly > > matching too many devices. > > > > What device ID specifically is used in the ACPI tables for the device > > in question? > > It is "WACF2200", how about the change as below, is it safe to say the > length of a pnp device id is 7? > > diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c > index 4ed755a963aa..1e828378238c 100644 > --- a/drivers/acpi/acpi_pnp.c > +++ b/drivers/acpi/acpi_pnp.c > @@ -336,6 +336,10 @@ static bool acpi_pnp_match(const char *idstr, const > struct acpi_device_id **matc > { > const struct acpi_device_id *devid; > > + /* Expect the pnp device id has CCCdddd format (C character, d > digital) */ > + if (strlen(idstr) != 7) > + return false; > + > for (devid = acpi_pnp_device_ids; devid->id[0]; devid++) > if (matching_id(idstr, (char *)devid->id)) { > if (matchid) Alternatively, matching_id() can be updated to compare the length (which arguably it should be doing).
On 12/10/20 12:56 AM, Rafael J. Wysocki wrote: > On Wed, Dec 9, 2020 at 4:08 AM Hui Wang <hui.wang@canonical.com> wrote: >> >> On 12/8/20 10:01 PM, Rafael J. Wysocki wrote: >>> On Tue, Dec 8, 2020 at 3:02 AM Hui Wang <hui.wang@canonical.com> wrote: >>>> On 12/7/20 9:11 PM, Rafael J. Wysocki wrote: >>>>> On Fri, Dec 4, 2020 at 8:51 AM Hui Wang <hui.wang@canonical.com> wrote: >>> [cut] >>> >>>>> Would dropping the device ID in question from acpi_pnp_device_ids[] >>>>> make the problem go away? >>>>> >>>>> If so, why don't you do just that? >>>> Yes, if remove "WACFXXX" from acpi_pnp_device_ids[], could fix this >>>> issue. I planned to do so, but I found the pnp_driver in the >>>> drivers/tty/serial/8250/8250_pnp.c still handle this ID, maybe it could >>>> introduce regression on old machines if removing it. >>> Well, "WACFXXX" is not a proper device ID, it is a wild card possibly >>> matching too many devices. >>> >>> What device ID specifically is used in the ACPI tables for the device >>> in question? >> It is "WACF2200", how about the change as below, is it safe to say the >> length of a pnp device id is 7? >> >> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c >> index 4ed755a963aa..1e828378238c 100644 >> --- a/drivers/acpi/acpi_pnp.c >> +++ b/drivers/acpi/acpi_pnp.c >> @@ -336,6 +336,10 @@ static bool acpi_pnp_match(const char *idstr, const >> struct acpi_device_id **matc >> { >> const struct acpi_device_id *devid; >> >> + /* Expect the pnp device id has CCCdddd format (C character, d >> digital) */ >> + if (strlen(idstr) != 7) >> + return false; >> + >> for (devid = acpi_pnp_device_ids; devid->id[0]; devid++) >> if (matching_id(idstr, (char *)devid->id)) { >> if (matchid) > Alternatively, matching_id() can be updated to compare the length > (which arguably it should be doing). OK, got it, will do the change in the matching_id(). (putting the length checking in acpi_pnp_match() at least has one benefit, it could reduce some meaningless loops)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 1682f8b454a2..8aa0a861ca29 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -582,6 +582,15 @@ bool acpi_device_is_first_physical_node(struct acpi_device *adev, return !!acpi_primary_dev_companion(adev, dev); } +/* Could move this function to linux/pnp.h in the future */ +static bool acpi_dev_is_on_pnp_bus(const struct device *dev) +{ + if (dev->bus) + return !strcmp(dev->bus->name, "pnp"); + else + return false; +} + /* * acpi_companion_match() - Can we match via ACPI companion device * @dev: Device in question @@ -597,7 +606,9 @@ bool acpi_device_is_first_physical_node(struct acpi_device *adev, * companion. A typical case is an MFD device where all the sub-devices share * the parent's ACPI companion. In such cases we can only allow the primary * (first) physical device to be matched with the help of the companion's PNP - * IDs. + * IDs. And another case is a pnp device is attached to ACPI device first, then + * other function devices are attached too, in this case, the primary physical + * device (pnp) is ignored, just use the device in question to match. * * Additional physical devices sharing the ACPI companion can still use * resources available from it but they will be matched normally using functions @@ -605,7 +616,7 @@ bool acpi_device_is_first_physical_node(struct acpi_device *adev, */ struct acpi_device *acpi_companion_match(const struct device *dev) { - struct acpi_device *adev; + struct acpi_device *adev, *radev; adev = ACPI_COMPANION(dev); if (!adev) @@ -614,7 +625,15 @@ struct acpi_device *acpi_companion_match(const struct device *dev) if (list_empty(&adev->pnp.ids)) return NULL; - return acpi_primary_dev_companion(adev, dev); + radev = acpi_primary_dev_companion(adev, dev); + if (radev == NULL) { + const struct device *first_dev = acpi_get_first_physical_node(adev); + + if (acpi_dev_is_on_pnp_bus(first_dev) && !acpi_dev_is_on_pnp_bus(dev)) + radev = adev; + } + + return radev; } /** @@ -831,7 +850,6 @@ bool acpi_driver_match_device(struct device *dev, return acpi_of_match_device(ACPI_COMPANION(dev), drv->of_match_table, NULL); - return __acpi_match_device(acpi_companion_match(dev), drv->acpi_match_table, drv->of_match_table, NULL, NULL);
We are working on some latest Thinkpad Yoga and Carbon laptops, the touchscreen doesn't work on those machines. And we found the touchscreen module is I2C wacom WACF2200 (056A:5276). The problem is in the acpi_pnp.c, the WACFXXX is in the acpi_pnp_device_ids[], so a pnp device will be built and attach to the acpi_dev as the 1st physical_node, later when I2C subsystem starts to initialize, it will build an I2C_dev and attach to the acpi_dev as the 2nd physical_node. When I2C bus needs to match the acpi_id_table, it will call acpi_companion_match(), because the 1st physical_node is not I2C_dev, it fails to match, then the i2c driver (hid_i2c) will not be called. To fix it, adding a special treatment in the companion_match(): if the 1st dev is on pnp bus and the device in question is not on pnp bus, skip the 1st physical device, just use the device in question to match. We could refer to the pnpacpi_add_device() in the pnp/pnpacpi/core.c, pnp device will not be built if the acpi_dev is already attached to a physical device, so a pnp device has lower priority than other devices, it is safe to skip it in the companion_match(). Signed-off-by: Hui Wang <hui.wang@canonical.com> --- drivers/acpi/bus.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)