Re: [PATCH 06/10] i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE

[Jean Delvare <jdelvare@suse.de>]

On Thu, 5 Aug 2021 22:04:18 +0200, Heiner Kallweit wrote:
> On 05.08.2021 12:41, Jean Delvare wrote:
> > 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.

But not all systems use ACPI. The i2c-i801 driver could be used on
non-ACPI systems. I don't know if this is actually the case though. But
we definitely allow building kernels with ACPI disabled and I2C_I801
enabled.

> 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.

Reading the !CONFIG_SPARSE_IRQ version of that function, it doesn't
seem so. irq_to_desc(0) would return &irq_desc[0]. IRQ 0 is not
invalid, it was the system clock on legacy PC systems, and probably
still is for compatibility reasons. I suppose the CONFIG_SPARSE_IRQ
version of irq_to_desc() is compatible with that too.

That being said, I suppose IRQ 0 is requested early at boot, so the
i2c-i801 driver would get -EBUSY or similar when trying to request it,
which in turn would result in falling back to polling mode, which is
what we want.

> 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.

I'm still uncertain about what to do here. On the one hand, the check
can't hurt, and if we hit a corner case, could provide useful debugging
information. On the other hand, it may be dead code if you are correct,
and I don't like dead code.

I suppose we could remove the code for now, and see if anyone reports a
regression.

-- 
Jean Delvare
SUSE L3 Support