[PATCH] i2c: i801: Fix Failed to allocate irq -2147483648 error

[PATCH] i2c: i801: Fix Failed to allocate irq -2147483648 error

[Hans de Goede]

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(+)

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;
 
-- 
2.14.3

Re: [PATCH] i2c: i801: Fix Failed to allocate irq -2147483648 error

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Fix Failed to allocate irq -2147483648 error

[Hans de Goede]

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

Re: [PATCH] i2c: i801: Fix Failed to allocate irq -2147483648 error

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Fix Failed to allocate irq -2147483648 error

[Hans de Goede]

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

Re: [PATCH] i2c: i801: Fix Failed to allocate irq -2147483648 error

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Fix Failed to allocate irq -2147483648 error

[Hans de Goede]

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

Re: [PATCH] i2c: i801: Fix Failed to allocate irq -2147483648 error

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]