| Message ID | 20230514103634.235917-2-mail@mariushoch.de |
|---|---|
| State | Under Review |
| Delegated to: | Wolfram Sang |
| Headers | show |
| Series | i2c: i801: Force no IRQ for Dell Latitude E7450 | expand |
Hi Marius,
kernel test robot noticed the following build warnings:
[auto build test WARNING on cc3c44c9fda264c6d401be04e95449a57c1231c6]
url: https://github.com/intel-lab-lkp/linux/commits/Marius-Hoch/i2c-i801-Force-no-IRQ-for-Dell-Latitude-E7450/20230514-183912
base: cc3c44c9fda264c6d401be04e95449a57c1231c6
patch link: https://lore.kernel.org/r/20230514103634.235917-2-mail%40mariushoch.de
patch subject: [PATCH 1/2] i2c: i801: Force no IRQ for Dell Latitude E7450
config: ia64-buildonly-randconfig-r002-20230514
compiler: ia64-linux-gcc (GCC) 12.1.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
# https://github.com/intel-lab-lkp/linux/commit/4de98f83205987d00c89fd710f3cc85f7b64fc87
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Marius-Hoch/i2c-i801-Force-no-IRQ-for-Dell-Latitude-E7450/20230514-183912
git checkout 4de98f83205987d00c89fd710f3cc85f7b64fc87
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/i2c/busses/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305141951.IaiS1vCr-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-i801.c:1628: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* These DELL devices claim an IRQ for the SMBus (usually 18), but we can't use
vim +1628 drivers/i2c/busses/i2c-i801.c
1626
1627 /**
> 1628 * These DELL devices claim an IRQ for the SMBus (usually 18), but we can't use
1629 * it, as its actually for the I2C accelerometer.
1630 */
1631 static const struct dmi_system_id dmi_force_no_irq[] = {
1632 {
1633 .matches = {
1634 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
1635 DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E7450"),
1636 },
1637 },
1638 {} /* Terminating entry */
1639 };
1640
Hi Marius,
kernel test robot noticed the following build warnings:
[auto build test WARNING on cc3c44c9fda264c6d401be04e95449a57c1231c6]
url: https://github.com/intel-lab-lkp/linux/commits/Marius-Hoch/i2c-i801-Force-no-IRQ-for-Dell-Latitude-E7450/20230514-183912
base: cc3c44c9fda264c6d401be04e95449a57c1231c6
patch link: https://lore.kernel.org/r/20230514103634.235917-2-mail%40mariushoch.de
patch subject: [PATCH 1/2] i2c: i801: Force no IRQ for Dell Latitude E7450
config: riscv-randconfig-r042-20230514
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
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 riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/4de98f83205987d00c89fd710f3cc85f7b64fc87
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Marius-Hoch/i2c-i801-Force-no-IRQ-for-Dell-Latitude-E7450/20230514-183912
git checkout 4de98f83205987d00c89fd710f3cc85f7b64fc87
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/i2c/busses/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305141904.2XlN6bop-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-i801.c:1628: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* These DELL devices claim an IRQ for the SMBus (usually 18), but we can't use
vim +1628 drivers/i2c/busses/i2c-i801.c
1626
1627 /**
> 1628 * These DELL devices claim an IRQ for the SMBus (usually 18), but we can't use
1629 * it, as its actually for the I2C accelerometer.
1630 */
1631 static const struct dmi_system_id dmi_force_no_irq[] = {
1632 {
1633 .matches = {
1634 DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
1635 DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E7450"),
1636 },
1637 },
1638 {} /* Terminating entry */
1639 };
1640
On Sun, 14 May 2023 12:36:33 +0200, Marius Hoch wrote: > The Dell Latitude E7450 uses IRQ 18 for the accelerometer, > but also claims that the SMBus uses IRQ 18. This will > result in: > > i801_smbus 0000:00:1f.3: PCI INT C: failed to register GSI > i801_smbus 0000:00:1f.3: Failed to enable SMBus PCI device (-16) > i801_smbus: probe of 0000:00:1f.3 failed with error -16 > > Force the SMBus IRQ to IRQ_NOTCONNECTED in this case, so that > we fall back to polling, which also seems to be what the (very > dated) Windows 7 drivers on the Dell Latitude E7450 do. > > This was tested on Dell Latitude E7450. > > Signed-off-by: Marius Hoch <mail@mariushoch.de> > --- > drivers/i2c/busses/i2c-i801.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index ac5326747c51..5fd2ac585160 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -1624,6 +1624,20 @@ static void i801_setup_hstcfg(struct i801_priv *priv) > pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg); > } > > +/** As reported by the kernel test robot, please don't start a comment with /** unless it's a kernel-doc-style comment. > + * These DELL devices claim an IRQ for the SMBus (usually 18), but we can't use > + * it, as its actually for the I2C accelerometer. > + */ > +static const struct dmi_system_id dmi_force_no_irq[] = { > + { > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > + DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E7450"), > + }, > + }, > + {} /* Terminating entry */ > +}; > + > static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > { > int err, i; > @@ -1657,6 +1671,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > if (!(priv->features & FEATURE_BLOCK_BUFFER)) > priv->features &= ~FEATURE_BLOCK_PROC; > > + if (dmi_check_system(dmi_force_no_irq)) { If the problem is caused by dev->irq being 255, and now that we know that this value is special on x86, wouldn't it make more sense to restrict this quirk to CONFIG_X86 and simply check for dev->irq == 0xff? This would save us the extra effort of maintaining a list of machines which need the quirk. > + /* Force no IRQ for these devices, otherwise pcim_enable_device will fail */ > + dev->irq = IRQ_NOTCONNECTED; > + dev->irq_managed = 1; This field is undocumented so I have no idea what it does. Is it not sufficient to set irq to IRQ_NOTCONNECTED? > + } > + > err = pcim_enable_device(dev); > if (err) { > dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
Hi Jean, On 04/06/2023 16:38, Jean Delvare wrote: > On Sun, 14 May 2023 12:36:33 +0200, Marius Hoch wrote: >> The Dell Latitude E7450 uses IRQ 18 for the accelerometer, >> but also claims that the SMBus uses IRQ 18. This will >> result in: >> >> i801_smbus 0000:00:1f.3: PCI INT C: failed to register GSI >> i801_smbus 0000:00:1f.3: Failed to enable SMBus PCI device (-16) >> i801_smbus: probe of 0000:00:1f.3 failed with error -16 >> >> Force the SMBus IRQ to IRQ_NOTCONNECTED in this case, so that >> we fall back to polling, which also seems to be what the (very >> dated) Windows 7 drivers on the Dell Latitude E7450 do. >> >> This was tested on Dell Latitude E7450. >> >> Signed-off-by: Marius Hoch <mail@mariushoch.de> >> --- >> drivers/i2c/busses/i2c-i801.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >> index ac5326747c51..5fd2ac585160 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -1624,6 +1624,20 @@ static void i801_setup_hstcfg(struct i801_priv *priv) >> pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg); >> } >> >> +/** > As reported by the kernel test robot, please don't start a comment with > /** unless it's a kernel-doc-style comment. > >> + * These DELL devices claim an IRQ for the SMBus (usually 18), but we can't use >> + * it, as its actually for the I2C accelerometer. >> + */ >> +static const struct dmi_system_id dmi_force_no_irq[] = { >> + { >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >> + DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E7450"), >> + }, >> + }, >> + {} /* Terminating entry */ >> +}; >> + >> static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) >> { >> int err, i; >> @@ -1657,6 +1671,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) >> if (!(priv->features & FEATURE_BLOCK_BUFFER)) >> priv->features &= ~FEATURE_BLOCK_PROC; >> >> + if (dmi_check_system(dmi_force_no_irq)) { > If the problem is caused by dev->irq being 255, and now that we know > that this value is special on x86, wouldn't it make more sense to > restrict this quirk to CONFIG_X86 and simply check for dev->irq == > 0xff? This would save us the extra effort of maintaining a list of > machines which need the quirk. > >> + /* Force no IRQ for these devices, otherwise pcim_enable_device will fail */ >> + dev->irq = IRQ_NOTCONNECTED; >> + dev->irq_managed = 1; > This field is undocumented so I have no idea what it does. Is it not > sufficient to set irq to IRQ_NOTCONNECTED? If irq_managed is not set our irq value will be entirely ignored it seems (thus leading to the same code path/ failure initially outlined). > >> + } >> + >> err = pcim_enable_device(dev); >> if (err) { >> dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n", >
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index ac5326747c51..5fd2ac585160 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1624,6 +1624,20 @@ static void i801_setup_hstcfg(struct i801_priv *priv) pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg); } +/** + * These DELL devices claim an IRQ for the SMBus (usually 18), but we can't use + * it, as its actually for the I2C accelerometer. + */ +static const struct dmi_system_id dmi_force_no_irq[] = { + { + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), + DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E7450"), + }, + }, + {} /* Terminating entry */ +}; + static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) { int err, i; @@ -1657,6 +1671,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) if (!(priv->features & FEATURE_BLOCK_BUFFER)) priv->features &= ~FEATURE_BLOCK_PROC; + if (dmi_check_system(dmi_force_no_irq)) { + /* Force no IRQ for these devices, otherwise pcim_enable_device will fail */ + dev->irq = IRQ_NOTCONNECTED; + dev->irq_managed = 1; + } + err = pcim_enable_device(dev); if (err) { dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
The Dell Latitude E7450 uses IRQ 18 for the accelerometer, but also claims that the SMBus uses IRQ 18. This will result in: i801_smbus 0000:00:1f.3: PCI INT C: failed to register GSI i801_smbus 0000:00:1f.3: Failed to enable SMBus PCI device (-16) i801_smbus: probe of 0000:00:1f.3 failed with error -16 Force the SMBus IRQ to IRQ_NOTCONNECTED in this case, so that we fall back to polling, which also seems to be what the (very dated) Windows 7 drivers on the Dell Latitude E7450 do. This was tested on Dell Latitude E7450. Signed-off-by: Marius Hoch <mail@mariushoch.de> --- drivers/i2c/busses/i2c-i801.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)