| Message ID | f9115eb4-4b19-0a9c-0354-b3ded261c155@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: Series with improvements | expand |
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? 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@gmail.com> > --- > 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) {
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@gmail.com> >> --- >> 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) { > >
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.
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) {
do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE is cleared if a legacy interrupt is used. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)