* Re: [PATCH] counter: 104-quad-8: Fix incorrect return value in IRQ handler
2025-12-02 8:39 ` [PATCH] counter: 104-quad-8: Fix incorrect return value in IRQ handler Haotian Zhang
@ 2025-12-06 4:24 ` William Breathitt Gray
2025-12-06 16:30 ` Andy Shevchenko
2025-12-06 18:00 ` [PATCH v2] " Haotian Zhang
2025-12-15 2:01 ` [PATCH v3] " Haotian Zhang
2 siblings, 1 reply; 8+ messages in thread
From: William Breathitt Gray @ 2025-12-06 4:24 UTC (permalink / raw)
To: Haotian Zhang; +Cc: andriy.shevchenko, linux-iio, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]
On Tue, Dec 02, 2025 at 04:39:52PM +0800, Haotian Zhang wrote:
> quad8_irq_handler() should return irqreturn_t enum values, but it
> directly returns negative errno codes from regmap operations on error.
>
> Return IRQ_NONE instead of raw errno codes on regmap operation failures.
>
> Fixes: 98ffe0252911 ("counter: 104-quad-8: Migrate to the regmap API")
> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
> ---
> drivers/counter/104-quad-8.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
> index ce81fc4e1ae7..17f4da6c24af 100644
> --- a/drivers/counter/104-quad-8.c
> +++ b/drivers/counter/104-quad-8.c
> @@ -1201,7 +1201,7 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
>
> ret = regmap_read(priv->map, QUAD8_INTERRUPT_STATUS, &status);
> if (ret)
> - return ret;
> + return IRQ_NONE;
> if (!status)
> return IRQ_NONE;
>
> @@ -1233,7 +1233,7 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
>
> ret = regmap_write(priv->map, QUAD8_CHANNEL_OPERATION, CLEAR_PENDING_INTERRUPTS);
> if (ret)
> - return ret;
> + return IRQ_NONE;
>
> return IRQ_HANDLED;
> }
Hello Haotian,
You are correct, we should return a value of irqreturn_t and not raw
errno codes. However, it would be nice to indicate to users why the IRQ
was left unserviced before return IRQ_NONE. Is there a way to indicate
the regmap_read failure, perhaps via WARN_ONCE() or similar? Is
regmap_read actually capable of failing in this context, or should we
just remove the conditional check entirely?
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] counter: 104-quad-8: Fix incorrect return value in IRQ handler
2025-12-06 4:24 ` William Breathitt Gray
@ 2025-12-06 16:30 ` Andy Shevchenko
0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2025-12-06 16:30 UTC (permalink / raw)
To: William Breathitt Gray; +Cc: Haotian Zhang, linux-iio, linux-kernel
On Sat, Dec 06, 2025 at 01:24:16PM +0900, William Breathitt Gray wrote:
> On Tue, Dec 02, 2025 at 04:39:52PM +0800, Haotian Zhang wrote:
> > quad8_irq_handler() should return irqreturn_t enum values, but it
> > directly returns negative errno codes from regmap operations on error.
> >
> > Return IRQ_NONE instead of raw errno codes on regmap operation failures.
...
> > ret = regmap_read(priv->map, QUAD8_INTERRUPT_STATUS, &status);
> > if (ret)
> > - return ret;
> > + return IRQ_NONE;
This return is correct and we can't do much, if we got again to this handler
due to unserviced IRQ, hopefully the second attempt will succeed. At the end it
will mean something really bad happened to the HW state.
> > if (!status)
> > return IRQ_NONE;
...
> > ret = regmap_write(priv->map, QUAD8_CHANNEL_OPERATION, CLEAR_PENDING_INTERRUPTS);
> > if (ret)
> > - return ret;
> > + return IRQ_NONE;
> >
> > return IRQ_HANDLED;
>
> You are correct, we should return a value of irqreturn_t and not raw
> errno codes. However, it would be nice to indicate to users why the IRQ
> was left unserviced before return IRQ_NONE. Is there a way to indicate
> the regmap_read failure, perhaps via WARN_ONCE() or similar? Is
> regmap_read actually capable of failing in this context, or should we
> just remove the conditional check entirely?
I'm not sure about this case, clearing pending interrupts is something
that should not affect much the flow, I think if we return IRQ_HANDLED here
we can re-enter to the handler and re-read the status. Yes, it will be
spurious interrupt, but at least it will reduce the probability of IRQ storm
registration (when when we return IRQ_NONE).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] counter: 104-quad-8: Fix incorrect return value in IRQ handler
2025-12-02 8:39 ` [PATCH] counter: 104-quad-8: Fix incorrect return value in IRQ handler Haotian Zhang
2025-12-06 4:24 ` William Breathitt Gray
@ 2025-12-06 18:00 ` Haotian Zhang
2025-12-12 3:34 ` William Breathitt Gray
2025-12-15 2:01 ` [PATCH v3] " Haotian Zhang
2 siblings, 1 reply; 8+ messages in thread
From: Haotian Zhang @ 2025-12-06 18:00 UTC (permalink / raw)
To: wbg, andriy.shevchenko; +Cc: linux-iio, linux-kernel, Haotian Zhang
quad8_irq_handler() should return irqreturn_t enum values, but it
directly returns negative errno codes from regmap operations on error.
Return IRQ_NONE if the interrupt status cannot be read. If clearing the
interrupt fails, return IRQ_HANDLED to prevent the kernel from disabling
the IRQ line due to a spurious interrupt storm. Also, log these regmap
failures with WARN_ONCE.
Fixes: 98ffe0252911 ("counter: 104-quad-8: Migrate to the regmap API")
Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
---
Changes in v2:
-Return IRQ_HANDLED if regmap_write() fails.
-Add WARN_ONCE() to log the error messages.
---
drivers/counter/104-quad-8.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index ce81fc4e1ae7..836ab9349dd7 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -1200,8 +1200,10 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
int ret;
ret = regmap_read(priv->map, QUAD8_INTERRUPT_STATUS, &status);
- if (ret)
- return ret;
+ if (ret) {
+ WARN_ONCE(true, "quad8: regmap_read failed: %d\n", ret);
+ return IRQ_NONE;
+ }
if (!status)
return IRQ_NONE;
@@ -1232,8 +1234,10 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
}
ret = regmap_write(priv->map, QUAD8_CHANNEL_OPERATION, CLEAR_PENDING_INTERRUPTS);
- if (ret)
- return ret;
+ if (ret) {
+ WARN_ONCE(true, "quad8: regmap_write failed: %d\n", ret);
+ return IRQ_HANDLED;
+ }
return IRQ_HANDLED;
}
--
2.50.1.windows.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] counter: 104-quad-8: Fix incorrect return value in IRQ handler
2025-12-06 18:00 ` [PATCH v2] " Haotian Zhang
@ 2025-12-12 3:34 ` William Breathitt Gray
2025-12-12 15:04 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: William Breathitt Gray @ 2025-12-12 3:34 UTC (permalink / raw)
To: Haotian Zhang
Cc: William Breathitt Gray, andriy.shevchenko, linux-iio,
linux-kernel
On Sun, Dec 07, 2025 at 02:00:07AM +0800, Haotian Zhang wrote:
> quad8_irq_handler() should return irqreturn_t enum values, but it
> directly returns negative errno codes from regmap operations on error.
>
> Return IRQ_NONE if the interrupt status cannot be read. If clearing the
> interrupt fails, return IRQ_HANDLED to prevent the kernel from disabling
> the IRQ line due to a spurious interrupt storm. Also, log these regmap
> failures with WARN_ONCE.
>
> Fixes: 98ffe0252911 ("counter: 104-quad-8: Migrate to the regmap API")
> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
> ---
> Changes in v2:
> -Return IRQ_HANDLED if regmap_write() fails.
> -Add WARN_ONCE() to log the error messages.
Hi Haotian,
Thank you for making the requested changes. I have a couple more
suggestions below, and then I think this patch will be ready to merge.
> ret = regmap_read(priv->map, QUAD8_INTERRUPT_STATUS, &status);
> - if (ret)
> - return ret;
> + if (ret) {
> + WARN_ONCE(true, "quad8: regmap_read failed: %d\n", ret);
The warning should indicate the purpose of the operation so users know
what failed for the hardware. So perhaps "quad8: Attempt to read
Interrupt Status Register failed: %d\n" is better.
> ret = regmap_write(priv->map, QUAD8_CHANNEL_OPERATION, CLEAR_PENDING_INTERRUPTS);
> - if (ret)
> - return ret;
> + if (ret) {
> + WARN_ONCE(true, "quad8: regmap_write failed: %d\n", ret);
For the same reason as above, perhaps "quad8: Attempt to clear pending
interrupts by writing to Channel Operation Register failed: %d\n" is
better.
William Breathitt Gray
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] counter: 104-quad-8: Fix incorrect return value in IRQ handler
2025-12-12 3:34 ` William Breathitt Gray
@ 2025-12-12 15:04 ` Andy Shevchenko
0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2025-12-12 15:04 UTC (permalink / raw)
To: William Breathitt Gray; +Cc: Haotian Zhang, linux-iio, linux-kernel
On Fri, Dec 12, 2025 at 12:34:07PM +0900, William Breathitt Gray wrote:
> On Sun, Dec 07, 2025 at 02:00:07AM +0800, Haotian Zhang wrote:
...
> > ret = regmap_read(priv->map, QUAD8_INTERRUPT_STATUS, &status);
> > - if (ret)
> > - return ret;
> > + if (ret) {
> > + WARN_ONCE(true, "quad8: regmap_read failed: %d\n", ret);
>
> The warning should indicate the purpose of the operation so users know
> what failed for the hardware. So perhaps "quad8: Attempt to read
> Interrupt Status Register failed: %d\n" is better.
>
> > ret = regmap_write(priv->map, QUAD8_CHANNEL_OPERATION, CLEAR_PENDING_INTERRUPTS);
> > - if (ret)
> > - return ret;
> > + if (ret) {
> > + WARN_ONCE(true, "quad8: regmap_write failed: %d\n", ret);
>
> For the same reason as above, perhaps "quad8: Attempt to clear pending
> interrupts by writing to Channel Operation Register failed: %d\n" is
> better.
On top of that, I'm wondering if we can use dev_WARN_ONCE() and drop "quad8: "
prefix.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] counter: 104-quad-8: Fix incorrect return value in IRQ handler
2025-12-02 8:39 ` [PATCH] counter: 104-quad-8: Fix incorrect return value in IRQ handler Haotian Zhang
2025-12-06 4:24 ` William Breathitt Gray
2025-12-06 18:00 ` [PATCH v2] " Haotian Zhang
@ 2025-12-15 2:01 ` Haotian Zhang
2025-12-22 11:06 ` William Breathitt Gray
2 siblings, 1 reply; 8+ messages in thread
From: Haotian Zhang @ 2025-12-15 2:01 UTC (permalink / raw)
To: wbg, andriy.shevchenko; +Cc: linux-iio, linux-kernel, Haotian Zhang
quad8_irq_handler() should return irqreturn_t enum values, but it
directly returns negative errno codes from regmap operations on error.
Return IRQ_NONE if the interrupt status cannot be read. If clearing the
interrupt fails, return IRQ_HANDLED to prevent the kernel from disabling
the IRQ line due to a spurious interrupt storm. Also, log these regmap
failures with dev_WARN_ONCE.
Fixes: 98ffe0252911 ("counter: 104-quad-8: Migrate to the regmap API")
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
---
Changes in v3:
- Replace WARN_ONCE() calls with dev_WARN_ONCE() to automatically include
device context in log messages.
- Provide more descriptive warning messages indicating the specific
register operation that failed.
Changes in v2:
- Return IRQ_HANDLED if regmap_write() fails.
- Add WARN_ONCE() to log the error messages.
---
drivers/counter/104-quad-8.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index ce81fc4e1ae7..573b2fe93253 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -1192,6 +1192,7 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
{
struct counter_device *counter = private;
struct quad8 *const priv = counter_priv(counter);
+ struct device *dev = counter->parent;
unsigned int status;
unsigned long irq_status;
unsigned long channel;
@@ -1200,8 +1201,11 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
int ret;
ret = regmap_read(priv->map, QUAD8_INTERRUPT_STATUS, &status);
- if (ret)
- return ret;
+ if (ret) {
+ dev_WARN_ONCE(dev, true,
+ "Attempt to read Interrupt Status Register failed: %d\n", ret);
+ return IRQ_NONE;
+ }
if (!status)
return IRQ_NONE;
@@ -1223,8 +1227,9 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
break;
default:
/* should never reach this path */
- WARN_ONCE(true, "invalid interrupt trigger function %u configured for channel %lu\n",
- flg_pins, channel);
+ dev_WARN_ONCE(dev, true,
+ "invalid interrupt trigger function %u configured for channel %lu\n",
+ flg_pins, channel);
continue;
}
@@ -1232,8 +1237,11 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
}
ret = regmap_write(priv->map, QUAD8_CHANNEL_OPERATION, CLEAR_PENDING_INTERRUPTS);
- if (ret)
- return ret;
+ if (ret) {
+ dev_WARN_ONCE(dev, true,
+ "Attempt to clear pending interrupts by writing to Channel Operation Register failed: %d\n", ret);
+ return IRQ_HANDLED;
+ }
return IRQ_HANDLED;
}
--
2.50.1.windows.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3] counter: 104-quad-8: Fix incorrect return value in IRQ handler
2025-12-15 2:01 ` [PATCH v3] " Haotian Zhang
@ 2025-12-22 11:06 ` William Breathitt Gray
0 siblings, 0 replies; 8+ messages in thread
From: William Breathitt Gray @ 2025-12-22 11:06 UTC (permalink / raw)
To: andriy.shevchenko, Haotian Zhang
Cc: William Breathitt Gray, linux-iio, linux-kernel
On Mon, 15 Dec 2025 10:01:14 +0800, Haotian Zhang wrote:
> quad8_irq_handler() should return irqreturn_t enum values, but it
> directly returns negative errno codes from regmap operations on error.
>
> Return IRQ_NONE if the interrupt status cannot be read. If clearing the
> interrupt fails, return IRQ_HANDLED to prevent the kernel from disabling
> the IRQ line due to a spurious interrupt storm. Also, log these regmap
> failures with dev_WARN_ONCE.
>
> [...]
Applied, thanks!
[1/1] counter: 104-quad-8: Fix incorrect return value in IRQ handler
commit: 9517d76dd160208b7a432301ce7bec8fc1ddc305
Best regards,
--
William Breathitt Gray <wbg@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread