On 05.08.2021 12:41, Jean Delvare wrote:
> Hi Heiner,
>
> On Sun, 01 Aug 2021 16:21:08 +0200, Heiner Kallweit wrote:
>> do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE
>> is cleared if a legacy interrupt is used.
>
> Only if pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin) returned a
> non-zero pin, if I read the code correctly. While I can't remember the
> context in which I wrote this piece of code, I suppose that pin == 0
> was the situation where this test was needed. I mean, the board
> designer can legitimately not wire the interrupt pin, and require that
> polling is being used, right?
>
I think we have such a use case, but it's handled in ACPI and results
in dev->irq == IRQ_NOTCONNECTED.
In case of pin == 0 pci_dev->irq is 0, and I'd expect that irq_to_desc(0)
returns NULL and request_threaded_irq() returns -EINVAL. This would
result in switching to polling.
Having said that I see no scenario where the check would be needed.
> In your favor, I can't find any online kernel log with this message.
> However that doesn't mean I'm comfortable removing the safety check.
>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>> drivers/i2c/busses/i2c-i801.c | 9 +--------
>> 1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index a6287c520..5b9eebc1c 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1825,19 +1825,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>> priv->features &= ~FEATURE_IRQ;
>>
>> if (priv->features & FEATURE_IRQ) {
>> - u16 pcictl, pcists;
>> + u16 pcists;
>>
>> /* Complain if an interrupt is already pending */
>> pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
>> if (pcists & PCI_STATUS_INTERRUPT)
>> dev_warn(&dev->dev, "An interrupt is pending!\n");
>> -
>> - /* Check if interrupts have been disabled */
>> - pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pcictl);
>> - if (pcictl & PCI_COMMAND_INTX_DISABLE) {
>> - dev_info(&dev->dev, "Interrupts are disabled\n");
>> - priv->features &= ~FEATURE_IRQ;
>> - }
>> }
>>
>> if (priv->features & FEATURE_IRQ) {
>
>
