| Message ID | 20141110223139.02aafde4@endymion.delvare |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Nov 10, 2014 at 10:31:39PM +0100, Jean Delvare wrote: > There is a control bit in the PCI configuration space which disables > interrupts. If this bit is set, the driver should not try to make use > of interrupts, it won't receive any. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Cc: Wolfram Sang <wsa@the-dreams.de> > --- > drivers/i2c/busses/i2c-i801.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > --- linux-3.18-rc4.orig/drivers/i2c/busses/i2c-i801.c 2014-11-10 22:29:44.416991298 +0100 > +++ linux-3.18-rc4/drivers/i2c/busses/i2c-i801.c 2014-11-10 22:29:46.915045662 +0100 > @@ -110,12 +110,16 @@ > > /* PCI Address Constants */ > #define SMBBAR 4 > +#define SMBPCICTL 0x004 > #define SMBPCISTS 0x006 > #define SMBHSTCFG 0x040 > > /* Host status bits for SMBPCISTS */ > #define SMBPCISTS_INTS 0x08 > > +/* Control bits for SMBPCICTL */ > +#define SMBPCICTL_INTDIS 0x0400 > + > /* Host configuration bits for SMBHSTCFG */ > #define SMBHSTCFG_HST_EN 1 > #define SMBHSTCFG_SMB_SMI_EN 2 > @@ -1235,6 +1239,23 @@ static int i801_probe(struct pci_dev *de > priv->adapter.timeout = HZ / 5; > > if (priv->features & FEATURE_IRQ) { > + u16 pcictl, pcists; > + > + /* Complain if an interrupt is already pending */ > + pci_read_config_word(priv->pci_dev, SMBPCISTS, &pcists); > + if (pcists & SMBPCISTS_INTS) > + dev_warn(&dev->dev, "An interrupt is pending!\n"); You think it is better to not clear it? > + > + /* Check if interrupts have been disabled */ > + pci_read_config_word(priv->pci_dev, SMBPCICTL, &pcictl); > + if (pcictl & SMBPCICTL_INTDIS) { > + dev_warn(&dev->dev, > + "Interrupts are disabled, will use polling\n"); I'd say it is more info than warning? > + priv->features &= ~FEATURE_IRQ; > + } > + } > + > + if (priv->features & FEATURE_IRQ) { > init_waitqueue_head(&priv->waitq); > > err = request_irq(dev->irq, i801_isr, IRQF_SHARED, > > -- > Jean Delvare > SUSE L3 Support
Hi Wolfram, On Tue, 11 Nov 2014 11:58:54 +0100, Wolfram Sang wrote: > On Mon, Nov 10, 2014 at 10:31:39PM +0100, Jean Delvare wrote: > > There is a control bit in the PCI configuration space which disables > > interrupts. If this bit is set, the driver should not try to make use > > of interrupts, it won't receive any. > > > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > --- > > drivers/i2c/busses/i2c-i801.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > --- linux-3.18-rc4.orig/drivers/i2c/busses/i2c-i801.c 2014-11-10 22:29:44.416991298 +0100 > > +++ linux-3.18-rc4/drivers/i2c/busses/i2c-i801.c 2014-11-10 22:29:46.915045662 +0100 > > @@ -110,12 +110,16 @@ > > > > /* PCI Address Constants */ > > #define SMBBAR 4 > > +#define SMBPCICTL 0x004 > > #define SMBPCISTS 0x006 > > #define SMBHSTCFG 0x040 > > > > /* Host status bits for SMBPCISTS */ > > #define SMBPCISTS_INTS 0x08 > > > > +/* Control bits for SMBPCICTL */ > > +#define SMBPCICTL_INTDIS 0x0400 > > + > > /* Host configuration bits for SMBHSTCFG */ > > #define SMBHSTCFG_HST_EN 1 > > #define SMBHSTCFG_SMB_SMI_EN 2 > > @@ -1235,6 +1239,23 @@ static int i801_probe(struct pci_dev *de > > priv->adapter.timeout = HZ / 5; > > > > if (priv->features & FEATURE_IRQ) { > > + u16 pcictl, pcists; > > + > > + /* Complain if an interrupt is already pending */ > > + pci_read_config_word(priv->pci_dev, SMBPCISTS, &pcists); > > + if (pcists & SMBPCISTS_INTS) > > + dev_warn(&dev->dev, "An interrupt is pending!\n"); > > You think it is better to not clear it? I admit I did not think about it. As I am trying to better understand the few mysterious failure cases that have been reported to me, I just wanted to log everything out of the ordinary. I have no idea what's considered the right thing to do in such a situation. Do you believe that clearing the interrupt is the appropriate action in that case? > > + > > + /* Check if interrupts have been disabled */ > > + pci_read_config_word(priv->pci_dev, SMBPCICTL, &pcictl); > > + if (pcictl & SMBPCICTL_INTDIS) { > > + dev_warn(&dev->dev, > > + "Interrupts are disabled, will use polling\n"); > > I'd say it is more info than warning? Correct, I'll change it. > > + priv->features &= ~FEATURE_IRQ; > > + } > > + } > > + > > + if (priv->features & FEATURE_IRQ) { > > init_waitqueue_head(&priv->waitq); > > > > err = request_irq(dev->irq, i801_isr, IRQF_SHARED, Thanks again,
> > > + if (pcists & SMBPCISTS_INTS) > > > + dev_warn(&dev->dev, "An interrupt is pending!\n"); > > > > You think it is better to not clear it? > > I admit I did not think about it. As I am trying to better understand > the few mysterious failure cases that have been reported to me, I just > wanted to log everything out of the ordinary. I have no idea what's > considered the right thing to do in such a situation. Do you believe > that clearing the interrupt is the appropriate action in that case? Depends on the driver, I'd say (which I haven't looked at in detail). If this causes the irq handler to be called as soon as the irq is registered, and if the state machine gets confused then, then it should be cleared beforehand, of course. If this is not the case, then it might be better to be less intrusive, spit the warning, and wait for somebody to show up with this message in the logs.
On Tue, 11 Nov 2014 15:41:07 +0100, Wolfram Sang wrote: > > > > > + if (pcists & SMBPCISTS_INTS) > > > > + dev_warn(&dev->dev, "An interrupt is pending!\n"); > > > > > > You think it is better to not clear it? > > > > I admit I did not think about it. As I am trying to better understand > > the few mysterious failure cases that have been reported to me, I just > > wanted to log everything out of the ordinary. I have no idea what's > > considered the right thing to do in such a situation. Do you believe > > that clearing the interrupt is the appropriate action in that case? > > Depends on the driver, I'd say (which I haven't looked at in detail). If > this causes the irq handler to be called as soon as the irq is > registered, and if the state machine gets confused then, then it should > be cleared beforehand, of course. The driver should survive just fine if the irq handler is called early, but lacking the context, it won't do anything useful. Essentially it will clear the interrupt and wake up an empty queue. My actual concern is that I don't know what that would mean if an interrupt is pending at driver load time. If this is a leftover from a previous driver removal, or from the machine initialization (BIOS) then clearing it would be the right thing to do. But this could also mean that something (e.g. ACPI) is using the hardware and we should not. That being said, hitting the very moment where an interrupt is pending would be pretty random, so we can hardly rely on that anyway. Or this could mean that the interrupt setup is wrong somehow and we can't trust the SMBPCISTS_INTS because it is always set. At this point I just don't know. Another thing I am wondering about, and thinking about it again (I wrote this code several months ago) this is probably the reason why I added the test in the first place: can a new interrupt be triggered as long as the previous one has not been cleared? If not, that could explain why the driver sometimes didn't work at all in interrupt mode on some systems. > If this is not the case, then it might be better to be less intrusive, > spit the warning, and wait for somebody to show up with this message in > the logs. That was my intent, yes. We can always add an action later when we understand the situation better. If we ever get such a report, which might in fact never happen. Thanks,
> > If this is not the case, then it might be better to be less intrusive, > > spit the warning, and wait for somebody to show up with this message in > > the logs. > > That was my intent, yes. We can always add an action later when we > understand the situation better. If we ever get such a report, which > might in fact never happen. Let's do it then!
--- linux-3.18-rc4.orig/drivers/i2c/busses/i2c-i801.c 2014-11-10 22:29:44.416991298 +0100 +++ linux-3.18-rc4/drivers/i2c/busses/i2c-i801.c 2014-11-10 22:29:46.915045662 +0100 @@ -110,12 +110,16 @@ /* PCI Address Constants */ #define SMBBAR 4 +#define SMBPCICTL 0x004 #define SMBPCISTS 0x006 #define SMBHSTCFG 0x040 /* Host status bits for SMBPCISTS */ #define SMBPCISTS_INTS 0x08 +/* Control bits for SMBPCICTL */ +#define SMBPCICTL_INTDIS 0x0400 + /* Host configuration bits for SMBHSTCFG */ #define SMBHSTCFG_HST_EN 1 #define SMBHSTCFG_SMB_SMI_EN 2 @@ -1235,6 +1239,23 @@ static int i801_probe(struct pci_dev *de priv->adapter.timeout = HZ / 5; if (priv->features & FEATURE_IRQ) { + u16 pcictl, pcists; + + /* Complain if an interrupt is already pending */ + pci_read_config_word(priv->pci_dev, SMBPCISTS, &pcists); + if (pcists & SMBPCISTS_INTS) + dev_warn(&dev->dev, "An interrupt is pending!\n"); + + /* Check if interrupts have been disabled */ + pci_read_config_word(priv->pci_dev, SMBPCICTL, &pcictl); + if (pcictl & SMBPCICTL_INTDIS) { + dev_warn(&dev->dev, + "Interrupts are disabled, will use polling\n"); + priv->features &= ~FEATURE_IRQ; + } + } + + if (priv->features & FEATURE_IRQ) { init_waitqueue_head(&priv->waitq); err = request_irq(dev->irq, i801_isr, IRQF_SHARED,
There is a control bit in the PCI configuration space which disables interrupts. If this bit is set, the driver should not try to make use of interrupts, it won't receive any. Signed-off-by: Jean Delvare <jdelvare@suse.de> Cc: Wolfram Sang <wsa@the-dreams.de> --- drivers/i2c/busses/i2c-i801.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)