| Message ID | 20180717005719.258905-2-djkurtz@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] pinctrl/amd: only handle irq if it is pending and unmasked | expand |
On Mon, Jul 16, 2018 at 7:57 PM, Daniel Kurtz <djkurtz@chromium.org> wrote: > Commit 6afb10267c1692 ("pinctrl/amd: fix masking of GPIO interrupts") > changed to the clearing of interrupt status bits to a RMW in a critical > section. This works, but is a bit overkill. > > The relevant interrupt/wake status bits are in the Most Significant Byte > of a 32-bit word. These two are the only write-able bits in this byte. I don't have the hardware to test this any more, and I also don't have any docs to double if those are really the only writable bits, but looking at the existing driver code it does seem to be the case. I think you should retain the comment noting that the value of the register may have changed since it was read just a few lines above (and hence explaining more precisely why we make the special effort just to modify the MSB), just in case there is further rework of this code in future and we end up walking into the same trap. It was one of those issues that took a frustratingly long time to figure out... Thanks Daniel -- 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
Hi Daniel, On Tue, Jul 17, 2018 at 6:30 AM Daniel Drake <drake@endlessm.com> wrote: > > On Mon, Jul 16, 2018 at 7:57 PM, Daniel Kurtz <djkurtz@chromium.org> wrote: > > Commit 6afb10267c1692 ("pinctrl/amd: fix masking of GPIO interrupts") > > changed to the clearing of interrupt status bits to a RMW in a critical > > section. This works, but is a bit overkill. > > > > The relevant interrupt/wake status bits are in the Most Significant Byte > > of a 32-bit word. These two are the only write-able bits in this byte. > > I don't have the hardware to test this any more, and I also don't have > any docs to double if those are really the only writable bits, but > looking at the existing driver code it does seem to be the case. > > I think you should retain the comment noting that the value of the > register may have changed since it was read just a few lines above > (and hence explaining more precisely why we make the special effort > just to modify the MSB), just in case there is further rework of this > code in future and we end up walking into the same trap. It was one of > those issues that took a frustratingly long time to figure out... Sounds reasonable. How about: - /* Clear interrupt. - * We must read the pin register again, in case the - * value was changed while executing - * generic_handle_irq() above. + /* + * Write-1-to-clear irq/wake status bits in MSByte. + * All other bits in this byte are read-only. + * This avoids modifying the lower 24-bits because they may have + * changed while executing generic_handle_irq() above. */ > > Thanks > Daniel -- 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 Fri, Jul 20, 2018 at 6:38 PM, Daniel Kurtz <djkurtz@chromium.org> wrote: > Sounds reasonable. How about: > > - /* Clear interrupt. > - * We must read the pin register again, in case the > - * value was changed while executing > - * generic_handle_irq() above. > + /* > + * Write-1-to-clear irq/wake status bits in MSByte. > + * All other bits in this byte are read-only. > + * This avoids modifying the lower 24-bits > because they may have > + * changed while executing generic_handle_irq() above. > */ That looks good. Thanks Daniel -- 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/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c index b91db89eb9247c..52efe77ffb9991 100644 --- a/drivers/pinctrl/pinctrl-amd.c +++ b/drivers/pinctrl/pinctrl-amd.c @@ -558,15 +558,11 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id) irq = irq_find_mapping(gc->irq.domain, irqnr + i); generic_handle_irq(irq); - /* Clear interrupt. - * We must read the pin register again, in case the - * value was changed while executing - * generic_handle_irq() above. + /* + * Write-1-to-clear irq/wake status bits in MSByte. + * All other bits in this byte are read-only. */ - raw_spin_lock_irqsave(&gpio_dev->lock, flags); - regval = readl(regs + i); - writel(regval, regs + i); - raw_spin_unlock_irqrestore(&gpio_dev->lock, flags); + writeb((regval >> 24), (u8 *)(regs + i) + 3); ret = IRQ_HANDLED; } }
Commit 6afb10267c1692 ("pinctrl/amd: fix masking of GPIO interrupts") changed to the clearing of interrupt status bits to a RMW in a critical section. This works, but is a bit overkill. The relevant interrupt/wake status bits are in the Most Significant Byte of a 32-bit word. These two are the only write-able bits in this byte. Therefore, it should be safe to just write these bits back as a byte access without any additional locking. Signed-off-by: Daniel Kurtz <djkurtz@chromium.org> --- drivers/pinctrl/pinctrl-amd.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)