2021-03-22 11:12:26

by Johan Hovold

[permalink] [raw]
Subject: [PATCH] serial: imx: drop workaround for forced irq threading

Force-threaded interrupt handlers used to run with interrupts enabled,
something which could lead to deadlocks in case a threaded handler
shared a lock with code running in hard interrupt context (e.g. timer
callbacks) and did not explicitly disable interrupts.

This was specifically the case for serial drivers that take the port
lock in their console write path as printk can be called from hard
interrupt context also with forced threading ("threadirqs").

Since commit 81e2073c175b ("genirq: Disable interrupts for force
threaded handlers") interrupt handlers always run with interrupts
disabled on non-RT so that drivers no longer need to do handle this.

Drop the now obsolete workaround added by commit 33f16855dcb9 ("tty:
serial: imx: fix potential deadlock").

Cc: Sam Nobs <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/serial/imx.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8257597d034d..cca730fc2b0b 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -922,14 +922,8 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id)
struct imx_port *sport = dev_id;
unsigned int usr1, usr2, ucr1, ucr2, ucr3, ucr4;
irqreturn_t ret = IRQ_NONE;
- unsigned long flags = 0;

- /*
- * IRQs might not be disabled upon entering this interrupt handler,
- * e.g. when interrupt handlers are forced to be threaded. To support
- * this scenario as well, disable IRQs when acquiring the spinlock.
- */
- spin_lock_irqsave(&sport->port.lock, flags);
+ spin_lock(&sport->port.lock);

usr1 = imx_uart_readl(sport, USR1);
usr2 = imx_uart_readl(sport, USR2);
@@ -999,7 +993,7 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id)
ret = IRQ_HANDLED;
}

- spin_unlock_irqrestore(&sport->port.lock, flags);
+ spin_unlock(&sport->port.lock);

return ret;
}
--
2.26.3


