| Message ID | 20171122112817.9554-1-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: i801: Fix Failed to allocate irq -2147483648 error | expand |
Hi Hans, On Wed, 22 Nov 2017 12:28:17 +0100, Hans de Goede wrote: > On Apollo Lake devices the BIOS does not set up IRQ routing for the i801 > SMBUS controller IRQ, so we end up with dev->irq set to IRQ_NOTCONNECTED. > > Detect this and do not try to use the irq in this case silencing: > i801_smbus 0000:00:1f.1: Failed to allocate irq -2147483648: -107 Isn't it a BIOS bug? > Cc: stable@vger.kernel.org > BugLink: https://communities.intel.com/thread/114759 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/i2c/busses/i2c-i801.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 9e12a53ef7b8..8eac00efadc1 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -1617,6 +1617,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > /* Default timeout in interrupt mode: 200 ms */ > priv->adapter.timeout = HZ / 5; > > + if (dev->irq == IRQ_NOTCONNECTED) > + priv->features &= ~FEATURE_IRQ; > + > if (priv->features & FEATURE_IRQ) { > u16 pcictl, pcists; Certainly better than returning a cryptic error message, so: Reviewed-by: Jean Delvare <jdelvare@suse.de> However if this is considered a BIOS bug then a pr_err(FW_BUG...) would be good to add IMHO.
Hi, On 11/24/2017 02:49 PM, Jean Delvare wrote: > Hi Hans, > > On Wed, 22 Nov 2017 12:28:17 +0100, Hans de Goede wrote: >> On Apollo Lake devices the BIOS does not set up IRQ routing for the i801 >> SMBUS controller IRQ, so we end up with dev->irq set to IRQ_NOTCONNECTED. >> >> Detect this and do not try to use the irq in this case silencing: >> i801_smbus 0000:00:1f.1: Failed to allocate irq -2147483648: -107 > > Isn't it a BIOS bug? No, as mentioned in: >> BugLink: https://communities.intel.com/thread/114759 The BIOS deliberately does not contain IRQ routing issues because some variants of the Windows driver for this falling over if their is any IRQ routing set, so this is a "feature" not a bug :| >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/i2c/busses/i2c-i801.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >> index 9e12a53ef7b8..8eac00efadc1 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -1617,6 +1617,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) >> /* Default timeout in interrupt mode: 200 ms */ >> priv->adapter.timeout = HZ / 5; >> >> + if (dev->irq == IRQ_NOTCONNECTED) >> + priv->features &= ~FEATURE_IRQ; >> + >> if (priv->features & FEATURE_IRQ) { >> u16 pcictl, pcists; > > Certainly better than returning a cryptic error message, so: > > Reviewed-by: Jean Delvare <jdelvare@suse.de> Thanks. > However if this is considered a BIOS bug then a pr_err(FW_BUG...) would > be good to add IMHO. See above, that would just replace one error message with another on all Apollo Lake systems. Regards, Hans
On Sat, 25 Nov 2017 14:43:08 +0100, Hans de Goede wrote: > On 11/24/2017 02:49 PM, Jean Delvare wrote: > > Isn't it a BIOS bug? > > No, as mentioned in: > > >> BugLink: https://communities.intel.com/thread/114759 > > The BIOS deliberately does not contain IRQ routing issues because some > variants of the Windows driver for this falling over if their is > any IRQ routing set, so this is a "feature" not a bug :| What I do not understand is how Apollo Lake systems are different from any other Intel system, which already had a compatible SMBus controller and apparently having an IRQ configured was never an issue? > > (...) > > However if this is considered a BIOS bug then a pr_err(FW_BUG...) would > > be good to add IMHO. > > See above, that would just replace one error message with another on all > Apollo Lake systems. Turning a cryptic error message into a meaningful error message isn't necessarily a bad thing.
Hi, On 27-11-17 11:04, Jean Delvare wrote: > On Sat, 25 Nov 2017 14:43:08 +0100, Hans de Goede wrote: >> On 11/24/2017 02:49 PM, Jean Delvare wrote: >>> Isn't it a BIOS bug? >> >> No, as mentioned in: >> >>>> BugLink: https://communities.intel.com/thread/114759 >> >> The BIOS deliberately does not contain IRQ routing issues because some >> variants of the Windows driver for this falling over if their is >> any IRQ routing set, so this is a "feature" not a bug :| > > What I do not understand is how Apollo Lake systems are different from > any other Intel system, which already had a compatible SMBus controller > and apparently having an IRQ configured was never an issue? I've no answer there I'm afraid. >>> (...) >>> However if this is considered a BIOS bug then a pr_err(FW_BUG...) would >>> be good to add IMHO. >> >> See above, that would just replace one error message with another on all >> Apollo Lake systems. > > Turning a cryptic error message into a meaningful error message isn't > necessarily a bad thing. Given all the effort distros have done with splash-screens to give users a nice clean boot experience, we really want dmesg --level=err to not print anything unless there is a real problem with either the hardware or the kernel, printing an error message on all Apollo Lake systems is really not helpful IMHO. Regards, Hans
On Wed, Nov 22, 2017 at 12:28:17PM +0100, Hans de Goede wrote: > On Apollo Lake devices the BIOS does not set up IRQ routing for the i801 > SMBUS controller IRQ, so we end up with dev->irq set to IRQ_NOTCONNECTED. > > Detect this and do not try to use the irq in this case silencing: > i801_smbus 0000:00:1f.1: Failed to allocate irq -2147483648: -107 > > Cc: stable@vger.kernel.org > BugLink: https://communities.intel.com/thread/114759 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Applied to for-current, thanks!
On Mon, 27 Nov 2017 16:38:50 +0100, Hans de Goede wrote: > On 27-11-17 11:04, Jean Delvare wrote: > > Turning a cryptic error message into a meaningful error message isn't > > necessarily a bad thing. > > Given all the effort distros have done with splash-screens to give > users a nice clean boot experience, we really want dmesg --level=err > to not print anything unless there is a real problem with either the > hardware or the kernel, I completely agree with that, same goal here. > printing an error message on all Apollo Lake > systems is really not helpful IMHO. It wouldn't have to be an error message, it could be a mere notice. What I'm afraid of is that the missing IRQ is an overlook from the standard BIOS provided by Intel to integrators, and the error will keep spreading until BIOS developers are aware of the problem.
Hi, On 28-11-17 14:37, Jean Delvare wrote: > On Mon, 27 Nov 2017 16:38:50 +0100, Hans de Goede wrote: >> On 27-11-17 11:04, Jean Delvare wrote: >>> Turning a cryptic error message into a meaningful error message isn't >>> necessarily a bad thing. >> >> Given all the effort distros have done with splash-screens to give >> users a nice clean boot experience, we really want dmesg --level=err >> to not print anything unless there is a real problem with either the >> hardware or the kernel, > > I completely agree with that, same goal here. > >> printing an error message on all Apollo Lake >> systems is really not helpful IMHO. > > It wouldn't have to be an error message, it could be a mere notice. > > What I'm afraid of is that the missing IRQ is an overlook from the > standard BIOS provided by Intel to integrators, and the error will keep > spreading until BIOS developers are aware of the problem. According to: https://communities.intel.com/thread/114759 it was not an oversight, but done deliberately: "BTW, the SMBus interface was removed from the mapping table because, in the Windows environment (where Intel does not provide a driver for general use), there were all sorts of 3rd-party drivers that are screwed up by the interrupt routing. It was easier to remove it from the mapping than to rein in all of these drivers (which mostly seemed to want to use it in polled mode anyway). As you saw in the log, the i801_i2c driver just switched to polled mode and kept on trucking..." I think the clue is "Intel does not provide a driver for general use" and the 3th party drivers have varying levels of quality, I guess there is something special about the IRQ routing on Apollo Lake that this was not an issue before. Regards, Hans
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 9e12a53ef7b8..8eac00efadc1 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1617,6 +1617,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) /* Default timeout in interrupt mode: 200 ms */ priv->adapter.timeout = HZ / 5; + if (dev->irq == IRQ_NOTCONNECTED) + priv->features &= ~FEATURE_IRQ; + if (priv->features & FEATURE_IRQ) { u16 pcictl, pcists;
On Apollo Lake devices the BIOS does not set up IRQ routing for the i801 SMBUS controller IRQ, so we end up with dev->irq set to IRQ_NOTCONNECTED. Detect this and do not try to use the irq in this case silencing: i801_smbus 0000:00:1f.1: Failed to allocate irq -2147483648: -107 Cc: stable@vger.kernel.org BugLink: https://communities.intel.com/thread/114759 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/busses/i2c-i801.c | 3 +++ 1 file changed, 3 insertions(+)