| Message ID | 20200826144920.110605-1-sergey.senozhatsky@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | i2c: do not acpi/of match device in i2c_device_probe() | expand |
On Wed, Aug 26, 2020 at 11:49:20PM +0900, Sergey Senozhatsky wrote: > i2c, apparently, can match the same device twice - the first > time in ->match bus hook (i2c_device_match()), and the second > one in ->probe (i2c_device_probe()) bus hook. > > To make things more complicated, the second matching does not > do exactly same checks as the first one. Namely, i2c_device_match() > calls acpi_driver_match_device() which considers devices that > provide of_match_table and performs of_compatible() matching for > such devices. One important thing to note here is that ACPI > of_compatible() matching (acpi_of_match_device()) is part of ACPI > and does not depend on CONFIG_OF. > > i2c_device_probe(), on the other hand, calls acpi_match_device() > which does not perform of_compatible() matching, but instead > i2c_device_probe() relies on CONFIG_OF API to perform of_match_table > matching, IOW ->probe matching, unlike ->match matching, depends on > CONFIG_OF. This can break i2c device probing on !CONFIG_OF systems > if the device does not provide .id_table. > > i2c_device_probe() > ... > if (!driver->id_table && > !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && > !i2c_of_match_device(dev->driver->of_match_table, client)) { > status = -ENODEV; > goto put_sync_adapter; > } > > i2c_of_match_device() on !CONFIG_OF systems is always false, so we never > perform of_match_table matching. i2c_acpi_match_device() does ACPI match > only, no of_compatible() matching takes place, even though the device > provides .of_match_table and ACPI is capable of matching such device. > > It is not entirely clear why the device is matched again in bus > ->probe after successful and proper matching in bus ->match. Let's > remove ->probe matching. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> (assuming it's okay to go) > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/i2c/i2c-core-base.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 5ec082e2039d..77eea5c0bc71 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -475,17 +475,6 @@ static int i2c_device_probe(struct device *dev) > > driver = to_i2c_driver(dev->driver); > > - /* > - * An I2C ID table is not mandatory, if and only if, a suitable OF > - * or ACPI ID table is supplied for the probing device. > - */ > - if (!driver->id_table && > - !acpi_driver_match_device(dev, dev->driver) && > - !i2c_of_match_device(dev->driver->of_match_table, client)) { > - status = -ENODEV; > - goto put_sync_adapter; > - } > - > if (client->flags & I2C_CLIENT_WAKE) { > int wakeirq; > > -- > 2.28.0 >
On (20/08/26 18:16), Andy Shevchenko wrote: > On Wed, Aug 26, 2020 at 11:49:20PM +0900, Sergey Senozhatsky wrote: > > i2c, apparently, can match the same device twice - the first > > time in ->match bus hook (i2c_device_match()), and the second > > one in ->probe (i2c_device_probe()) bus hook. > > > > To make things more complicated, the second matching does not > > do exactly same checks as the first one. Namely, i2c_device_match() > > calls acpi_driver_match_device() which considers devices that > > provide of_match_table and performs of_compatible() matching for > > such devices. One important thing to note here is that ACPI > > of_compatible() matching (acpi_of_match_device()) is part of ACPI > > and does not depend on CONFIG_OF. > > > > i2c_device_probe(), on the other hand, calls acpi_match_device() > > which does not perform of_compatible() matching, but instead > > i2c_device_probe() relies on CONFIG_OF API to perform of_match_table > > matching, IOW ->probe matching, unlike ->match matching, depends on > > CONFIG_OF. This can break i2c device probing on !CONFIG_OF systems > > if the device does not provide .id_table. > > > > i2c_device_probe() > > ... > > if (!driver->id_table && > > !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && > > !i2c_of_match_device(dev->driver->of_match_table, client)) { > > status = -ENODEV; > > goto put_sync_adapter; > > } > > > > i2c_of_match_device() on !CONFIG_OF systems is always false, so we never > > perform of_match_table matching. i2c_acpi_match_device() does ACPI match > > only, no of_compatible() matching takes place, even though the device > > provides .of_match_table and ACPI is capable of matching such device. > > > > It is not entirely clear why the device is matched again in bus > > ->probe after successful and proper matching in bus ->match. Let's > > remove ->probe matching. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > (assuming it's okay to go) Thanks. I tested the patch on x86_64 (a mix of i2c devices with and without .id_table) and arm64 boards - didn't notice any difference, module probing wise. -ss
On (20/08/26 23:49), Sergey Senozhatsky wrote: > i2c, apparently, can match the same device twice - the first > time in ->match bus hook (i2c_device_match()), and the second > one in ->probe (i2c_device_probe()) bus hook. > > To make things more complicated, the second matching does not > do exactly same checks as the first one. Namely, i2c_device_match() > calls acpi_driver_match_device() which considers devices that > provide of_match_table and performs of_compatible() matching for > such devices. One important thing to note here is that ACPI > of_compatible() matching (acpi_of_match_device()) is part of ACPI > and does not depend on CONFIG_OF. > > i2c_device_probe(), on the other hand, calls acpi_match_device() > which does not perform of_compatible() matching, but instead > i2c_device_probe() relies on CONFIG_OF API to perform of_match_table > matching, IOW ->probe matching, unlike ->match matching, depends on > CONFIG_OF. This can break i2c device probing on !CONFIG_OF systems > if the device does not provide .id_table. > > i2c_device_probe() > ... > if (!driver->id_table && > !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && > !i2c_of_match_device(dev->driver->of_match_table, client)) { > status = -ENODEV; > goto put_sync_adapter; > } > > i2c_of_match_device() on !CONFIG_OF systems is always false, so we never > perform of_match_table matching. i2c_acpi_match_device() does ACPI match > only, no of_compatible() matching takes place, even though the device > provides .of_match_table and ACPI is capable of matching such device. > > It is not entirely clear why the device is matched again in bus > ->probe after successful and proper matching in bus ->match. Let's > remove ->probe matching. Hi, Gentle ping. -ss
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 5ec082e2039d..77eea5c0bc71 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -475,17 +475,6 @@ static int i2c_device_probe(struct device *dev) driver = to_i2c_driver(dev->driver); - /* - * An I2C ID table is not mandatory, if and only if, a suitable OF - * or ACPI ID table is supplied for the probing device. - */ - if (!driver->id_table && - !acpi_driver_match_device(dev, dev->driver) && - !i2c_of_match_device(dev->driver->of_match_table, client)) { - status = -ENODEV; - goto put_sync_adapter; - } - if (client->flags & I2C_CLIENT_WAKE) { int wakeirq;
i2c, apparently, can match the same device twice - the first time in ->match bus hook (i2c_device_match()), and the second one in ->probe (i2c_device_probe()) bus hook. To make things more complicated, the second matching does not do exactly same checks as the first one. Namely, i2c_device_match() calls acpi_driver_match_device() which considers devices that provide of_match_table and performs of_compatible() matching for such devices. One important thing to note here is that ACPI of_compatible() matching (acpi_of_match_device()) is part of ACPI and does not depend on CONFIG_OF. i2c_device_probe(), on the other hand, calls acpi_match_device() which does not perform of_compatible() matching, but instead i2c_device_probe() relies on CONFIG_OF API to perform of_match_table matching, IOW ->probe matching, unlike ->match matching, depends on CONFIG_OF. This can break i2c device probing on !CONFIG_OF systems if the device does not provide .id_table. i2c_device_probe() ... if (!driver->id_table && !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && !i2c_of_match_device(dev->driver->of_match_table, client)) { status = -ENODEV; goto put_sync_adapter; } i2c_of_match_device() on !CONFIG_OF systems is always false, so we never perform of_match_table matching. i2c_acpi_match_device() does ACPI match only, no of_compatible() matching takes place, even though the device provides .of_match_table and ACPI is capable of matching such device. It is not entirely clear why the device is matched again in bus ->probe after successful and proper matching in bus ->match. Let's remove ->probe matching. Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/i2c/i2c-core-base.c | 11 ----------- 1 file changed, 11 deletions(-)