2021-03-22 11:36:06

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> Force-threaded interrupt handlers used to run with interrupts enabled,
> something which could lead to deadlocks in case a threaded handler
> shared a lock with code running in hard interrupt context (e.g. timer
> callbacks) and did not explicitly disable interrupts.
>
> This was specifically the case for serial drivers that take the port
> lock in their console write path as printk can be called from hard
> interrupt context also with forced threading ("threadirqs").
>
> Since commit 81e2073c175b ("genirq: Disable interrupts for force
> threaded handlers") interrupt handlers always run with interrupts
> disabled on non-RT so that drivers no longer need to do handle this.

So we're breaking RT knowingly here? If this is the case I'm not happy
with your change. (And if RT is not affected a different wording would
be good.)

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |


Attachments:
(No filename) (1.07 kB)
signature.asc (499.00 B)
Download all attachments
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

On 2021-03-22 12:34:02 [+0100], Uwe Kleine-König wrote:
> On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> > Force-threaded interrupt handlers used to run with interrupts enabled,
> > something which could lead to deadlocks in case a threaded handler
> > shared a lock with code running in hard interrupt context (e.g. timer
> > callbacks) and did not explicitly disable interrupts.
> >
> > This was specifically the case for serial drivers that take the port
> > lock in their console write path as printk can be called from hard
> > interrupt context also with forced threading ("threadirqs").
> >
> > Since commit 81e2073c175b ("genirq: Disable interrupts for force
> > threaded handlers") interrupt handlers always run with interrupts
> > disabled on non-RT so that drivers no longer need to do handle this.
>
> So we're breaking RT knowingly here? If this is the case I'm not happy
> with your change. (And if RT is not affected a different wording would
> be good.)

Which wording, could you be more specific? It looks good from here and
no, RT is not affected.

> Best regards
> Uwe

Sebastian

2021-03-22 11:57:38

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

Hallo Sebastian,

On Mon, Mar 22, 2021 at 12:39:18PM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-03-22 12:34:02 [+0100], Uwe Kleine-K?nig wrote:
> > On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> > > Force-threaded interrupt handlers used to run with interrupts enabled,
> > > something which could lead to deadlocks in case a threaded handler
> > > shared a lock with code running in hard interrupt context (e.g. timer
> > > callbacks) and did not explicitly disable interrupts.
> > >
> > > This was specifically the case for serial drivers that take the port
> > > lock in their console write path as printk can be called from hard
> > > interrupt context also with forced threading ("threadirqs").
> > >
> > > Since commit 81e2073c175b ("genirq: Disable interrupts for force
> > > threaded handlers") interrupt handlers always run with interrupts
> > > disabled on non-RT so that drivers no longer need to do handle this.
> >
> > So we're breaking RT knowingly here? If this is the case I'm not happy
> > with your change. (And if RT is not affected a different wording would
> > be good.)
>
> Which wording, could you be more specific? It looks good from here and
> no, RT is not affected.

The commit log says essentially: "The change is fine on non-RT" which
suggests there is a problem on RT. So something like:

On non-RT interrupts are disabled also for force threaded handlers
(since commit 81e2073c175b ...). On RT there is no problem either
because ... So we don't need to handle this case in the driver any more.

would be preferable.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |


Attachments:
(No filename) (1.75 kB)
signature.asc (499.00 B)
Download all attachments

2021-03-22 13:25:32

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

On Mon, Mar 22, 2021 at 12:55:36PM +0100, Uwe Kleine-König wrote:
> Hallo Sebastian,
>
> On Mon, Mar 22, 2021 at 12:39:18PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2021-03-22 12:34:02 [+0100], Uwe Kleine-König wrote:
> > > On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> > > > Force-threaded interrupt handlers used to run with interrupts enabled,
> > > > something which could lead to deadlocks in case a threaded handler
> > > > shared a lock with code running in hard interrupt context (e.g. timer
> > > > callbacks) and did not explicitly disable interrupts.
> > > >
> > > > This was specifically the case for serial drivers that take the port
> > > > lock in their console write path as printk can be called from hard
> > > > interrupt context also with forced threading ("threadirqs").
> > > >
> > > > Since commit 81e2073c175b ("genirq: Disable interrupts for force
> > > > threaded handlers") interrupt handlers always run with interrupts
> > > > disabled on non-RT so that drivers no longer need to do handle this.
> > >
> > > So we're breaking RT knowingly here? If this is the case I'm not happy
> > > with your change. (And if RT is not affected a different wording would
> > > be good.)
> >
> > Which wording, could you be more specific? It looks good from here and
> > no, RT is not affected.
>
> The commit log says essentially: "The change is fine on non-RT" which
> suggests there is a problem on RT.

I don't think you can read that into the commit message. Why would I
knowingly break RT? And why would RT depend on patching individual
serial drivers in the first place?

That interrupt handlers run with interrupts enabled in RT is kind of
given. The unfortunate (buggy) bit was that they weren't always disabled
for non-RT.

Johan


Attachments:
(No filename) (1.78 kB)
signature.asc (235.00 B)
Download all attachments

2021-03-22 13:42:31

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

Hello Johan,

On Mon, Mar 22, 2021 at 02:20:57PM +0100, Johan Hovold wrote:
> On Mon, Mar 22, 2021 at 12:55:36PM +0100, Uwe Kleine-K?nig wrote:
> > On Mon, Mar 22, 2021 at 12:39:18PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2021-03-22 12:34:02 [+0100], Uwe Kleine-K?nig wrote:
> > > > On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> > > > > Force-threaded interrupt handlers used to run with interrupts enabled,
> > > > > something which could lead to deadlocks in case a threaded handler
> > > > > shared a lock with code running in hard interrupt context (e.g. timer
> > > > > callbacks) and did not explicitly disable interrupts.
> > > > >
> > > > > This was specifically the case for serial drivers that take the port
> > > > > lock in their console write path as printk can be called from hard
> > > > > interrupt context also with forced threading ("threadirqs").
> > > > >
> > > > > Since commit 81e2073c175b ("genirq: Disable interrupts for force
> > > > > threaded handlers") interrupt handlers always run with interrupts
> > > > > disabled on non-RT so that drivers no longer need to do handle this.
> > > >
> > > > So we're breaking RT knowingly here? If this is the case I'm not happy
> > > > with your change. (And if RT is not affected a different wording would
> > > > be good.)
> > >
> > > Which wording, could you be more specific? It looks good from here and
> > > no, RT is not affected.
> >
> > The commit log says essentially: "The change is fine on non-RT" which
> > suggests there is a problem on RT.
>
> I don't think you can read that into the commit message.

From a strictly logically point of view you indeed cannot. But if you go
to the street and say to people there that they can park their car in
this street free of charge between Monday and Friday, I expect that most
of them will assume that they have to pay for parking on weekends.

So when you said that on on-RT the reason why it used to need a
workaround is gone made me wonder what that implies for RT.

> Why would I knowingly break RT?

No offence intended. Priorities are different for different maintainers
and I wouldn't bet that all maintainers care for RT.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |


Attachments:
(No filename) (2.35 kB)
signature.asc (499.00 B)
Download all attachments
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

On 2021-03-22 14:40:32 [+0100], Uwe Kleine-König wrote:
> From a strictly logically point of view you indeed cannot. But if you go
> to the street and say to people there that they can park their car in
> this street free of charge between Monday and Friday, I expect that most
> of them will assume that they have to pay for parking on weekends.

If I hear that parking is free on weekdays and on paid on weekends, I
expect it to be a scam.

Uwe, the patch reverts a change which was needed for !RT + threadirqs.
The commit message claims that since the referenced commit "… interrupt
handlers always run with interrupts disabled on non-RT… ". This has
nothing to do with _this_ change. It argues why the workaround is not
needed.
If the referenced commit breaks RT then this is another story.

> So when you said that on on-RT the reason why it used to need a
> workaround is gone made me wonder what that implies for RT.

There was never reason (or a lockdep splat) for it on RT. If so you
should have seen it, right?

Sebastian

2021-03-23 07:36:50

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

Hello Sebastian,

On Mon, Mar 22, 2021 at 09:48:36PM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-03-22 14:40:32 [+0100], Uwe Kleine-König wrote:
> > From a strictly logically point of view you indeed cannot. But if you go
> > to the street and say to people there that they can park their car in
> > this street free of charge between Monday and Friday, I expect that most
> > of them will assume that they have to pay for parking on weekends.
>
> If I hear that parking is free on weekdays and on paid on weekends, I
> expect it to be a scam.

I don't feel taken seriously with this reply.

> Uwe, the patch reverts a change which was needed for !RT + threadirqs.

This would be a useful information for the commit log.

> The commit message claims that since the referenced commit "… interrupt
> handlers always run with interrupts disabled on non-RT… ". This has
> nothing to do with _this_ change. It argues why the workaround is not
> needed.

It argues why the work around is not needed on non-RT. It might be
obvious for someone who is firm in the RT concepts, but IMHO commit logs
should be understandable by and make sense for a wider audience than the
deep experts. From what I know about RT "Force-threaded interrupt
handlers used to run with interrupts enabled" still applies there.

> If the referenced commit breaks RT then this is another story.

I'm surprised to hear that from you. With the goal to get RT into
mainline I would expect you to be happy if people consider the effects
on RT in their reviews.

> > So when you said that on on-RT the reason why it used to need a
> > workaround is gone made me wonder what that implies for RT.
>
> There was never reason (or a lockdep splat) for it on RT. If so you
> should have seen it, right?

No, I don't consider myself to be an RT expert who is aware of all the
problems. So I admit that for me the effect on RT of the patch under
discussion isn't obvious. I just wonder that the change is justified
with being OK on non-RT. So it's either bad that it breaks RT *or*
improving the commit log would be great.

And even if I had reason to believe that there is no problem with the
commit on RT, I'd still wish that the commit log wouldn't suggest to the
casual reader that there might be a problem.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |


Attachments:
(No filename) (2.45 kB)
signature.asc (499.00 B)
Download all attachments
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

On 2021-03-23 08:34:47 [+0100], Uwe Kleine-König wrote:
> Hello Sebastian,
Hi Uwe,

> On Mon, Mar 22, 2021 at 09:48:36PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2021-03-22 14:40:32 [+0100], Uwe Kleine-König wrote:
> > > From a strictly logically point of view you indeed cannot. But if you go
> > > to the street and say to people there that they can park their car in
> > > this street free of charge between Monday and Friday, I expect that most
> > > of them will assume that they have to pay for parking on weekends.
> >
> > If I hear that parking is free on weekdays and on paid on weekends, I
> > expect it to be a scam.
>
> I don't feel taken seriously with this reply.

I'm sorry.

> > Uwe, the patch reverts a change which was needed for !RT + threadirqs.
>
> This would be a useful information for the commit log.
>
> > The commit message claims that since the referenced commit "… interrupt
> > handlers always run with interrupts disabled on non-RT… ". This has
> > nothing to do with _this_ change. It argues why the workaround is not
> > needed.
>
> It argues why the work around is not needed on non-RT. It might be
> obvious for someone who is firm in the RT concepts, but IMHO commit logs
> should be understandable by and make sense for a wider audience than the
> deep experts. From what I know about RT "Force-threaded interrupt
> handlers used to run with interrupts enabled" still applies there.

Yes. The commit Johan referenced explains it in more detail.

> > If the referenced commit breaks RT then this is another story.
>
> I'm surprised to hear that from you. With the goal to get RT into
> mainline I would expect you to be happy if people consider the effects
> on RT in their reviews.

Correct, I do and I am glad if people consider other aspects of the
kernel in their review including RT.

> > > So when you said that on on-RT the reason why it used to need a
> > > workaround is gone made me wonder what that implies for RT.
> >
> > There was never reason (or a lockdep splat) for it on RT. If so you
> > should have seen it, right?
>
> No, I don't consider myself to be an RT expert who is aware of all the
> problems. So I admit that for me the effect on RT of the patch under
> discussion isn't obvious. I just wonder that the change is justified
> with being OK on non-RT. So it's either bad that it breaks RT *or*
> improving the commit log would be great.
>
> And even if I had reason to believe that there is no problem with the
> commit on RT, I'd still wish that the commit log wouldn't suggest to the
> casual reader that there might be a problem.

Okay. I added a sentence. What about this rewording:

Force-threaded interrupt handlers used to run with interrupts enabled,
something which could lead to deadlocks in case a threaded handler
shared a lock with code running in hard interrupt context (e.g. timer
callbacks) and did not explicitly disable interrupts.

This was specifically the case for serial drivers that take the port
lock in their console write path as printk can be called from hard
interrupt context also with forced threading ("threadirqs").

Since commit 81e2073c175b ("genirq: Disable interrupts for force
threaded handlers") interrupt handlers always run with interrupts
disabled on non-RT so that drivers no longer need to do handle this.
RT is not affected by the referenced commit and the workaround, that is
reverted, was not required because spinlock_t must not be acquired on
RT in hardirq context.

Drop the now obsolete workaround added by commit 33f16855dcb9 ("tty:
serial: imx: fix potential deadlock").

> Best regards
> Uwe
>

Sebastian

2021-03-23 14:40:15

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

On Mon, Mar 22, 2021 at 02:40:32PM +0100, Uwe Kleine-König wrote:
> Hello Johan,
>
> On Mon, Mar 22, 2021 at 02:20:57PM +0100, Johan Hovold wrote:
> > On Mon, Mar 22, 2021 at 12:55:36PM +0100, Uwe Kleine-König wrote:
> > > On Mon, Mar 22, 2021 at 12:39:18PM +0100, Sebastian Andrzej Siewior wrote:
> > > > On 2021-03-22 12:34:02 [+0100], Uwe Kleine-König wrote:
> > > > > On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> > > > > > Force-threaded interrupt handlers used to run with interrupts enabled,
> > > > > > something which could lead to deadlocks in case a threaded handler
> > > > > > shared a lock with code running in hard interrupt context (e.g. timer
> > > > > > callbacks) and did not explicitly disable interrupts.
> > > > > >
> > > > > > This was specifically the case for serial drivers that take the port
> > > > > > lock in their console write path as printk can be called from hard
> > > > > > interrupt context also with forced threading ("threadirqs").
> > > > > >
> > > > > > Since commit 81e2073c175b ("genirq: Disable interrupts for force
> > > > > > threaded handlers") interrupt handlers always run with interrupts
> > > > > > disabled on non-RT so that drivers no longer need to do handle this.
> > > > >
> > > > > So we're breaking RT knowingly here? If this is the case I'm not happy
> > > > > with your change. (And if RT is not affected a different wording would
> > > > > be good.)
> > > >
> > > > Which wording, could you be more specific? It looks good from here and
> > > > no, RT is not affected.
> > >
> > > The commit log says essentially: "The change is fine on non-RT" which
> > > suggests there is a problem on RT.
> >
> > I don't think you can read that into the commit message.
>
> From a strictly logically point of view you indeed cannot. But if you go
> to the street and say to people there that they can park their car in
> this street free of charge between Monday and Friday, I expect that most
> of them will assume that they have to pay for parking on weekends.

That analogy would almost seem to suggest bad intent on my side.

To say that this workaround is no longer needed on !RT does not imply
that it is needed on RT. If anything it suggests I have considered RT,
I'd say.

> So when you said that on on-RT the reason why it used to need a
> workaround is gone made me wonder what that implies for RT.

Fair enough. I thought it was obvious from the commit message and the
commits referred to, and that RT wouldn't depend on patching random
mainline drivers like this without a clear marking such as using raw
spin locks.

Greg's already picked this one up and this is hopefully the last one
we'll see of these.

Johan


Attachments:
(No filename) (2.69 kB)
signature.asc (235.00 B)
Download all attachments

2021-03-25 02:57:56

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

Hello Sebastian,

On Tue, Mar 23, 2021 at 10:04:13AM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-03-23 08:34:47 [+0100], Uwe Kleine-König wrote:
> > On Mon, Mar 22, 2021 at 09:48:36PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2021-03-22 14:40:32 [+0100], Uwe Kleine-König wrote:
> > > > From a strictly logically point of view you indeed cannot. But if you go
> > > > to the street and say to people there that they can park their car in
> > > > this street free of charge between Monday and Friday, I expect that most
> > > > of them will assume that they have to pay for parking on weekends.
> > >
> > > Uwe, the patch reverts a change which was needed for !RT + threadirqs.
> >
> > This would be a useful information for the commit log.
> >
> > > The commit message claims that since the referenced commit "… interrupt
> > > handlers always run with interrupts disabled on non-RT… ". This has
> > > nothing to do with _this_ change. It argues why the workaround is not
> > > needed.
> >
> > It argues why the work around is not needed on non-RT. It might be
> > obvious for someone who is firm in the RT concepts, but IMHO commit logs
> > should be understandable by and make sense for a wider audience than the
> > deep experts. From what I know about RT "Force-threaded interrupt
> > handlers used to run with interrupts enabled" still applies there.
>
> Yes. The commit Johan referenced explains it in more detail.

In my book the commit log should be understandable without reading the
referenced commits.

> > > If the referenced commit breaks RT then this is another story.
> >
> > I'm surprised to hear that from you. With the goal to get RT into
> > mainline I would expect you to be happy if people consider the effects
> > on RT in their reviews.
>
> Correct, I do and I am glad if people consider other aspects of the
> kernel in their review including RT.
>
> > > > So when you said that on on-RT the reason why it used to need a
> > > > workaround is gone made me wonder what that implies for RT.
> > >
> > > There was never reason (or a lockdep splat) for it on RT. If so you
> > > should have seen it, right?
> >
> > No, I don't consider myself to be an RT expert who is aware of all the
> > problems. So I admit that for me the effect on RT of the patch under
> > discussion isn't obvious. I just wonder that the change is justified
> > with being OK on non-RT. So it's either bad that it breaks RT *or*
> > improving the commit log would be great.
> >
> > And even if I had reason to believe that there is no problem with the
> > commit on RT, I'd still wish that the commit log wouldn't suggest to the
> > casual reader that there might be a problem.
>
> Okay. I added a sentence. What about this rewording:
>
> Force-threaded interrupt handlers used to run with interrupts enabled,
> something which could lead to deadlocks in case a threaded handler
> shared a lock with code running in hard interrupt context (e.g. timer
> callbacks) and did not explicitly disable interrupts.
>
> This was specifically the case for serial drivers that take the port
> lock in their console write path as printk can be called from hard
> interrupt context also with forced threading ("threadirqs").
>
> Since commit 81e2073c175b ("genirq: Disable interrupts for force
> threaded handlers") interrupt handlers always run with interrupts
> disabled on non-RT so that drivers no longer need to do handle this.
> RT is not affected by the referenced commit and the workaround, that is
> reverted, was not required because spinlock_t must not be acquired on
> RT in hardirq context.
>
> Drop the now obsolete workaround added by commit 33f16855dcb9 ("tty:
> serial: imx: fix potential deadlock").

This resolves my concerns. Thanks
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |


Attachments:
(No filename) (3.96 kB)
signature.asc (499.00 B)
Download all attachments

2021-03-25 02:59:03

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH] serial: imx: drop workaround for forced irq threading

Hello Johan,

On Tue, Mar 23, 2021 at 03:37:29PM +0100, Johan Hovold wrote:
> On Mon, Mar 22, 2021 at 02:40:32PM +0100, Uwe Kleine-K?nig wrote:
> > On Mon, Mar 22, 2021 at 02:20:57PM +0100, Johan Hovold wrote:
> > > On Mon, Mar 22, 2021 at 12:55:36PM +0100, Uwe Kleine-K?nig wrote:
> > > > On Mon, Mar 22, 2021 at 12:39:18PM +0100, Sebastian Andrzej Siewior wrote:
> > > > > On 2021-03-22 12:34:02 [+0100], Uwe Kleine-K?nig wrote:
> > > > > > On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> > > > > > > Force-threaded interrupt handlers used to run with interrupts enabled,
> > > > > > > something which could lead to deadlocks in case a threaded handler
> > > > > > > shared a lock with code running in hard interrupt context (e.g. timer
> > > > > > > callbacks) and did not explicitly disable interrupts.
> > > > > > >
> > > > > > > This was specifically the case for serial drivers that take the port
> > > > > > > lock in their console write path as printk can be called from hard
> > > > > > > interrupt context also with forced threading ("threadirqs").
> > > > > > >
> > > > > > > Since commit 81e2073c175b ("genirq: Disable interrupts for force
> > > > > > > threaded handlers") interrupt handlers always run with interrupts
> > > > > > > disabled on non-RT so that drivers no longer need to do handle this.
> > > > > >
> > > > > > So we're breaking RT knowingly here? If this is the case I'm not happy
> > > > > > with your change. (And if RT is not affected a different wording would
> > > > > > be good.)
> > > > >
> > > > > Which wording, could you be more specific? It looks good from here and
> > > > > no, RT is not affected.
> > > >
> > > > The commit log says essentially: "The change is fine on non-RT" which
> > > > suggests there is a problem on RT.
> > >
> > > I don't think you can read that into the commit message.
> >
> > From a strictly logically point of view you indeed cannot. But if you go
> > to the street and say to people there that they can park their car in
> > this street free of charge between Monday and Friday, I expect that most
> > of them will assume that they have to pay for parking on weekends.
>
> That analogy would almost seem to suggest bad intent on my side.

That analogy's purpose was to put over my point that writing
(paraphrased) "Since non-RT changed, this workaround isn't necessary any
more" suggests to me that the change might be bad for RT. So again,
there was no harm intended, this is just a call for clearing up either
the commit log to make it obvious the change is right or to fix the
problem on RT if there is any.

> To say that this workaround is no longer needed on !RT does not imply
> that it is needed on RT. If anything it suggests I have considered RT,
> I'd say.

The code in question was used for both RT and non-RT. You drop it for
both cases and only justify one of them. OK, fine, you considered both
cases. Just from reading the commit log I considered you didn't. It's
completely ok for me to be wrong here, but I still think the chosen
words are not optimal and stumbling as I did is easy. So I still see a
potential to improve the wording.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |


Attachments:
(No filename) (3.31 kB)
signature.asc (499.00 B)
Download all attachments