In the hardware implementation of the i2c hid driver based on dwapb gpio
irq, when the user continues to use the i2c hid device in the suspend
process, the i2c hid interrupt will be masked after the resume process
is finished.
This is because the disable_irq()/enable_irq() of the dwapb gpio driver
does not synchronize the irq mask register state. In normal use of the
i2c hid procedure, the gpio irq irq_mask()/irq_unmask() functions are
called in pairs. In case of an exception, i2c_hid_core_suspend() calls
disable_irq() to disable the gpio irq. With low probability, this causes
irq_unmask() to not be called, which causes the gpio irq to be masked
and not unmasked in enable_irq(), raising an exception.
Add synchronization to the masked register state in the
dwapb_irq_enable()/dwapb_irq_disable() function. mask the gpio irq
before disabling it. After enabling the gpio irq, unmask the irq.
Fixes: 7779b3455697 ("gpio: add a driver for the Synopsys DesignWare APB GPIO block")
Cc: [email protected]
Co-developed-by: Riwen Lu <[email protected]>
Signed-off-by: Riwen Lu <[email protected]>
Signed-off-by: xiongxin <[email protected]>
Acked-by: Serge Semin <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
v4:
* Add patch tag information
v3:
* Modify the submitter's information
v2:
* Resubmit the patch to fix this exception from the dwapb gpio
driver side.
v1:
* Resolve the exception from the IRQ core layer. (key point not
found correctly)
---
drivers/gpio/gpio-dwapb.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 4a4f61bf6c58..8c59332429c2 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -282,13 +282,15 @@ static void dwapb_irq_enable(struct irq_data *d)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(d);
unsigned long flags;
u32 val;
raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
- val = dwapb_read(gpio, GPIO_INTEN);
- val |= BIT(irqd_to_hwirq(d));
+ val = dwapb_read(gpio, GPIO_INTEN) | BIT(hwirq);
dwapb_write(gpio, GPIO_INTEN, val);
+ val = dwapb_read(gpio, GPIO_INTMASK) & ~BIT(hwirq);
+ dwapb_write(gpio, GPIO_INTMASK, val);
raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
}
@@ -296,12 +298,14 @@ static void dwapb_irq_disable(struct irq_data *d)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(d);
unsigned long flags;
u32 val;
raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
- val = dwapb_read(gpio, GPIO_INTEN);
- val &= ~BIT(irqd_to_hwirq(d));
+ val = dwapb_read(gpio, GPIO_INTMASK) | BIT(hwirq);
+ dwapb_write(gpio, GPIO_INTMASK, val);
+ val = dwapb_read(gpio, GPIO_INTEN) & ~BIT(hwirq);
dwapb_write(gpio, GPIO_INTEN, val);
raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
}
--
2.34.1
On Tue, Dec 19, 2023 at 06:16:20PM +0800, xiongxin wrote:
Code wise it's fine, but while answering to Serge's email I realized that
the Subject has a typo and additionally you or Bart (if he is fine with that)
can amend:
> In the hardware implementation of the i2c hid driver based on dwapb gpio
I?C HID (alternatively: I2C HID)
here and everywhere else in the commit message unless it's the file or
function name.
> irq,
DesignWare GPIO IRQ chip
> when the user continues to use the i2c hid device in the suspend
> process, the i2c hid interrupt will be masked after the resume process
> is finished.
>
> This is because the disable_irq()/enable_irq() of the dwapb gpio driver
DesignWare GPIO
> does not synchronize the irq mask register state. In normal use of the
IRQ
> i2c hid procedure, the gpio irq irq_mask()/irq_unmask() functions are
> called in pairs. In case of an exception, i2c_hid_core_suspend() calls
> disable_irq() to disable the gpio irq. With low probability, this causes
GPIO IRQ
> irq_unmask() to not be called, which causes the gpio irq to be masked
GPIO IRQ
> and not unmasked in enable_irq(), raising an exception.
>
> Add synchronization to the masked register state in the
> dwapb_irq_enable()/dwapb_irq_disable() function. mask the gpio irq
GPIO IRQ
> before disabling it. After enabling the gpio irq, unmask the irq.
GPIO IRQ
IRQ
--
With Best Regards,
Andy Shevchenko
In the hardware implementation of the I2C HID driver based on DesignWare
GPIO IRQ chip, when the user continues to use the I2C HID device in the
suspend process, the I2C HID interrupt will be masked after the resume
process is finished.
This is because the disable_irq()/enable_irq() of the DesignWare GPIO
driver does not synchronize the IRQ mask register state. In normal use
of the I2C HID procedure, the GPIO IRQ irq_mask()/irq_unmask() functions
are called in pairs. In case of an exception, i2c_hid_core_suspend()
calls disable_irq() to disable the GPIO IRQ. With low probability, this
causes irq_unmask() to not be called, which causes the GPIO IRQ to be
masked and not unmasked in enable_irq(), raising an exception.
Add synchronization to the masked register state in the
dwapb_irq_enable()/dwapb_irq_disable() function. mask the GPIO IRQ
before disabling it. After enabling the GPIO IRQ, unmask the IRQ.
Fixes: 7779b3455697 ("gpio: add a driver for the Synopsys DesignWare APB GPIO block")
Cc: [email protected]
Co-developed-by: Riwen Lu <[email protected]>
Signed-off-by: Riwen Lu <[email protected]>
Signed-off-by: xiongxin <[email protected]>
Acked-by: Serge Semin <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
v5:
* fix typo in patch description
v4:
* Add patch tag information
v3:
* Modify the submitter's information
v2:
* Resubmit the patch to fix this exception from the DesignWare
GPIO driver side
v1:
* Resolve the exception from the IRQ core layer. (key point not
found correctly)
---
drivers/gpio/gpio-dwapb.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 4a4f61bf6c58..8c59332429c2 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -282,13 +282,15 @@ static void dwapb_irq_enable(struct irq_data *d)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(d);
unsigned long flags;
u32 val;
raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
- val = dwapb_read(gpio, GPIO_INTEN);
- val |= BIT(irqd_to_hwirq(d));
+ val = dwapb_read(gpio, GPIO_INTEN) | BIT(hwirq);
dwapb_write(gpio, GPIO_INTEN, val);
+ val = dwapb_read(gpio, GPIO_INTMASK) & ~BIT(hwirq);
+ dwapb_write(gpio, GPIO_INTMASK, val);
raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
}
@@ -296,12 +298,14 @@ static void dwapb_irq_disable(struct irq_data *d)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(d);
unsigned long flags;
u32 val;
raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
- val = dwapb_read(gpio, GPIO_INTEN);
- val &= ~BIT(irqd_to_hwirq(d));
+ val = dwapb_read(gpio, GPIO_INTMASK) | BIT(hwirq);
+ dwapb_write(gpio, GPIO_INTMASK, val);
+ val = dwapb_read(gpio, GPIO_INTEN) & ~BIT(hwirq);
dwapb_write(gpio, GPIO_INTEN, val);
raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
}
--
2.34.1
On Wed, Dec 20, 2023 at 10:29:01AM +0800, xiongxin wrote:
> In the hardware implementation of the I2C HID driver based on DesignWare
> GPIO IRQ chip, when the user continues to use the I2C HID device in the
> suspend process, the I2C HID interrupt will be masked after the resume
> process is finished.
>
> This is because the disable_irq()/enable_irq() of the DesignWare GPIO
> driver does not synchronize the IRQ mask register state. In normal use
> of the I2C HID procedure, the GPIO IRQ irq_mask()/irq_unmask() functions
> are called in pairs. In case of an exception, i2c_hid_core_suspend()
> calls disable_irq() to disable the GPIO IRQ. With low probability, this
> causes irq_unmask() to not be called, which causes the GPIO IRQ to be
> masked and not unmasked in enable_irq(), raising an exception.
>
> Add synchronization to the masked register state in the
> dwapb_irq_enable()/dwapb_irq_disable() function. mask the GPIO IRQ
> before disabling it. After enabling the GPIO IRQ, unmask the IRQ.
...
> v5:
> * fix typo in patch description
Thank you! This looks perfect!
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 20, 2023 at 3:29 AM xiongxin <[email protected]> wrote:
>
> In the hardware implementation of the I2C HID driver based on DesignWare
> GPIO IRQ chip, when the user continues to use the I2C HID device in the
> suspend process, the I2C HID interrupt will be masked after the resume
> process is finished.
>
> This is because the disable_irq()/enable_irq() of the DesignWare GPIO
> driver does not synchronize the IRQ mask register state. In normal use
> of the I2C HID procedure, the GPIO IRQ irq_mask()/irq_unmask() functions
> are called in pairs. In case of an exception, i2c_hid_core_suspend()
> calls disable_irq() to disable the GPIO IRQ. With low probability, this
> causes irq_unmask() to not be called, which causes the GPIO IRQ to be
> masked and not unmasked in enable_irq(), raising an exception.
>
> Add synchronization to the masked register state in the
> dwapb_irq_enable()/dwapb_irq_disable() function. mask the GPIO IRQ
> before disabling it. After enabling the GPIO IRQ, unmask the IRQ.
>
> Fixes: 7779b3455697 ("gpio: add a driver for the Synopsys DesignWare APB GPIO block")
> Cc: [email protected]
> Co-developed-by: Riwen Lu <[email protected]>
> Signed-off-by: Riwen Lu <[email protected]>
> Signed-off-by: xiongxin <[email protected]>
> Acked-by: Serge Semin <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
> v5:
> * fix typo in patch description
> v4:
> * Add patch tag information
> v3:
> * Modify the submitter's information
> v2:
> * Resubmit the patch to fix this exception from the DesignWare
> GPIO driver side
> v1:
> * Resolve the exception from the IRQ core layer. (key point not
> found correctly)
> ---
> drivers/gpio/gpio-dwapb.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 4a4f61bf6c58..8c59332429c2 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -282,13 +282,15 @@ static void dwapb_irq_enable(struct irq_data *d)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> unsigned long flags;
> u32 val;
>
> raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> - val = dwapb_read(gpio, GPIO_INTEN);
> - val |= BIT(irqd_to_hwirq(d));
> + val = dwapb_read(gpio, GPIO_INTEN) | BIT(hwirq);
> dwapb_write(gpio, GPIO_INTEN, val);
> + val = dwapb_read(gpio, GPIO_INTMASK) & ~BIT(hwirq);
> + dwapb_write(gpio, GPIO_INTMASK, val);
> raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> }
>
> @@ -296,12 +298,14 @@ static void dwapb_irq_disable(struct irq_data *d)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> unsigned long flags;
> u32 val;
>
> raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> - val = dwapb_read(gpio, GPIO_INTEN);
> - val &= ~BIT(irqd_to_hwirq(d));
> + val = dwapb_read(gpio, GPIO_INTMASK) | BIT(hwirq);
> + dwapb_write(gpio, GPIO_INTMASK, val);
> + val = dwapb_read(gpio, GPIO_INTEN) & ~BIT(hwirq);
> dwapb_write(gpio, GPIO_INTEN, val);
> raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> }
> --
> 2.34.1
>
Queued for fixes, thanks!
Bartosz