| Message ID | 20200826042938.3259-1-sergey.senozhatsky@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/2] i2c: consider devices with of_match_table during i2c device probing | expand |
On (20/08/26 13:29), Sergey Senozhatsky wrote: > Unlike acpi_match_device(), acpi_driver_match_device() does > consider devices that provide of_match_table and performs > of_compatible() matching for such devices. The key point here is > that ACPI of_compatible() matching - acpi_of_match_device() - is > part of ACPI and does not depend on CONFIG_OF. > > Consider the following case: > o !CONFIG_OF system > o probing of i2c device that provides .of_match_table, but no .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() depends on CONFIG_OF and, thus, is always false. > i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching > takes place, even though the device provides .of_match_table and ACPI, > technically, is capable of matching such device. The result is -ENODEV. > Probing will succeed, however, if we'd use .of_match_table aware ACPI > matching. Or, alternatively, we can drop i2c_acpi_match_device() and use i2c_device_match() in i2c_device_probe(). --- diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 34a9609f256d..14bfc5c83f84 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -96,7 +96,6 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv) struct i2c_client *client = i2c_verify_client(dev); struct i2c_driver *driver; - /* Attempt an OF style match */ if (i2c_of_match_device(drv->of_match_table, client)) return 1; @@ -480,8 +479,7 @@ static int i2c_device_probe(struct device *dev) * or ACPI ID table is supplied for the probing device. */ if (!driver->id_table && - !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && - !i2c_of_match_device(dev->driver->of_match_table, client)) { + !(client && i2c_device_match(&client->dev, dev->driver))) { status = -ENODEV; goto put_sync_adapter; } diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h index 94ff1693b391..8ce261167a2d 100644 --- a/drivers/i2c/i2c-core.h +++ b/drivers/i2c/i2c-core.h @@ -59,20 +59,11 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap) } #ifdef CONFIG_ACPI -const struct acpi_device_id * -i2c_acpi_match_device(const struct acpi_device_id *matches, - struct i2c_client *client); void i2c_acpi_register_devices(struct i2c_adapter *adap); int i2c_acpi_get_irq(struct i2c_client *client); #else /* CONFIG_ACPI */ static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { } -static inline const struct acpi_device_id * -i2c_acpi_match_device(const struct acpi_device_id *matches, - struct i2c_client *client) -{ - return NULL; -} static inline int i2c_acpi_get_irq(struct i2c_client *client) {
On Wed, Aug 26, 2020 at 01:29:37PM +0900, Sergey Senozhatsky wrote: > Unlike acpi_match_device(), acpi_driver_match_device() does > consider devices that provide of_match_table and performs > of_compatible() matching for such devices. The key point here is > that ACPI of_compatible() matching - acpi_of_match_device() - is > part of ACPI and does not depend on CONFIG_OF. > > Consider the following case: > o !CONFIG_OF system > o probing of i2c device that provides .of_match_table, but no .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() depends on CONFIG_OF and, thus, is always false. > i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching > takes place, even though the device provides .of_match_table and ACPI, > technically, is capable of matching such device. The result is -ENODEV. > Probing will succeed, however, if we'd use .of_match_table aware ACPI > matching. > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> We have currently this in for-current which is even removing i2c_acpi_match_device(): http://patchwork.ozlabs.org/project/linux-i2c/list/?series=196990&state=* From a glimpse, this is basically equal but CCing Andy for the details. > --- > drivers/i2c/i2c-core-acpi.c | 10 ++++------ > drivers/i2c/i2c-core-base.c | 2 +- > drivers/i2c/i2c-core.h | 10 +++------- > 3 files changed, 8 insertions(+), 14 deletions(-) > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index 2ade99b105b9..1dd152ae75e5 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -276,14 +276,12 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap) > dev_warn(&adap->dev, "failed to enumerate I2C slaves\n"); > } > > -const struct acpi_device_id * > -i2c_acpi_match_device(const struct acpi_device_id *matches, > - struct i2c_client *client) > +bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client) > { > - if (!(client && matches)) > - return NULL; > + if (!client) > + return false; > > - return acpi_match_device(matches, &client->dev); > + return acpi_driver_match_device(&client->dev, dev->driver); > } > > static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = { > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 34a9609f256d..35ec6335852b 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -480,7 +480,7 @@ static int i2c_device_probe(struct device *dev) > * or ACPI ID table is supplied for the probing device. > */ > if (!driver->id_table && > - !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && > + !i2c_acpi_match_device(dev, client) && > !i2c_of_match_device(dev->driver->of_match_table, client)) { > status = -ENODEV; > goto put_sync_adapter; > diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h > index 94ff1693b391..7ee6a6e3fb8d 100644 > --- a/drivers/i2c/i2c-core.h > +++ b/drivers/i2c/i2c-core.h > @@ -59,19 +59,15 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap) > } > > #ifdef CONFIG_ACPI > -const struct acpi_device_id * > -i2c_acpi_match_device(const struct acpi_device_id *matches, > - struct i2c_client *client); > +bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client); > void i2c_acpi_register_devices(struct i2c_adapter *adap); > > int i2c_acpi_get_irq(struct i2c_client *client); > #else /* CONFIG_ACPI */ > static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { } > -static inline const struct acpi_device_id * > -i2c_acpi_match_device(const struct acpi_device_id *matches, > - struct i2c_client *client) > +bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client) > { > - return NULL; > + return false; > } > > static inline int i2c_acpi_get_irq(struct i2c_client *client) > -- > 2.28.0 >
On (20/08/26 07:08), Wolfram Sang wrote: > On Wed, Aug 26, 2020 at 01:29:37PM +0900, Sergey Senozhatsky wrote: > > Unlike acpi_match_device(), acpi_driver_match_device() does > > consider devices that provide of_match_table and performs > > of_compatible() matching for such devices. The key point here is > > that ACPI of_compatible() matching - acpi_of_match_device() - is > > part of ACPI and does not depend on CONFIG_OF. > > > > Consider the following case: > > o !CONFIG_OF system > > o probing of i2c device that provides .of_match_table, but no .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() depends on CONFIG_OF and, thus, is always false. > > i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching > > takes place, even though the device provides .of_match_table and ACPI, > > technically, is capable of matching such device. The result is -ENODEV. > > Probing will succeed, however, if we'd use .of_match_table aware ACPI > > matching. > > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > > We have currently this in for-current which is even removing > i2c_acpi_match_device(): > > http://patchwork.ozlabs.org/project/linux-i2c/list/?series=196990&state=* Oh, nice! Can we go a bit further and use i2c_device_match() in i2c_device_probe() instead of a mix of APIs from different subsystems? E.g. if (!driver->id_table && - !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && - !i2c_of_match_device(dev->driver->of_match_table, client)) { + !(client && i2c_device_match(&client->dev, dev->driver))) { status = -ENODEV; goto put_sync_adapter; } -ss
Hi Sergey,
I love your patch! Yet something to improve:
[auto build test ERROR on v5.9-rc2]
[also build test ERROR on next-20200825]
[cannot apply to wsa/i2c/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
base: d012a7190fc1fd72ed48911e77ca97ba4521bccd
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
sh4-linux-ld: drivers/i2c/i2c-core-smbus.o: in function `i2c_acpi_match_device':
>> (.text+0x2400): multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:(.text+0x3260): first defined here
sh4-linux-ld: drivers/i2c/i2c-core-slave.o: in function `i2c_acpi_match_device':
(.text+0x2c0): multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:(.text+0x3260): first defined here
sh4-linux-ld: drivers/i2c/i2c-core-of.o: in function `i2c_acpi_match_device':
(.text+0x600): multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:(.text+0x3260): first defined here
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Aug 26, 2020 at 02:25:44PM +0900, Sergey Senozhatsky wrote: > On (20/08/26 07:08), Wolfram Sang wrote: > > On Wed, Aug 26, 2020 at 01:29:37PM +0900, Sergey Senozhatsky wrote: > > > Unlike acpi_match_device(), acpi_driver_match_device() does > > > consider devices that provide of_match_table and performs > > > of_compatible() matching for such devices. The key point here is > > > that ACPI of_compatible() matching - acpi_of_match_device() - is > > > part of ACPI and does not depend on CONFIG_OF. > > > > > > Consider the following case: > > > o !CONFIG_OF system > > > o probing of i2c device that provides .of_match_table, but no .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() depends on CONFIG_OF and, thus, is always false. > > > i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching > > > takes place, even though the device provides .of_match_table and ACPI, > > > technically, is capable of matching such device. The result is -ENODEV. > > > Probing will succeed, however, if we'd use .of_match_table aware ACPI > > > matching. Looks like you read same StackOverflow question :-) > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > > > > We have currently this in for-current which is even removing > > i2c_acpi_match_device(): > > > > http://patchwork.ozlabs.org/project/linux-i2c/list/?series=196990&state=* > > Oh, nice! > Can we go a bit further and use i2c_device_match() in i2c_device_probe() > instead of a mix of APIs from different subsystems? > E.g. > > if (!driver->id_table && > - !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && > - !i2c_of_match_device(dev->driver->of_match_table, client)) { > + !(client && i2c_device_match(&client->dev, dev->driver))) { You probably meant simply: if (!i2c_device_match(dev, dev->driver)) { > status = -ENODEV; > goto put_sync_adapter; > } On the first glance it will work the same way but slightly longer in case of ID table matching. Send a patch!
On Wed, Aug 26, 2020 at 12:53:56PM +0300, Andy Shevchenko wrote: > On Wed, Aug 26, 2020 at 02:25:44PM +0900, Sergey Senozhatsky wrote: > > On (20/08/26 07:08), Wolfram Sang wrote: ... > You probably meant simply: > > if (!i2c_device_match(dev, dev->driver)) { > > > status = -ENODEV; > > goto put_sync_adapter; > > } > > On the first glance it will work the same way but slightly longer in case of ID > table matching. > > Send a patch! But then the question is why we have this code in the ->probe() at all? ->match() is run before probe by bus core, no? Wolfram?
On (20/08/26 12:53), Andy Shevchenko wrote: > On Wed, Aug 26, 2020 at 02:25:44PM +0900, Sergey Senozhatsky wrote: > > On (20/08/26 07:08), Wolfram Sang wrote: > > > On Wed, Aug 26, 2020 at 01:29:37PM +0900, Sergey Senozhatsky wrote: [..] > > > > i2c_of_match_device() depends on CONFIG_OF and, thus, is always false. > > > > i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching > > > > takes place, even though the device provides .of_match_table and ACPI, > > > > technically, is capable of matching such device. The result is -ENODEV. > > > > Probing will succeed, however, if we'd use .of_match_table aware ACPI > > > > matching. > > Looks like you read same StackOverflow question :-) Nope :) Ran into actual media/i2c driver probing issue several days ago [..] > > if (!driver->id_table && > > - !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && > > - !i2c_of_match_device(dev->driver->of_match_table, client)) { > > + !(client && i2c_device_match(&client->dev, dev->driver))) { > > You probably meant simply: > > if (!i2c_device_match(dev, dev->driver)) { > > > status = -ENODEV; > > goto put_sync_adapter; > > } That's shorter, yes. I wanted to keep the existing "workaround" in order to avoid extra id_table matching. Because it probably will take place earlier somewhere in bus_for_each_dev() __driver_attach() i2c_device_match() // OF ACPI id_table match > On the first glance it will work the same way but slightly longer in case of ID > table matching. Right. -ss
On (20/08/26 12:56), Andy Shevchenko wrote: > > You probably meant simply: > > > > if (!i2c_device_match(dev, dev->driver)) { > > > > > status = -ENODEV; > > > goto put_sync_adapter; > > > } > > > > On the first glance it will work the same way but slightly longer in case of ID > > table matching. > > > > Send a patch! > > But then the question is why we have this code in the ->probe() at all? > ->match() is run before probe by bus core, no? That's a good question. There is also one more .id_table traversal done right before ->probe() call: driver->probe(client, i2c_match_id(driver->id_table, client)) So in the worst case we can end up doing 3 .id_table lookups. -ss
On (20/08/26 19:24), Sergey Senozhatsky wrote: > > But then the question is why we have this code in the ->probe() at all? > > ->match() is run before probe by bus core, no? > > That's a good question. Everything seem to be working OK on my test board with this patch: --- 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;
On Wed, Aug 26, 2020 at 07:38:07PM +0900, Sergey Senozhatsky wrote: > On (20/08/26 19:24), Sergey Senozhatsky wrote: > > > But then the question is why we have this code in the ->probe() at all? > > > ->match() is run before probe by bus core, no? > > > > That's a good question. > > Everything seem to be working OK on my test board with this patch: I'm okay with it, but I want to hear Wolfram about this. If it gets a green light to go, feel free to add Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > > 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; >
On Wed, Aug 26, 2020 at 01:54:26PM +0300, Andy Shevchenko wrote: > On Wed, Aug 26, 2020 at 07:38:07PM +0900, Sergey Senozhatsky wrote: > > On (20/08/26 19:24), Sergey Senozhatsky wrote: > > > > But then the question is why we have this code in the ->probe() at all? > > > > ->match() is run before probe by bus core, no? > > > > > > That's a good question. > > > > Everything seem to be working OK on my test board with this patch: > > I'm okay with it, but I want to hear Wolfram about this. > If it gets a green light to go, feel free to add > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Sergey, Can you send a proper patch (with patch description) and me and Jean Delvare <jdelvare@suse.de> in the To: field? The origins of this matching code are pretty old and Jean is more experienced there than I am. Nonetheless, I will check it, too, of course. Thanks for the work! > > > --- > > > > 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; > > > > -- > With Best Regards, > Andy Shevchenko > >
Hi Sergey,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v5.9-rc2]
[cannot apply to wsa/i2c/for-next next-20200826]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
base: d012a7190fc1fd72ed48911e77ca97ba4521bccd
config: arm64-randconfig-r002-20200826 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 7cfcecece0e0430937cf529ce74d3a071a4dedc6)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/i2c/i2c-boardinfo.c:13:
>> drivers/i2c/i2c-core.h:68:6: warning: no previous prototype for function 'i2c_acpi_match_device' [-Wmissing-prototypes]
bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
^
drivers/i2c/i2c-core.h:68:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
^
static
1 warning generated.
# https://github.com/0day-ci/linux/commit/8cfc5676303d021ce274c0b608cac342b4aeda55
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
git checkout 8cfc5676303d021ce274c0b608cac342b4aeda55
vim +/i2c_acpi_match_device +68 drivers/i2c/i2c-core.h
64
65 int i2c_acpi_get_irq(struct i2c_client *client);
66 #else /* CONFIG_ACPI */
67 static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
> 68 bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
69 {
70 return false;
71 }
72
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Sergey,
I love your patch! Yet something to improve:
[auto build test ERROR on v5.9-rc2]
[cannot apply to wsa/i2c/for-next next-20200826]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
base: d012a7190fc1fd72ed48911e77ca97ba4521bccd
config: arc-randconfig-r026-20200826 (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arceb-elf-ld: drivers/i2c/i2c-core-smbus.o: in function `i2c_acpi_match_device':
>> drivers/i2c/i2c-core.h:71: multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:drivers/i2c/i2c-core.h:71: first defined here
arceb-elf-ld: drivers/i2c/i2c-core-slave.o: in function `i2c_acpi_match_device':
>> drivers/i2c/i2c-core.h:71: multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:drivers/i2c/i2c-core.h:71: first defined here
arceb-elf-ld: drivers/i2c/i2c-core-of.o: in function `i2c_acpi_match_device':
>> drivers/i2c/i2c-core.h:71: multiple definition of `i2c_acpi_match_device'; drivers/i2c/i2c-core-base.o:drivers/i2c/i2c-core.h:71: first defined here
# https://github.com/0day-ci/linux/commit/8cfc5676303d021ce274c0b608cac342b4aeda55
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sergey-Senozhatsky/i2c-consider-devices-with-of_match_table-during-i2c-device-probing/20200826-123138
git checkout 8cfc5676303d021ce274c0b608cac342b4aeda55
vim +71 drivers/i2c/i2c-core.h
16c9db1dd84cef Charles Keepax 2019-06-27 64
16c9db1dd84cef Charles Keepax 2019-06-27 65 int i2c_acpi_get_irq(struct i2c_client *client);
53f8f7c5cf145d Wolfram Sang 2017-05-23 66 #else /* CONFIG_ACPI */
53f8f7c5cf145d Wolfram Sang 2017-05-23 67 static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
8cfc5676303d02 Sergey Senozhatsky 2020-08-26 68 bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client)
c64ffff7a9d1ee Andy Shevchenko 2017-07-17 69 {
8cfc5676303d02 Sergey Senozhatsky 2020-08-26 70 return false;
c64ffff7a9d1ee Andy Shevchenko 2017-07-17 @71 }
16c9db1dd84cef Charles Keepax 2019-06-27 72
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On (20/08/26 13:23), Wolfram Sang wrote: > On Wed, Aug 26, 2020 at 01:54:26PM +0300, Andy Shevchenko wrote: > > On Wed, Aug 26, 2020 at 07:38:07PM +0900, Sergey Senozhatsky wrote: > > > On (20/08/26 19:24), Sergey Senozhatsky wrote: > > > > > But then the question is why we have this code in the ->probe() at all? > > > > > ->match() is run before probe by bus core, no? > > > > > > > > That's a good question. > > > > > > Everything seem to be working OK on my test board with this patch: > > > > I'm okay with it, but I want to hear Wolfram about this. > > If it gets a green light to go, feel free to add > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Sergey, > > Can you send a proper patch (with patch description) and me and Jean > Delvare <jdelvare@suse.de> in the To: field? > > The origins of this matching code are pretty old and Jean is more > experienced there than I am. Nonetheless, I will check it, too, of > course. Oh, sure, will do. Is that OK if I'll base my patch on linux-next? I'm also going to test the patch on more devices here on my side. -ss
On Wed, Aug 26, 2020 at 11:18:10PM +0900, Sergey Senozhatsky wrote: > On (20/08/26 13:23), Wolfram Sang wrote: > > On Wed, Aug 26, 2020 at 01:54:26PM +0300, Andy Shevchenko wrote: > > > On Wed, Aug 26, 2020 at 07:38:07PM +0900, Sergey Senozhatsky wrote: > > > > On (20/08/26 19:24), Sergey Senozhatsky wrote: > > > > > > But then the question is why we have this code in the ->probe() at all? > > > > > > ->match() is run before probe by bus core, no? > > > > > > > > > > That's a good question. > > > > > > > > Everything seem to be working OK on my test board with this patch: > > > > > > I'm okay with it, but I want to hear Wolfram about this. > > > If it gets a green light to go, feel free to add > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Sergey, > > > > Can you send a proper patch (with patch description) and me and Jean > > Delvare <jdelvare@suse.de> in the To: field? > > > > The origins of this matching code are pretty old and Jean is more > > experienced there than I am. Nonetheless, I will check it, too, of > > course. > > Oh, sure, will do. Is that OK if I'll base my patch on linux-next? > I'm also going to test the patch on more devices here on my side. Today's one includes above mentioned patches, I think it's okay. The i2c/for-next is currently listed ab70935d37bb i2c: Remove 'default n' from busses/Kconfig on top of current, don't see how it may interfere with this.
On (20/08/26 17:34), Andy Shevchenko wrote: > > Oh, sure, will do. Is that OK if I'll base my patch on linux-next? > > I'm also going to test the patch on more devices here on my side. > > Today's one includes above mentioned patches, I think it's okay. Right, just noticed that as well. Thanks. -ss
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c index 2ade99b105b9..1dd152ae75e5 100644 --- a/drivers/i2c/i2c-core-acpi.c +++ b/drivers/i2c/i2c-core-acpi.c @@ -276,14 +276,12 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap) dev_warn(&adap->dev, "failed to enumerate I2C slaves\n"); } -const struct acpi_device_id * -i2c_acpi_match_device(const struct acpi_device_id *matches, - struct i2c_client *client) +bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client) { - if (!(client && matches)) - return NULL; + if (!client) + return false; - return acpi_match_device(matches, &client->dev); + return acpi_driver_match_device(&client->dev, dev->driver); } static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = { diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 34a9609f256d..35ec6335852b 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -480,7 +480,7 @@ static int i2c_device_probe(struct device *dev) * or ACPI ID table is supplied for the probing device. */ if (!driver->id_table && - !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && + !i2c_acpi_match_device(dev, client) && !i2c_of_match_device(dev->driver->of_match_table, client)) { status = -ENODEV; goto put_sync_adapter; diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h index 94ff1693b391..7ee6a6e3fb8d 100644 --- a/drivers/i2c/i2c-core.h +++ b/drivers/i2c/i2c-core.h @@ -59,19 +59,15 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap) } #ifdef CONFIG_ACPI -const struct acpi_device_id * -i2c_acpi_match_device(const struct acpi_device_id *matches, - struct i2c_client *client); +bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client); void i2c_acpi_register_devices(struct i2c_adapter *adap); int i2c_acpi_get_irq(struct i2c_client *client); #else /* CONFIG_ACPI */ static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { } -static inline const struct acpi_device_id * -i2c_acpi_match_device(const struct acpi_device_id *matches, - struct i2c_client *client) +bool i2c_acpi_match_device(struct device *dev, struct i2c_client *client) { - return NULL; + return false; } static inline int i2c_acpi_get_irq(struct i2c_client *client)
Unlike acpi_match_device(), acpi_driver_match_device() does consider devices that provide of_match_table and performs of_compatible() matching for such devices. The key point here is that ACPI of_compatible() matching - acpi_of_match_device() - is part of ACPI and does not depend on CONFIG_OF. Consider the following case: o !CONFIG_OF system o probing of i2c device that provides .of_match_table, but no .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() depends on CONFIG_OF and, thus, is always false. i2c_acpi_match_device() does ACPI match only, no of_comtatible() matching takes place, even though the device provides .of_match_table and ACPI, technically, is capable of matching such device. The result is -ENODEV. Probing will succeed, however, if we'd use .of_match_table aware ACPI matching. Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> --- drivers/i2c/i2c-core-acpi.c | 10 ++++------ drivers/i2c/i2c-core-base.c | 2 +- drivers/i2c/i2c-core.h | 10 +++------- 3 files changed, 8 insertions(+), 14 deletions(-)