| Message ID | 1457715962-108484-2-git-send-email-qipeng.zha@intel.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Mar 12, 2016 at 01:06:01AM +0800, Qipeng Zha wrote: > There is one unexpected GPIO interrupt coming in below scenario. > 1. GPIO X is going to be used as falling edge interrupt. > 2. Before request_irq call, this GPIO X interrupt was masked. > 3. But the IRQ mode may be set for some mode in default (by BIOS). > 4. Toggle GPIO X from high to low. > 5. The GPIO X interrupt status will be set even if it was masked. > 6. Register the interrupt for GPIO X, the interrupt will be unmasked. > 7. Even if no change on GPIO X afterwards, but one GPIO X interrupt > will be triggered because the interrupt status was set. > > To avoid above issue, the interrupt status need clear before request_irq. > > Signed-off-by: Qi Zheng <qi.zheng@intel.com> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > index ded5378..d6fe659 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type) > return -EPERM; > } > > + intel_gpio_irq_ack(d); If the pin toggles right here, we still have the same issue, no? We should check in the interrupt handler whether the interrupt is actually enabled which I think we do already. Maybe that code has bug somewhere? > + > spin_lock_irqsave(&pctrl->lock, flags); > > value = readl(reg); > -- > 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Mar 12, 2016 at 01:06:01AM +0800, Qipeng Zha wrote: > There is one unexpected GPIO interrupt coming in below scenario. > 1. GPIO X is going to be used as falling edge interrupt. > 2. Before request_irq call, this GPIO X interrupt was masked. > 3. But the IRQ mode may be set for some mode in default (by BIOS). > 4. Toggle GPIO X from high to low. > 5. The GPIO X interrupt status will be set even if it was masked. > 6. Register the interrupt for GPIO X, the interrupt will be unmasked. > 7. Even if no change on GPIO X afterwards, but one GPIO X interrupt > will be triggered because the interrupt status was set. > > To avoid above issue, the interrupt status need clear before request_irq. > > Signed-off-by: Qi Zheng <qi.zheng@intel.com> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c > b/drivers/pinctrl/intel/pinctrl-intel.c > index ded5378..d6fe659 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type) > return -EPERM; > } > > + intel_gpio_irq_ack(d); > If the pin toggles right here, we still have the same issue, no? Yes. But it is very short time from here to the place IRQ got enabled. To me, it is the only available platform dependent interface to do the "ACK" in the flow of request_irq. Maybe you have other better option here? > We should check in the interrupt handler whether the interrupt is actually enabled which I think we do already. Maybe that code has bug somewhere? The check in the interrupt handler can't distinguish if the interrupt status was set before or after the request_irq. On the Broxton RVP, one unexpected GPIO interrupt was captured during our GPIO unit test program. This issue can be fixed by this patch. > + > spin_lock_irqsave(&pctrl->lock, flags); > > value = readl(reg); > -- > 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote: > > On Sat, Mar 12, 2016 at 01:06:01AM +0800, Qipeng Zha wrote: > > There is one unexpected GPIO interrupt coming in below scenario. > > 1. GPIO X is going to be used as falling edge interrupt. > > 2. Before request_irq call, this GPIO X interrupt was masked. > > 3. But the IRQ mode may be set for some mode in default (by BIOS). > > 4. Toggle GPIO X from high to low. > > 5. The GPIO X interrupt status will be set even if it was masked. > > 6. Register the interrupt for GPIO X, the interrupt will be unmasked. > > 7. Even if no change on GPIO X afterwards, but one GPIO X interrupt > > will be triggered because the interrupt status was set. > > > > To avoid above issue, the interrupt status need clear before request_irq. > > > > Signed-off-by: Qi Zheng <qi.zheng@intel.com> > > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com> > > --- > > drivers/pinctrl/intel/pinctrl-intel.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c > > b/drivers/pinctrl/intel/pinctrl-intel.c > > index ded5378..d6fe659 100644 > > --- a/drivers/pinctrl/intel/pinctrl-intel.c > > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > > @@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type) > > return -EPERM; > > } > > > > + intel_gpio_irq_ack(d); > > > If the pin toggles right here, we still have the same issue, no? > > Yes. But it is very short time from here to the place IRQ got enabled. That is still a race. > To me, it is the only available platform dependent interface to do the "ACK" in the flow of request_irq. > Maybe you have other better option here? > > > We should check in the interrupt handler whether the interrupt is actually enabled which I think we do already. Maybe that code has bug somewhere? > > The check in the interrupt handler can't distinguish if the interrupt status was set before or after the request_irq. > On the Broxton RVP, one unexpected GPIO interrupt was captured during our GPIO unit test program. > This issue can be fixed by this patch. Drivers need to deal with the fact that they might get spurious interrupts from time to time. You need to check in the interrupt handler if the interrupt was from the device you are driving or not. request_irq() enables the interrupt line and if the pin is already in a state that triggers an interrupt, the driver interrupt handler will be called. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote: > > On Sat, Mar 12, 2016 at 01:06:01AM +0800, Qipeng Zha wrote: > > There is one unexpected GPIO interrupt coming in below scenario. > > 1. GPIO X is going to be used as falling edge interrupt. > > 2. Before request_irq call, this GPIO X interrupt was masked. > > 3. But the IRQ mode may be set for some mode in default (by BIOS). > > 4. Toggle GPIO X from high to low. > > 5. The GPIO X interrupt status will be set even if it was masked. > > 6. Register the interrupt for GPIO X, the interrupt will be unmasked. > > 7. Even if no change on GPIO X afterwards, but one GPIO X interrupt > > will be triggered because the interrupt status was set. > > > > To avoid above issue, the interrupt status need clear before request_irq. > > > > Signed-off-by: Qi Zheng <qi.zheng@intel.com> > > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com> > > --- > > drivers/pinctrl/intel/pinctrl-intel.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c > > b/drivers/pinctrl/intel/pinctrl-intel.c > > index ded5378..d6fe659 100644 > > --- a/drivers/pinctrl/intel/pinctrl-intel.c > > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > > @@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type) > > return -EPERM; > > } > > > > + intel_gpio_irq_ack(d); > > > If the pin toggles right here, we still have the same issue, no? > > Yes. But it is very short time from here to the place IRQ got enabled. >That is still a race. > To me, it is the only available platform dependent interface to do the "ACK" in the flow of request_irq. > Maybe you have other better option here? > > > We should check in the interrupt handler whether the interrupt is actually enabled which I think we do already. Maybe that code has bug somewhere? > > The check in the interrupt handler can't distinguish if the interrupt status was set before or after the request_irq. > On the Broxton RVP, one unexpected GPIO interrupt was captured during our GPIO unit test program. > This issue can be fixed by this patch. > Drivers need to deal with the fact that they might get spurious interrupts from time to time. You need to check in the interrupt handler if the interrupt was from the device you are driving or not. > request_irq() enables the interrupt line and if the pin is already in a state that triggers an interrupt, the driver interrupt handler will be called. I agree with you that the driver need deal with spurious interrupts, but it is expected that the GPIO driver eliminate this spurious interrupts as well which the patch can help. And IMHO, adding ACK in the irq_set_type makes sense since a clean interrupt status is expected behavior before enabling the interrupt. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 11:02:04AM +0200, Zheng, Qi wrote: > I agree with you that the driver need deal with spurious interrupts, > but it is expected that the GPIO driver eliminate this spurious > interrupts as well which the patch can help. And IMHO, adding ACK in > the irq_set_type makes sense since a clean interrupt status is > expected behavior before enabling the interrupt. But it does not solve anything if the pin status changes right after you call ack. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 9:44 AM, Westerberg, Mika <mika.westerberg@intel.com> wrote: > On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote: >> > + intel_gpio_irq_ack(d); >> >> > If the pin toggles right here, we still have the same issue, no? >> >> Yes. But it is very short time from here to the place IRQ got enabled. > > That is still a race. > >> To me, it is the only available platform dependent interface to do > the "ACK" in the flow of request_irq. Maybe you have other better > option here? (...) > Drivers need to deal with the fact that they might get spurious > interrupts from time to time. You need to check in the interrupt handler > if the interrupt was from the device you are driving or not. > > request_irq() enables the interrupt line and if the pin is already in > a state that triggers an interrupt, the driver interrupt handler will > be called. This looks like something is wrong in the irqchip. Your set_type() is supporting edges but have all IRQs handled by handle_simple_irq() rather than handle_edge_irq() for the edges, which gives a more robust control flow from IRQ to ACK to calling the handler. Zheng/Mika: please look at how the level/edge IRQs are handled in drivers/gpio/gpio-pl061.c where I *tried* to do things right, switching handler in .set_type() using irq_set_handler_locked(). I think you may need to use handle_edge_irq() for the edge IRQs and handle_level_irq() for the level IRQs just like I do in the PL061 driver. (I might be wrong.) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 01:40:39PM +0100, Linus Walleij wrote: > On Mon, Mar 14, 2016 at 9:44 AM, Westerberg, Mika > <mika.westerberg@intel.com> wrote: > > On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote: > > >> > + intel_gpio_irq_ack(d); > >> > >> > If the pin toggles right here, we still have the same issue, no? > >> > >> Yes. But it is very short time from here to the place IRQ got enabled. > > > > That is still a race. > > > >> To me, it is the only available platform dependent interface to do > > the "ACK" in the flow of request_irq. Maybe you have other better > > option here? > (...) > > Drivers need to deal with the fact that they might get spurious > > interrupts from time to time. You need to check in the interrupt handler > > if the interrupt was from the device you are driving or not. > > > > request_irq() enables the interrupt line and if the pin is already in > > a state that triggers an interrupt, the driver interrupt handler will > > be called. > > This looks like something is wrong in the irqchip. If request_irq() is called so that the interrupt is level triggered, let's say active low, and the pin is already in that state I would expect interrupt to trigger immediately when enabled, no? If I understand correctly this is precisely what Zheng is describing they are trying to solve. > Your set_type() is supporting edges but have all IRQs > handled by handle_simple_irq() rather than handle_edge_irq() > for the edges, which gives a more robust control flow > from IRQ to ACK to calling the handler. > > Zheng/Mika: please look at how the level/edge > IRQs are handled in drivers/gpio/gpio-pl061.c > where I *tried* to do things right, switching handler > in .set_type() using irq_set_handler_locked(). I think > you may need to use handle_edge_irq() for the edge IRQs > and handle_level_irq() for the level IRQs just like I do > in the PL061 driver. The driver is already doing that as far as I can tell (see intel_gpio_irq_type()). -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 02:54:36PM +0200, Westerberg, Mika wrote: > On Mon, Mar 14, 2016 at 01:40:39PM +0100, Linus Walleij wrote: > > On Mon, Mar 14, 2016 at 9:44 AM, Westerberg, Mika > > <mika.westerberg@intel.com> wrote: > > > On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote: > > > > >> > + intel_gpio_irq_ack(d); > > >> > > >> > If the pin toggles right here, we still have the same issue, no? > > >> > > >> Yes. But it is very short time from here to the place IRQ got enabled. > > > > > > That is still a race. > > > > > >> To me, it is the only available platform dependent interface to do > > > the "ACK" in the flow of request_irq. Maybe you have other better > > > option here? > > (...) > > > Drivers need to deal with the fact that they might get spurious > > > interrupts from time to time. You need to check in the interrupt handler > > > if the interrupt was from the device you are driving or not. > > > > > > request_irq() enables the interrupt line and if the pin is already in > > > a state that triggers an interrupt, the driver interrupt handler will > > > be called. > > > > This looks like something is wrong in the irqchip. > > If request_irq() is called so that the interrupt is level triggered, > let's say active low, and the pin is already in that state I would > expect interrupt to trigger immediately when enabled, no? > > If I understand correctly this is precisely what Zheng is describing > they are trying to solve. Scratch that. I just re-read the patch changelog and they are configuring the pin as edge triggered. > > Your set_type() is supporting edges but have all IRQs > > handled by handle_simple_irq() rather than handle_edge_irq() > > for the edges, which gives a more robust control flow > > from IRQ to ACK to calling the handler. > > > > Zheng/Mika: please look at how the level/edge > > IRQs are handled in drivers/gpio/gpio-pl061.c > > where I *tried* to do things right, switching handler > > in .set_type() using irq_set_handler_locked(). I think > > you may need to use handle_edge_irq() for the edge IRQs > > and handle_level_irq() for the level IRQs just like I do > > in the PL061 driver. > > The driver is already doing that as far as I can tell (see > intel_gpio_irq_type()). I will check again if we are still missing something there. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index ded5378..d6fe659 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type) return -EPERM; } + intel_gpio_irq_ack(d); + spin_lock_irqsave(&pctrl->lock, flags); value = readl(reg);