[PATCH 0/2] rtc: isl1208: Fix IRQ return value and wake reference leak

[PATCH 0/2] rtc: isl1208: Fix IRQ return value and wake reference leak

[John Madieu]

Hi all,

This series fixes two issues in rtc-isl1208. The first is
isl1208_rtc_interrupt() returning negative i2c errnos where
irqreturn_t is expected, which genirq flags as bogus and logs
on each IRQ. The second is a wake-reference leak: setup_irq()
calls enable_irq_wake() on success but the driver has no remove
path that balances it, so each rebind cycle leaks one wake
reference per IRQ.

John Madieu (2):
  rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
  rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on
    cleanup

 drivers/rtc/rtc-isl1208.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler

[John Madieu]

isl1208_rtc_interrupt() is of irqreturn_t type but two paths
return a negative i2c errno instead of an IRQ_* value:

  - The SR-poll loop on timeout: `return sr;`
  - The post-alarm cleanup path: `return err;`

genirq's note_interrupt() casts the return to unsigned int and
flags any value above IRQ_HANDLED|IRQ_WAKE_THREAD as a bogus
return, logging "irq event N: bogus return value X" each time it
happens.

Return IRQ_NONE when the SR read failed (no progress, can't claim
the interrupt) and IRQ_HANDLED when toggle_alarm failed.

Fixes: cf044f0ed526 ("drivers/rtc/rtc-isl1208.c: add alarm support")
Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
---
 drivers/rtc/rtc-isl1208.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index f71a6bb77b2a..c93998c53e7a 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -654,7 +654,7 @@ isl1208_rtc_interrupt(int irq, void *data)
 		if (time_after(jiffies, timeout)) {
 			dev_err(&client->dev, "%s: reading SR failed\n",
 				__func__);
-			return sr;
+			return IRQ_NONE;
 		}
 	}
 
@@ -666,7 +666,7 @@ isl1208_rtc_interrupt(int irq, void *data)
 		/* Disable the alarm */
 		err = isl1208_rtc_toggle_alarm(client, 0);
 		if (err)
-			return err;
+			return IRQ_HANDLED;
 
 		fsleep(275);
 
-- 
2.25.1

RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler

[Biju Das]

Hi John,

Thanks for the patch.

> -----Original Message-----
> From: John Madieu <john.madieu.xa@bp.renesas.com>
> Sent: 25 April 2026 16:50
> Subject: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
> 
> isl1208_rtc_interrupt() is of irqreturn_t type but two paths return a negative i2c errno instead of an
> IRQ_* value:
> 
>   - The SR-poll loop on timeout: `return sr;`
>   - The post-alarm cleanup path: `return err;`
> 
> genirq's note_interrupt() casts the return to unsigned int and flags any value above
> IRQ_HANDLED|IRQ_WAKE_THREAD as a bogus return, logging "irq event N: bogus return value X" each time it
> happens.
> 
> Return IRQ_NONE when the SR read failed (no progress, can't claim the interrupt) and IRQ_HANDLED when
> toggle_alarm failed.
> 
> Fixes: cf044f0ed526 ("drivers/rtc/rtc-isl1208.c: add alarm support")
> Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
> ---
>  drivers/rtc/rtc-isl1208.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c index f71a6bb77b2a..c93998c53e7a
> 100644
> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -654,7 +654,7 @@ isl1208_rtc_interrupt(int irq, void *data)
>  		if (time_after(jiffies, timeout)) {
>  			dev_err(&client->dev, "%s: reading SR failed\n",
>  				__func__);
> -			return sr;
> +			return IRQ_NONE;

Maybe you can use a goto statement?? that will take care of handled IRQ's

		goto err_irq:

err_irq:
	return handled ? IRQ_HANDLED : IRQ_NONE;

>  		}
>  	}
> 
> @@ -666,7 +666,7 @@ isl1208_rtc_interrupt(int irq, void *data)
>  		/* Disable the alarm */
>  		err = isl1208_rtc_toggle_alarm(client, 0);
>  		if (err)
> -			return err;
> +			return IRQ_HANDLED;

Same as above.

Cheers,
Biju

> 
>  		fsleep(275);
> 
> --
> 2.25.1

RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler

[John Madieu]

Hi Biju,

Thanks fort he review.

> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: Samstag, 25. April 2026 19:16
> To: John Madieu <john.madieu.xa@bp.renesas.com>;
> alexandre.belloni@bootlin.com
> Subject: RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t
> in IRQ handler
> 
> Hi John,
> 
> Thanks for the patch.
> 
> > -----Original Message-----
> > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > Sent: 25 April 2026 16:50
> > Subject: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t
> > in IRQ handler
> >
> > isl1208_rtc_interrupt() is of irqreturn_t type but two paths return a
> > negative i2c errno instead of an
> > IRQ_* value:
> >
> >   - The SR-poll loop on timeout: `return sr;`
> >   - The post-alarm cleanup path: `return err;`
> >
> > genirq's note_interrupt() casts the return to unsigned int and flags
> > any value above IRQ_HANDLED|IRQ_WAKE_THREAD as a bogus return, logging
> > "irq event N: bogus return value X" each time it happens.
> >
> > Return IRQ_NONE when the SR read failed (no progress, can't claim the
> > interrupt) and IRQ_HANDLED when toggle_alarm failed.
> >
> > Fixes: cf044f0ed526 ("drivers/rtc/rtc-isl1208.c: add alarm support")
> > Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
> > ---
> >  drivers/rtc/rtc-isl1208.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> > index f71a6bb77b2a..c93998c53e7a
> > 100644
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
> > @@ -654,7 +654,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> >  		if (time_after(jiffies, timeout)) {
> >  			dev_err(&client->dev, "%s: reading SR failed\n",
> >  				__func__);
> > -			return sr;
> > +			return IRQ_NONE;
> 
> Maybe you can use a goto statement?? that will take care of handled IRQ's
> 
> 		goto err_irq:
> 
> err_irq:
> 	return handled ? IRQ_HANDLED : IRQ_NONE;

Agreed. I'll do it your way in v2.

> 
> >  		}
> >  	}
> >
> > @@ -666,7 +666,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> >  		/* Disable the alarm */
> >  		err = isl1208_rtc_toggle_alarm(client, 0);
> >  		if (err)
> > -			return err;
> > +			return IRQ_HANDLED;
> 
> Same as above.
> 

I'll set handled = 1 so that goto can return IRQ_HANDLED.

Regards,
John

> Cheers,
> Biju
> 
> >
> >  		fsleep(275);
> >
> > --
> > 2.25.1

RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler

[Biju Das]

Hi John,

> -----Original Message-----
> From: John Madieu <john.madieu.xa@bp.renesas.com>
> Sent: 26 April 2026 18:48
> To: Biju Das <biju.das.jz@bp.renesas.com>; alexandre.belloni@bootlin.com
> Cc: ryan@bluewatersys.com; akpm@linux-foundation.org; m.grzeschik@pengutronix.de;
> Denis.Osterland@diehl.com; linux-rtc@vger.kernel.org; linux-kernel@vger.kernel.org;
> john.madieu@gmail.com
> Subject: RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
> 
> Hi Biju,
> 
> Thanks fort he review.
> 
> > -----Original Message-----
> > From: Biju Das <biju.das.jz@bp.renesas.com>
> > Sent: Samstag, 25. April 2026 19:16
> > To: John Madieu <john.madieu.xa@bp.renesas.com>;
> > alexandre.belloni@bootlin.com
> > Subject: RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as
> > irqreturn_t in IRQ handler
> >
> > Hi John,
> >
> > Thanks for the patch.
> >
> > > -----Original Message-----
> > > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > > Sent: 25 April 2026 16:50
> > > Subject: [PATCH 1/2] rtc: isl1208: Fix returning errno as
> > > irqreturn_t in IRQ handler
> > >
> > > isl1208_rtc_interrupt() is of irqreturn_t type but two paths return
> > > a negative i2c errno instead of an
> > > IRQ_* value:
> > >
> > >   - The SR-poll loop on timeout: `return sr;`
> > >   - The post-alarm cleanup path: `return err;`
> > >
> > > genirq's note_interrupt() casts the return to unsigned int and flags
> > > any value above IRQ_HANDLED|IRQ_WAKE_THREAD as a bogus return,
> > > logging "irq event N: bogus return value X" each time it happens.
> > >
> > > Return IRQ_NONE when the SR read failed (no progress, can't claim
> > > the
> > > interrupt) and IRQ_HANDLED when toggle_alarm failed.
> > >
> > > Fixes: cf044f0ed526 ("drivers/rtc/rtc-isl1208.c: add alarm support")
> > > Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
> > > ---
> > >  drivers/rtc/rtc-isl1208.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> > > index f71a6bb77b2a..c93998c53e7a
> > > 100644
> > > --- a/drivers/rtc/rtc-isl1208.c
> > > +++ b/drivers/rtc/rtc-isl1208.c
> > > @@ -654,7 +654,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> > >  		if (time_after(jiffies, timeout)) {
> > >  			dev_err(&client->dev, "%s: reading SR failed\n",
> > >  				__func__);
> > > -			return sr;
> > > +			return IRQ_NONE;
> >
> > Maybe you can use a goto statement?? that will take care of handled
> > IRQ's
> >
> > 		goto err_irq:
> >
> > err_irq:
> > 	return handled ? IRQ_HANDLED : IRQ_NONE;
> 
> Agreed. I'll do it your way in v2.


> 
> >
> > >  		}
> > >  	}
> > >
> > > @@ -666,7 +666,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> > >  		/* Disable the alarm */
> > >  		err = isl1208_rtc_toggle_alarm(client, 0);
> > >  		if (err)
> > > -			return err;
> > > +			return IRQ_HANDLED;
> >
> > Same as above.
> >
> 
> I'll set handled = 1 so that goto can return IRQ_HANDLED.

