* Re: [PATCH 0/2] tty: n_gsm: revert tx_mutex usage
[not found] <DB9PR10MB58817A08B92BA7F943CDD9B2E02E9@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>
@ 2022-10-24 14:37 ` Greg KH
0 siblings, 0 replies; 3+ messages in thread
From: Greg KH @ 2022-10-24 14:37 UTC (permalink / raw)
To: Starke, Daniel
Cc: pchelkin, jirislaby, Alexey Khoroshilov, linux-kernel,
lvc-project, Pavel Machek
On Mon, Oct 24, 2022 at 01:30:23PM +0000, Starke, Daniel wrote:
> I have checked both patches. Those appear to be correct.
> Furthermore, I also recommend to bring in these reverts.
Can you send a "Reviewed-by:" tag for me to apply? Hint, don't send it
in an HTML email so that lore.kernel.org will properly pick it up.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] tty: n_gsm: avoid call of sleeping functions from atomic context
@ 2022-10-08 10:54 Fedor Pchelkin
2022-10-08 11:02 ` [PATCH 0/2] tty: n_gsm: revert tx_mutex usage Fedor Pchelkin
0 siblings, 1 reply; 3+ messages in thread
From: Fedor Pchelkin @ 2022-10-08 10:54 UTC (permalink / raw)
To: Starke, Daniel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-kernel, Alexey Khoroshilov,
Pavel Machek, lvc-project
On 05.10.2022 13:47, Daniel Starke wrote:
> This patch breaks packet retransmission. Basically tx_lock and now
tx_mutex
> protects the transmission packet queue. This works fine as long as
packets
> are transmitted in a context that allows sleep. However, the
retransmission
> timer T2 is called from soft IRQ context and spans an additional atomic
> context via control_lock within gsm_control_retransmit(). The call path
> looks like this:
> gsm_control_retransmit()
> spin_lock_irqsave(&gsm->control_lock, flags)
> gsm_control_transmit()
> gsm_data_queue()
> mutex_lock(&gsm->tx_mutex) // -> sleep in atomic context
As far as switching to tx_mutex turns out to have its own problems,
we suggest to revert it and to find another solution for the original
issue.
As it is described in commit 32dd59f ("tty: n_gsm: fix race condition in
gsmld_write()"), the issue is that gsmld_write() may be used by the user
directly and also by the n_gsm internal functions. But the proposed
solution to add a spinlock around the low side tty write is not suitable
since the tty write may sleep:
gsmld_write(...)
spin_lock_irqsave(&gsm->tx_lock, flags)
tty->ops->write(...);
con_write(...)
do_con_write(...)
console_lock()
might_sleep() // -> bug
So let's consider alternative approaches to avoid the race condition.
We have found the only potential concurrency place:
gsm->tty->ops->write() in gsmld_output() and tty->ops->write() in
gsmld_write().
Is that right? Or there are some other cases?
On 05.10.2022 13:47, Daniel Starke wrote:
> Long story short: The patch via mutex does not solve the issue. It is
only
> shifted to another function. I suggest splitting the TX lock into packet
> queue lock and underlying tty write mutex.
>
> I would have implemented the patch if I had means to verify it.
Probably splitting the TX lock would be rather complex as there is
gsm_data_kick() which in this way has to be protected by packet queue
spinlock and at the same time it contains gsmld_output() (via
gsm_send_packet()) that would require mutex protection.
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 0/2] tty: n_gsm: revert tx_mutex usage
2022-10-08 10:54 [PATCH v2] tty: n_gsm: avoid call of sleeping functions from atomic context Fedor Pchelkin
@ 2022-10-08 11:02 ` Fedor Pchelkin
2022-10-25 7:31 ` Pavel Machek
0 siblings, 1 reply; 3+ messages in thread
From: Fedor Pchelkin @ 2022-10-08 11:02 UTC (permalink / raw)
To: Daniel Starke
Cc: Fedor Pchelkin, Jiri Slaby, lvc-project, Alexey Khoroshilov,
Greg Kroah-Hartman, linux-kernel, Pavel Machek
As far as switching to tx_mutex turns out to have its own problems,
we suggest to revert it and to find another solution for the original
issue described in 902e02ea9385 ("tty: n_gsm: avoid call of sleeping
functions from atomic context").
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 0/2] tty: n_gsm: revert tx_mutex usage
2022-10-08 11:02 ` [PATCH 0/2] tty: n_gsm: revert tx_mutex usage Fedor Pchelkin
@ 2022-10-25 7:31 ` Pavel Machek
0 siblings, 0 replies; 3+ messages in thread
From: Pavel Machek @ 2022-10-25 7:31 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: Daniel Starke, Jiri Slaby, lvc-project, Alexey Khoroshilov,
Greg Kroah-Hartman, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 582 bytes --]
Hi!
> As far as switching to tx_mutex turns out to have its own problems,
> we suggest to revert it and to find another solution for the original
> issue described in 902e02ea9385 ("tty: n_gsm: avoid call of sleeping
> functions from atomic context").
We have lived with locking problems for a long time, and we really
don't want data corruption.
Reviewed-by: Pavel Machek <pavel@denx.de>
Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-25 7:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <DB9PR10MB58817A08B92BA7F943CDD9B2E02E9@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>
2022-10-24 14:37 ` [PATCH 0/2] tty: n_gsm: revert tx_mutex usage Greg KH
2022-10-08 10:54 [PATCH v2] tty: n_gsm: avoid call of sleeping functions from atomic context Fedor Pchelkin
2022-10-08 11:02 ` [PATCH 0/2] tty: n_gsm: revert tx_mutex usage Fedor Pchelkin
2022-10-25 7:31 ` Pavel Machek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).