diff mbox series

[v4,10/10] i2x: pnx: Use threaded irq to fix warning from del_timer_sync()

Message ID 20240620175657.358273-11-piotr.wojtaszczyk@timesys.com (mailing list archive)
State Not Applicable, archived
Headers show
Series Add audio support for LPC32XX CPUs | expand

Commit Message

Piotr Wojtaszczyk June 20, 2024, 5:56 p.m. UTC
When del_timer_sync() is called in an interrupt context it throws a warning
because of potential deadlock. Threaded irq handler fixes the potential
problem.

Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>
---
 drivers/i2c/busses/i2c-pnx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andi Shyti June 20, 2024, 10:57 p.m. UTC | #1
Hi Piotr,

On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote:
> When del_timer_sync() is called in an interrupt context it throws a warning
> because of potential deadlock. Threaded irq handler fixes the potential
> problem.
> 
> Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>

did you run into a lockdep splat?

Anything against using del_timer(), instead? Have you tried?

Thanks,
Andi
Piotr Wojtaszczyk June 21, 2024, 12:08 p.m. UTC | #2
Hi Andi,

On Fri, Jun 21, 2024 at 12:57 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote:
> > When del_timer_sync() is called in an interrupt context it throws a warning
> > because of potential deadlock. Threaded irq handler fixes the potential
> > problem.
> >
> > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>
>
> did you run into a lockdep splat?
>
> Anything against using del_timer(), instead? Have you tried?

I didn't get a lockdep splat but console was flooded with warnings from
https://github.com/torvalds/linux/blob/v6.10-rc4/kernel/time/timer.c#L1655
In the linux kernel v5.15 I didn't see these warnings.

I'm not a maintainer of the driver and I didn't do any research on
what kind of impact
would have using del_timer() instad. Maybe Vladimir Zapolskiy will know that.
Andi Shyti June 25, 2024, 9:12 p.m. UTC | #3
Hi Piotr,

On Fri, Jun 21, 2024 at 02:08:03PM GMT, Piotr Wojtaszczyk wrote:
> On Fri, Jun 21, 2024 at 12:57 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote:
> > > When del_timer_sync() is called in an interrupt context it throws a warning
> > > because of potential deadlock. Threaded irq handler fixes the potential
> > > problem.
> > >
> > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>
> >
> > did you run into a lockdep splat?
> >
> > Anything against using del_timer(), instead? Have you tried?
> 
> I didn't get a lockdep splat but console was flooded with warnings from
> https://github.com/torvalds/linux/blob/v6.10-rc4/kernel/time/timer.c#L1655
> In the linux kernel v5.15 I didn't see these warnings.
> 
> I'm not a maintainer of the driver and I didn't do any research on
> what kind of impact
> would have using del_timer() instad. Maybe Vladimir Zapolskiy will know that.

Your patch is definitely correct, no doubt about that.

And I don't have anything aginast changing irq handlers to
threaded handlers. But I would be careful at doing that depending
on the use of the controller and for accepting such change I
would need an ack from someone who knows the device. Vladimir,
perhaps?

There are cases where using threaded handlers are not totally
right, for example when the controller is used at early boot for
power management handling. I don't think it's the case for this
driver, but I can't be 100% sure.

If you were able to see the flood of WARN_ON's, would be
interesting to know how it behaves with del_timer(). Mind
giving it a test?

Thanks,
Andi
Piotr Wojtaszczyk June 27, 2024, 11:05 a.m. UTC | #4
On Tue, Jun 25, 2024 at 11:12 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Piotr,
>
> On Fri, Jun 21, 2024 at 02:08:03PM GMT, Piotr Wojtaszczyk wrote:
> > On Fri, Jun 21, 2024 at 12:57 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > > On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote:
> > > > When del_timer_sync() is called in an interrupt context it throws a warning
> > > > because of potential deadlock. Threaded irq handler fixes the potential
> > > > problem.
> > > >
> > > > Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>
> > >
> > > did you run into a lockdep splat?
> > >
> > > Anything against using del_timer(), instead? Have you tried?
> >
> > I didn't get a lockdep splat but console was flooded with warnings from
> > https://github.com/torvalds/linux/blob/v6.10-rc4/kernel/time/timer.c#L1655
> > In the linux kernel v5.15 I didn't see these warnings.
> >
> > I'm not a maintainer of the driver and I didn't do any research on
> > what kind of impact
> > would have using del_timer() instad. Maybe Vladimir Zapolskiy will know that.
>
> Your patch is definitely correct, no doubt about that.
>
> And I don't have anything aginast changing irq handlers to
> threaded handlers. But I would be careful at doing that depending
> on the use of the controller and for accepting such change I
> would need an ack from someone who knows the device. Vladimir,
> perhaps?
>
> There are cases where using threaded handlers are not totally
> right, for example when the controller is used at early boot for
> power management handling. I don't think it's the case for this
> driver, but I can't be 100% sure.
>
> If you were able to see the flood of WARN_ON's, would be
> interesting to know how it behaves with del_timer(). Mind
> giving it a test?
>
> Thanks,
> Andi

I took some time to take a closer look at this and it turns out that the
timer is used only to exit from the wait_for_completion(), after each
del_timer_sync() there is a complete() call. So I will remove the timer
all together and replace wait_for_completion() with
wait_for_completion_timeout()
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index a12525b3186b..b2ab6fb168bf 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -718,8 +718,8 @@  static int i2c_pnx_probe(struct platform_device *pdev)
 		ret = alg_data->irq;
 		goto out_clock;
 	}
-	ret = devm_request_irq(&pdev->dev, alg_data->irq, i2c_pnx_interrupt,
-			       0, pdev->name, alg_data);
+	ret = devm_request_threaded_irq(&pdev->dev, alg_data->irq, NULL, i2c_pnx_interrupt,
+					IRQF_ONESHOT, pdev->name, alg_data);
 	if (ret)
 		goto out_clock;