Looks this is change in behaviour compared to original code,
previous code does not set, handled = 1 for this path.

Cheers,
Biju

[PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup

[John Madieu]

isl1208_setup_irq() calls enable_irq_wake() after a successful
IRQ request, but the driver has no remove path that balances it.
The driver is devm-only, so on unbind devm releases the IRQ -
but enable_irq_wake() is not undone by IRQ release, so the wake
count for that IRQ stays incremented.

Each rebind therefore leaks one wake reference; the leak doubles
for the chip variant that has a separate evdet IRQ, since
isl1208_setup_irq() is then called twice during probe.

Register a devm action that calls disable_irq_wake() per IRQ.
While at it, check enable_irq_wake()'s return value:
on failure, propagate the error rather than silently registering
a disable action for an IRQ whose wake state was never enabled.

Fixes: 9ece7cd833a3 ("rtc: isl1208: Add "evdet" interrupt source for isl1219")
Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
---
 drivers/rtc/rtc-isl1208.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index c93998c53e7a..1de80fc9c9c9 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -822,6 +822,11 @@ static const struct nvmem_config isl1208_nvmem_config = {
 	.reg_write = isl1208_nvmem_write,
 };
 
+static void isl1208_disable_irq_wake_action(void *data)
+{
+	disable_irq_wake((unsigned long)data);
+}
+
 static int isl1208_setup_irq(struct i2c_client *client, int irq)
 {
 	int rc = devm_request_threaded_irq(&client->dev, irq, NULL,
@@ -831,7 +836,15 @@ static int isl1208_setup_irq(struct i2c_client *client, int irq)
 					client);
 	if (!rc) {
 		device_init_wakeup(&client->dev, true);
-		enable_irq_wake(irq);
+		rc = enable_irq_wake(irq);
+		if (rc)
+			return rc;
+
+		rc = devm_add_action_or_reset(&client->dev,
+					      isl1208_disable_irq_wake_action,
+					      (void *)(unsigned long)irq);
+		if (rc)
+			return rc;
 	} else {
 		dev_err(&client->dev,
 			"Unable to request irq %d, no alarm support\n",
-- 
2.25.1

RE: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup

[Biju Das]

Hi John,

> -----Original Message-----
> From: John Madieu <john.madieu.xa@bp.renesas.com>
> Sent: 25 April 2026 16:50
> Subject: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
> 
> isl1208_setup_irq() calls enable_irq_wake() after a successful IRQ request, but the driver has no
> remove path that balances it.
> The driver is devm-only, so on unbind devm releases the IRQ - but enable_irq_wake() is not undone by
> IRQ release, so the wake count for that IRQ stays incremented.
> 
> Each rebind therefore leaks one wake reference; the leak doubles for the chip variant that has a
> separate evdet IRQ, since
> isl1208_setup_irq() is then called twice during probe.

Is removal of RTC device possible [1]?

[1]
https://patchwork.ozlabs.org/project/rtc-linux/patch/20230922081208.26334-1-biju.das.jz@bp.renesas.com/#3195765

Cheers,
Biju

Re: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup

[Alexandre Belloni]

On 25/04/2026 16:39:16+0000, Biju Das wrote:
> Hi John,
> 
> > -----Original Message-----
> > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > Sent: 25 April 2026 16:50
> > Subject: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
> > 
> > isl1208_setup_irq() calls enable_irq_wake() after a successful IRQ request, but the driver has no
> > remove path that balances it.
> > The driver is devm-only, so on unbind devm releases the IRQ - but enable_irq_wake() is not undone by
> > IRQ release, so the wake count for that IRQ stays incremented.
> > 
> > Each rebind therefore leaks one wake reference; the leak doubles for the chip variant that has a
> > separate evdet IRQ, since
> > isl1208_setup_irq() is then called twice during probe.
> 
> Is removal of RTC device possible [1]?
> 
> [1]
> https://patchwork.ozlabs.org/project/rtc-linux/patch/20230922081208.26334-1-biju.das.jz@bp.renesas.com/#3195765
> 

I'd say yes if this is not the RTC that is backing alarmtimer or
alarmtimer is not compiled in the kernel.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup

[John Madieu]

Hi Biju, Alexandre,

Thanks for the feedback.

> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: Samstag, 25. April 2026 20:37
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Subject: Re: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with
> disable_irq_wake() on cleanup
> 
> On 25/04/2026 16:39:16+0000, Biju Das wrote:
> > Hi John,
> >
> > > -----Original Message-----
> > > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > > Sent: 25 April 2026 16:50
> > > Subject: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with
> > > disable_irq_wake() on cleanup
> > >
> > > isl1208_setup_irq() calls enable_irq_wake() after a successful IRQ
> > > request, but the driver has no remove path that balances it.
> > > The driver is devm-only, so on unbind devm releases the IRQ - but
> > > enable_irq_wake() is not undone by IRQ release, so the wake count for
> that IRQ stays incremented.
> > >
> > > Each rebind therefore leaks one wake reference; the leak doubles for
> > > the chip variant that has a separate evdet IRQ, since
> > > isl1208_setup_irq() is then called twice during probe.
> >
> > Is removal of RTC device possible [1]?
> >

This patch addresses the per-IRQ wake refcount leak, which I
think is independent of whether alarmtimer is holding the RTC.
Found this by code inspection while working on patch [1/2]'s
issue.

> > [1]
> > https://patchwork.ozlabs.org/project/rtc-linux/patch/20230922081208.26
> > 334-1-biju.das.jz@bp.renesas.com/#3195765
> >
> 
> I'd say yes if this is not the RTC that is backing alarmtimer or
> alarmtimer is not compiled in the kernel.
> 

Thanks for the clarification. That said, looking around I noticed
most RTC drivers don't bother balancing enable_irq_wake() either.

Alexandre, do you want this patch as-is, or would you rather I
drop it? I'm fine either way.

Regards,
John

RE: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup

[Biju Das]

Hi Alexandre Belloni,

Thanks for the feedback.

> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: 25 April 2026 19:37
> Subject: Re: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
> 
> On 25/04/2026 16:39:16+0000, Biju Das wrote:
> > Hi John,
> >
> > > -----Original Message-----
> > > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > > Sent: 25 April 2026 16:50
> > > Subject: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with
> > > disable_irq_wake() on cleanup
> > >
> > > isl1208_setup_irq() calls enable_irq_wake() after a successful IRQ
> > > request, but the driver has no remove path that balances it.
> > > The driver is devm-only, so on unbind devm releases the IRQ - but
> > > enable_irq_wake() is not undone by IRQ release, so the wake count for that IRQ stays incremented.
> > >
> > > Each rebind therefore leaks one wake reference; the leak doubles for
> > > the chip variant that has a separate evdet IRQ, since
> > > isl1208_setup_irq() is then called twice during probe.
> >
> > Is removal of RTC device possible [1]?
> >
> > [1]
> > https://patchwork.ozlabs.org/project/rtc-linux/patch/20230922081208.26
> > 334-1-biju.das.jz@bp.renesas.com/#3195765
> >
> 
> I'd say yes if this is not the RTC that is backing alarmtimer or alarmtimer is not compiled in the
> kernel.

obj-y += timeconv.o timecounter.o alarmtimer.o

Alarm timer is always compiled.

Now, On RZ/G3E SMARC EVK, only single RTC that have backing
alarmtimer support. On this platform, it cannot wake up the device
from deepsleep on RTC ALARM. But it can do timekeeping.

Cheers,
Biju