Re: [Linux-stm32] [PATCH] serial: stm32: optimize spin lock usage
On 4/12/21, Erwan LE RAY wrote:
> Hi Dillon,
>
> Thanks for your patch.
>
> Could you please elaborate the use case in your commit message ?
Sorry, local_irq_save() plus spin_lock() same to spin_lock_irqsave()
There is no deadlock . Please ignore this patch.
Thanks
Dillon
>
> Best Regards, Erwan.
>
> On 4/12/21 10:54 AM, dillon min wrote:
>> Hi Greg,
>>
>> On Mon, Apr 12, 2021 at 4:25 PM Greg KH
>> wrote:
>>>
>>> On Mon, Apr 12, 2021 at 02:50:20PM +0800, dillon min wrote:
Hi Greg，
Thanks for the quick response, please ignore the last private mail.
On Mon, Apr 12, 2021 at 1:52 PM Greg KH
wrote:
>
> On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.min...@gmail.com
> wrote:
>> From: dillon min
>>
>> To avoid potential deadlock in spin_lock usage, change to use
>> spin_lock_irqsave(), spin_unlock_irqrestore() in process(thread_fn)
>> context.
>> spin_lock(), spin_unlock() under handler context.
>>
>> remove unused local_irq_save/restore call.
>>
>> Signed-off-by: dillon min
>> ---
>> Was verified on stm32f469-disco board. need more test on stm32mp
>> platform.
>>
>> drivers/tty/serial/stm32-usart.c | 27 +--
>> 1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/tty/serial/stm32-usart.c
>> b/drivers/tty/serial/stm32-usart.c
>> index b3675cf25a69..c4c859b34367 100644
>> --- a/drivers/tty/serial/stm32-usart.c
>> +++ b/drivers/tty/serial/stm32-usart.c
>> @@ -214,7 +214,7 @@ static void stm32_usart_receive_chars(struct
>> uart_port *port, bool threaded)
>>struct tty_port *tport = &port->state->port;
>>struct stm32_port *stm32_port = to_stm32_port(port);
>>const struct stm32_usart_offsets *ofs =
>> &stm32_port->info->ofs;
>> - unsigned long c;
>> + unsigned long c, flags;
>>u32 sr;
>>char flag;
>>
>> @@ -276,9 +276,17 @@ static void stm32_usart_receive_chars(struct
>> uart_port *port, bool threaded)
>>uart_insert_char(port, sr, USART_SR_ORE, c, flag);
>>}
>>
>> - spin_unlock(&port->lock);
>> + if (threaded)
>> + spin_unlock_irqrestore(&port->lock, flags);
>> + else
>> + spin_unlock(&port->lock);
>
> You shouldn't have to check for this, see the other patches on the
> list
> recently that fixed this up to not be an issue for irq handlers.
Can you help to give more hints, or the commit id of the patch which
fixed this. thanks.
I'm still confused with this.
The stm32_usart_threaded_interrupt() is a kthread context, once
port->lock holds by this function, another serial interrupts raised,
such as USART_SR_TXE,stm32_usart_interrupt() can't get the lock,
there will be a deadlock. isn't it?
So, shouldn't I use spin_lock{_irqsave} according to the caller's
context ?
>>>
>>> Please see 81e2073c175b ("genirq: Disable interrupts for force threaded
>>> handlers") for when threaded irq handlers have irqs disabled, isn't that
>>> the case you are trying to "protect" from here?
>>>
>>> Why is the "threaded" flag used at all? The driver should not care.
>>>
>>> Also see 9baedb7baeda ("serial: imx: drop workaround for forced irq
>>> threading") in linux-next for an example of how this was fixed up in a
>>> serial driver.
>>>
>>> does that help?
>>>
>> Yes, it's really helpful. and 81e2073c175b should be highlighted in a
>> doc.
>> In my past knowledge, we should care about hard irq & thread_fn lock
>> conflict.
>> This patch has totally avoided patching code in the separate driver side.
>> thanks.
>>
>> I will just keep the changes in stm32_usart_console_write(), remove
>> these code in
>> thread_fn. update version 2 for you.
>>
>> thanks.
>>
>> Dillon,
>>> thanks,
>>>
>>> greg k-h
>> ___
>> Linux-stm32 mailing list
>> linux-st...@st-md-mailman.stormreply.com
>> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
>>
>
Re: [Linux-stm32] [PATCH] serial: stm32: optimize spin lock usage
Hi Dillon,
Thanks for your patch.
Could you please elaborate the use case in your commit message ?
Best Regards, Erwan.
On 4/12/21 10:54 AM, dillon min wrote:
Hi Greg,
On Mon, Apr 12, 2021 at 4:25 PM Greg KH wrote:
On Mon, Apr 12, 2021 at 02:50:20PM +0800, dillon min wrote:
Hi Greg，
Thanks for the quick response, please ignore the last private mail.
On Mon, Apr 12, 2021 at 1:52 PM Greg KH wrote:
On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.min...@gmail.com wrote:
From: dillon min
To avoid potential deadlock in spin_lock usage, change to use
spin_lock_irqsave(), spin_unlock_irqrestore() in process(thread_fn) context.
spin_lock(), spin_unlock() under handler context.
remove unused local_irq_save/restore call.
Signed-off-by: dillon min
---
Was verified on stm32f469-disco board. need more test on stm32mp platform.
drivers/tty/serial/stm32-usart.c | 27 +--
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index b3675cf25a69..c4c859b34367 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -214,7 +214,7 @@ static void stm32_usart_receive_chars(struct uart_port
*port, bool threaded)
struct tty_port *tport = &port->state->port;
struct stm32_port *stm32_port = to_stm32_port(port);
const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
- unsigned long c;
+ unsigned long c, flags;
u32 sr;
char flag;
@@ -276,9 +276,17 @@ static void stm32_usart_receive_chars(struct uart_port
*port, bool threaded)
uart_insert_char(port, sr, USART_SR_ORE, c, flag);
}
- spin_unlock(&port->lock);
+ if (threaded)
+ spin_unlock_irqrestore(&port->lock, flags);
+ else
+ spin_unlock(&port->lock);
You shouldn't have to check for this, see the other patches on the list
recently that fixed this up to not be an issue for irq handlers.
Can you help to give more hints, or the commit id of the patch which
fixed this. thanks.
I'm still confused with this.
The stm32_usart_threaded_interrupt() is a kthread context, once
port->lock holds by this function, another serial interrupts raised,
such as USART_SR_TXE,stm32_usart_interrupt() can't get the lock,
there will be a deadlock. isn't it?
So, shouldn't I use spin_lock{_irqsave} according to the caller's context ?
Please see 81e2073c175b ("genirq: Disable interrupts for force threaded
handlers") for when threaded irq handlers have irqs disabled, isn't that
the case you are trying to "protect" from here?
Why is the "threaded" flag used at all? The driver should not care.
Also see 9baedb7baeda ("serial: imx: drop workaround for forced irq
threading") in linux-next for an example of how this was fixed up in a
serial driver.
does that help?
Yes, it's really helpful. and 81e2073c175b should be highlighted in a doc.
In my past knowledge, we should care about hard irq & thread_fn lock conflict.
This patch has totally avoided patching code in the separate driver side.
thanks.
I will just keep the changes in stm32_usart_console_write(), remove
these code in
thread_fn. update version 2 for you.
thanks.
Dillon,
thanks,
greg k-h
___
Linux-stm32 mailing list
linux-st...@st-md-mailman.stormreply.com
https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
Re: [PATCH] serial: stm32: optimize spin lock usage
Hi Greg,
On Mon, Apr 12, 2021 at 4:25 PM Greg KH wrote:
>
> On Mon, Apr 12, 2021 at 02:50:20PM +0800, dillon min wrote:
> > Hi Greg，
> >
> > Thanks for the quick response, please ignore the last private mail.
> >
> > On Mon, Apr 12, 2021 at 1:52 PM Greg KH wrote:
> > >
> > > On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.min...@gmail.com wrote:
> > > > From: dillon min
> > > >
> > > > To avoid potential deadlock in spin_lock usage, change to use
> > > > spin_lock_irqsave(), spin_unlock_irqrestore() in process(thread_fn)
> > > > context.
> > > > spin_lock(), spin_unlock() under handler context.
> > > >
> > > > remove unused local_irq_save/restore call.
> > > >
> > > > Signed-off-by: dillon min
> > > > ---
> > > > Was verified on stm32f469-disco board. need more test on stm32mp
> > > > platform.
> > > >
> > > > drivers/tty/serial/stm32-usart.c | 27 +--
> > > > 1 file changed, 17 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/stm32-usart.c
> > > > b/drivers/tty/serial/stm32-usart.c
> > > > index b3675cf25a69..c4c859b34367 100644
> > > > --- a/drivers/tty/serial/stm32-usart.c
> > > > +++ b/drivers/tty/serial/stm32-usart.c
> > > > @@ -214,7 +214,7 @@ static void stm32_usart_receive_chars(struct
> > > > uart_port *port, bool threaded)
> > > > struct tty_port *tport = &port->state->port;
> > > > struct stm32_port *stm32_port = to_stm32_port(port);
> > > > const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> > > > - unsigned long c;
> > > > + unsigned long c, flags;
> > > > u32 sr;
> > > > char flag;
> > > >
> > > > @@ -276,9 +276,17 @@ static void stm32_usart_receive_chars(struct
> > > > uart_port *port, bool threaded)
> > > > uart_insert_char(port, sr, USART_SR_ORE, c, flag);
> > > > }
> > > >
> > > > - spin_unlock(&port->lock);
> > > > + if (threaded)
> > > > + spin_unlock_irqrestore(&port->lock, flags);
> > > > + else
> > > > + spin_unlock(&port->lock);
> > >
> > > You shouldn't have to check for this, see the other patches on the list
> > > recently that fixed this up to not be an issue for irq handlers.
> > Can you help to give more hints, or the commit id of the patch which
> > fixed this. thanks.
> >
> > I'm still confused with this.
> >
> > The stm32_usart_threaded_interrupt() is a kthread context, once
> > port->lock holds by this function, another serial interrupts raised,
> > such as USART_SR_TXE,stm32_usart_interrupt() can't get the lock,
> > there will be a deadlock. isn't it?
> >
> > So, shouldn't I use spin_lock{_irqsave} according to the caller's context ?
>
> Please see 81e2073c175b ("genirq: Disable interrupts for force threaded
> handlers") for when threaded irq handlers have irqs disabled, isn't that
> the case you are trying to "protect" from here?
>
> Why is the "threaded" flag used at all? The driver should not care.
>
> Also see 9baedb7baeda ("serial: imx: drop workaround for forced irq
> threading") in linux-next for an example of how this was fixed up in a
> serial driver.
>
> does that help?
>
Yes, it's really helpful. and 81e2073c175b should be highlighted in a doc.
In my past knowledge, we should care about hard irq & thread_fn lock conflict.
This patch has totally avoided patching code in the separate driver side.
thanks.
I will just keep the changes in stm32_usart_console_write(), remove
these code in
thread_fn. update version 2 for you.
thanks.
Dillon,
> thanks,
>
> greg k-h
Re: [PATCH] serial: stm32: optimize spin lock usage
On Mon, Apr 12, 2021 at 02:50:20PM +0800, dillon min wrote:
> Hi Greg，
>
> Thanks for the quick response, please ignore the last private mail.
>
> On Mon, Apr 12, 2021 at 1:52 PM Greg KH wrote:
> >
> > On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.min...@gmail.com wrote:
> > > From: dillon min
> > >
> > > To avoid potential deadlock in spin_lock usage, change to use
> > > spin_lock_irqsave(), spin_unlock_irqrestore() in process(thread_fn)
> > > context.
> > > spin_lock(), spin_unlock() under handler context.
> > >
> > > remove unused local_irq_save/restore call.
> > >
> > > Signed-off-by: dillon min
> > > ---
> > > Was verified on stm32f469-disco board. need more test on stm32mp platform.
> > >
> > > drivers/tty/serial/stm32-usart.c | 27 +--
> > > 1 file changed, 17 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/stm32-usart.c
> > > b/drivers/tty/serial/stm32-usart.c
> > > index b3675cf25a69..c4c859b34367 100644
> > > --- a/drivers/tty/serial/stm32-usart.c
> > > +++ b/drivers/tty/serial/stm32-usart.c
> > > @@ -214,7 +214,7 @@ static void stm32_usart_receive_chars(struct
> > > uart_port *port, bool threaded)
> > > struct tty_port *tport = &port->state->port;
> > > struct stm32_port *stm32_port = to_stm32_port(port);
> > > const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> > > - unsigned long c;
> > > + unsigned long c, flags;
> > > u32 sr;
> > > char flag;
> > >
> > > @@ -276,9 +276,17 @@ static void stm32_usart_receive_chars(struct
> > > uart_port *port, bool threaded)
> > > uart_insert_char(port, sr, USART_SR_ORE, c, flag);
> > > }
> > >
> > > - spin_unlock(&port->lock);
> > > + if (threaded)
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > + else
> > > + spin_unlock(&port->lock);
> >
> > You shouldn't have to check for this, see the other patches on the list
> > recently that fixed this up to not be an issue for irq handlers.
> Can you help to give more hints, or the commit id of the patch which
> fixed this. thanks.
>
> I'm still confused with this.
>
> The stm32_usart_threaded_interrupt() is a kthread context, once
> port->lock holds by this function, another serial interrupts raised,
> such as USART_SR_TXE,stm32_usart_interrupt() can't get the lock,
> there will be a deadlock. isn't it?
>
> So, shouldn't I use spin_lock{_irqsave} according to the caller's context ?
Please see 81e2073c175b ("genirq: Disable interrupts for force threaded
handlers") for when threaded irq handlers have irqs disabled, isn't that
the case you are trying to "protect" from here?
Why is the "threaded" flag used at all? The driver should not care.
Also see 9baedb7baeda ("serial: imx: drop workaround for forced irq
threading") in linux-next for an example of how this was fixed up in a
serial driver.
does that help?
thanks,
greg k-h
Re: [PATCH] serial: stm32: optimize spin lock usage
Hi lkp,
Thanks for the reminder.
I just noticed this from greg's mail. still waiting for his response
about the proper solution for this patch.
After that i will update version 2 for it.
Your test is really efficient, thanks.
Best regards,
Dillon.
On Mon, Apr 12, 2021 at 3:25 PM kernel test robot wrote:
>
> Hi,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on stm32/stm32-next]
> [also build test WARNING on usb/usb-testing v5.12-rc7]
> [cannot apply to tty/tty-testing next-20210409]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:
> https://github.com/0day-ci/linux/commits/dillon-minfei-gmail-com/serial-stm32-optimize-spin-lock-usage/20210412-123607
> base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git
> stm32-next
> config: riscv-randconfig-r034-20210412 (attached as .config)
> compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
> 9829f5e6b1bca9b61efc629770d28bb9014dec45)
> reproduce (this is a W=1 build):
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install riscv cross compiling tool for clang build
> # apt-get install binutils-riscv64-linux-gnu
> #
> https://github.com/0day-ci/linux/commit/a0e81ae10c46f768437d61cd3a3dfd4d1250b375
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review
> dillon-minfei-gmail-com/serial-stm32-optimize-spin-lock-usage/20210412-123607
> git checkout a0e81ae10c46f768437d61cd3a3dfd4d1250b375
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/tty/serial/stm32-usart.c:280:39: warning: variable 'flags' is
> >> uninitialized when used here [-Wuninitialized]
>spin_unlock_irqrestore(&port->lock, flags);
>^
>drivers/tty/serial/stm32-usart.c:217:24: note: initialize the variable
> 'flags' to silence this warning
>unsigned long c, flags;
> ^
> = 0
>1 warning generated.
>
>
> vim +/flags +280 drivers/tty/serial/stm32-usart.c
>
>211
>212 static void stm32_usart_receive_chars(struct uart_port *port, bool
> threaded)
>213 {
>214 struct tty_port *tport = &port->state->port;
>215 struct stm32_port *stm32_port = to_stm32_port(port);
>216 const struct stm32_usart_offsets *ofs =
> &stm32_port->info->ofs;
>217 unsigned long c, flags;
>218 u32 sr;
>219 char flag;
>220
>221 if (irqd_is_wakeup_set(irq_get_irq_data(port->irq)))
>222 pm_wakeup_event(tport->tty->dev, 0);
>223
>224 while (stm32_usart_pending_rx(port, &sr,
> &stm32_port->last_res,
>225threaded)) {
>226 sr |= USART_SR_DUMMY_RX;
>227 flag = TTY_NORMAL;
>228
>229 /*
>230 * Status bits has to be cleared before reading the
> RDR:
>231 * In FIFO mode, reading the RDR will pop the next
> data
>232 * (if any) along with its status bits into the SR.
>233 * Not doing so leads to misalignement between RDR
> and SR,
>234 * and clear status bits of the next rx data.
>235 *
>236 * Clear errors flags for stm32f7 and stm32h7
> compatible
>237 * devices. On stm32f4 compatible devices, the error
> bit is
>238 * cleared by the sequence [read SR - read DR].
>239 */
>240 if ((sr & USART_SR_ERR_MASK) && ofs->icr != UNDEF_REG)
>241 writel_relaxed(sr & USART_SR_ERR_MASK,
>242 port->membase + ofs->icr);
>243
>244 c = stm32_usart_get_char(port, &sr,
> &stm32_port->last_res);
>245 port->icount.rx++;
>246 if (sr & USART_SR_ERR_MASK) {
>247 if (sr & USART_SR_ORE) {
>248 port->icount.overrun++;
>249 } else if (sr & USART_SR_PE) {
>250 port->icount.parity++;
>251 } else if (sr & USART_SR_FE) {
>252
Re: [PATCH] serial: stm32: optimize spin lock usage
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on stm32/stm32-next]
[also build test WARNING on usb/usb-testing v5.12-rc7]
[cannot apply to tty/tty-testing next-20210409]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/dillon-minfei-gmail-com/serial-stm32-optimize-spin-lock-usage/20210412-123607
base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git
stm32-next
config: riscv-randconfig-r034-20210412 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
9829f5e6b1bca9b61efc629770d28bb9014dec45)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
#
https://github.com/0day-ci/linux/commit/a0e81ae10c46f768437d61cd3a3dfd4d1250b375
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
dillon-minfei-gmail-com/serial-stm32-optimize-spin-lock-usage/20210412-123607
git checkout a0e81ae10c46f768437d61cd3a3dfd4d1250b375
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/tty/serial/stm32-usart.c:280:39: warning: variable 'flags' is
>> uninitialized when used here [-Wuninitialized]
spin_unlock_irqrestore(&port->lock, flags);
^
drivers/tty/serial/stm32-usart.c:217:24: note: initialize the variable
'flags' to silence this warning
unsigned long c, flags;
^
= 0
1 warning generated.
vim +/flags +280 drivers/tty/serial/stm32-usart.c
211
212 static void stm32_usart_receive_chars(struct uart_port *port, bool
threaded)
213 {
214 struct tty_port *tport = &port->state->port;
215 struct stm32_port *stm32_port = to_stm32_port(port);
216 const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
217 unsigned long c, flags;
218 u32 sr;
219 char flag;
220
221 if (irqd_is_wakeup_set(irq_get_irq_data(port->irq)))
222 pm_wakeup_event(tport->tty->dev, 0);
223
224 while (stm32_usart_pending_rx(port, &sr, &stm32_port->last_res,
225threaded)) {
226 sr |= USART_SR_DUMMY_RX;
227 flag = TTY_NORMAL;
228
229 /*
230 * Status bits has to be cleared before reading the RDR:
231 * In FIFO mode, reading the RDR will pop the next data
232 * (if any) along with its status bits into the SR.
233 * Not doing so leads to misalignement between RDR and
SR,
234 * and clear status bits of the next rx data.
235 *
236 * Clear errors flags for stm32f7 and stm32h7 compatible
237 * devices. On stm32f4 compatible devices, the error
bit is
238 * cleared by the sequence [read SR - read DR].
239 */
240 if ((sr & USART_SR_ERR_MASK) && ofs->icr != UNDEF_REG)
241 writel_relaxed(sr & USART_SR_ERR_MASK,
242 port->membase + ofs->icr);
243
244 c = stm32_usart_get_char(port, &sr,
&stm32_port->last_res);
245 port->icount.rx++;
246 if (sr & USART_SR_ERR_MASK) {
247 if (sr & USART_SR_ORE) {
248 port->icount.overrun++;
249 } else if (sr & USART_SR_PE) {
250 port->icount.parity++;
251 } else if (sr & USART_SR_FE) {
252 /* Break detection if character is null
*/
253 if (!c) {
254 port->icount.brk++;
255 if (uart_handle_break(port))
256 continue;
257 } else {
258 port->icount.frame++;
259 }
260 }
Re: [PATCH] serial: stm32: optimize spin lock usage
Hi Greg，
Thanks for the quick response, please ignore the last private mail.
On Mon, Apr 12, 2021 at 1:52 PM Greg KH wrote:
>
> On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.min...@gmail.com wrote:
> > From: dillon min
> >
> > To avoid potential deadlock in spin_lock usage, change to use
> > spin_lock_irqsave(), spin_unlock_irqrestore() in process(thread_fn) context.
> > spin_lock(), spin_unlock() under handler context.
> >
> > remove unused local_irq_save/restore call.
> >
> > Signed-off-by: dillon min
> > ---
> > Was verified on stm32f469-disco board. need more test on stm32mp platform.
> >
> > drivers/tty/serial/stm32-usart.c | 27 +--
> > 1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/tty/serial/stm32-usart.c
> > b/drivers/tty/serial/stm32-usart.c
> > index b3675cf25a69..c4c859b34367 100644
> > --- a/drivers/tty/serial/stm32-usart.c
> > +++ b/drivers/tty/serial/stm32-usart.c
> > @@ -214,7 +214,7 @@ static void stm32_usart_receive_chars(struct uart_port
> > *port, bool threaded)
> > struct tty_port *tport = &port->state->port;
> > struct stm32_port *stm32_port = to_stm32_port(port);
> > const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> > - unsigned long c;
> > + unsigned long c, flags;
> > u32 sr;
> > char flag;
> >
> > @@ -276,9 +276,17 @@ static void stm32_usart_receive_chars(struct uart_port
> > *port, bool threaded)
> > uart_insert_char(port, sr, USART_SR_ORE, c, flag);
> > }
> >
> > - spin_unlock(&port->lock);
> > + if (threaded)
> > + spin_unlock_irqrestore(&port->lock, flags);
> > + else
> > + spin_unlock(&port->lock);
>
> You shouldn't have to check for this, see the other patches on the list
> recently that fixed this up to not be an issue for irq handlers.
Can you help to give more hints, or the commit id of the patch which
fixed this. thanks.
I'm still confused with this.
The stm32_usart_threaded_interrupt() is a kthread context, once
port->lock holds by this function, another serial interrupts raised,
such as USART_SR_TXE,stm32_usart_interrupt() can't get the lock,
there will be a deadlock. isn't it?
So, shouldn't I use spin_lock{_irqsave} according to the caller's context ?
There is a bug in this patch, the variable flags should be a global variable.
Thanks.
Dillon,
>
> thanks,
>
> greg k-h
Re: [PATCH] serial: stm32: optimize spin lock usage
On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.min...@gmail.com wrote: > From: dillon min > > To avoid potential deadlock in spin_lock usage, change to use > spin_lock_irqsave(), spin_unlock_irqrestore() in process(thread_fn) context. > spin_lock(), spin_unlock() under handler context. > > remove unused local_irq_save/restore call. > > Signed-off-by: dillon min > --- > Was verified on stm32f469-disco board. need more test on stm32mp platform. > > drivers/tty/serial/stm32-usart.c | 27 +-- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/drivers/tty/serial/stm32-usart.c > b/drivers/tty/serial/stm32-usart.c > index b3675cf25a69..c4c859b34367 100644 > --- a/drivers/tty/serial/stm32-usart.c > +++ b/drivers/tty/serial/stm32-usart.c > @@ -214,7 +214,7 @@ static void stm32_usart_receive_chars(struct uart_port > *port, bool threaded) > struct tty_port *tport = &port->state->port; > struct stm32_port *stm32_port = to_stm32_port(port); > const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs; > - unsigned long c; > + unsigned long c, flags; > u32 sr; > char flag; > > @@ -276,9 +276,17 @@ static void stm32_usart_receive_chars(struct uart_port > *port, bool threaded) > uart_insert_char(port, sr, USART_SR_ORE, c, flag); > } > > - spin_unlock(&port->lock); > + if (threaded) > + spin_unlock_irqrestore(&port->lock, flags); > + else > + spin_unlock(&port->lock); You shouldn't have to check for this, see the other patches on the list recently that fixed this up to not be an issue for irq handlers. thanks, greg k-h
[PATCH] serial: stm32: optimize spin lock usage
From: dillon min
To avoid potential deadlock in spin_lock usage, change to use
spin_lock_irqsave(), spin_unlock_irqrestore() in process(thread_fn) context.
spin_lock(), spin_unlock() under handler context.
remove unused local_irq_save/restore call.
Signed-off-by: dillon min
---
Was verified on stm32f469-disco board. need more test on stm32mp platform.
drivers/tty/serial/stm32-usart.c | 27 +--
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index b3675cf25a69..c4c859b34367 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -214,7 +214,7 @@ static void stm32_usart_receive_chars(struct uart_port
*port, bool threaded)
struct tty_port *tport = &port->state->port;
struct stm32_port *stm32_port = to_stm32_port(port);
const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
- unsigned long c;
+ unsigned long c, flags;
u32 sr;
char flag;
@@ -276,9 +276,17 @@ static void stm32_usart_receive_chars(struct uart_port
*port, bool threaded)
uart_insert_char(port, sr, USART_SR_ORE, c, flag);
}
- spin_unlock(&port->lock);
+ if (threaded)
+ spin_unlock_irqrestore(&port->lock, flags);
+ else
+ spin_unlock(&port->lock);
+
tty_flip_buffer_push(tport);
- spin_lock(&port->lock);
+
+ if (threaded)
+ spin_lock_irqsave(&port->lock, flags);
+ else
+ spin_lock(&port->lock);
}
static void stm32_usart_tx_dma_complete(void *arg)
@@ -489,13 +497,14 @@ static irqreturn_t stm32_usart_threaded_interrupt(int
irq, void *ptr)
{
struct uart_port *port = ptr;
struct stm32_port *stm32_port = to_stm32_port(port);
+ unsigned long flags;
- spin_lock(&port->lock);
+ spin_lock_irqsave(&port->lock, flags);
if (stm32_port->rx_ch)
stm32_usart_receive_chars(port, true);
- spin_unlock(&port->lock);
+ spin_unlock_irqrestore(&port->lock, flags);
return IRQ_HANDLED;
}
@@ -1354,13 +1363,12 @@ static void stm32_usart_console_write(struct console
*co, const char *s,
u32 old_cr1, new_cr1;
int locked = 1;
- local_irq_save(flags);
if (port->sysrq)
locked = 0;
else if (oops_in_progress)
- locked = spin_trylock(&port->lock);
+ locked = spin_trylock_irqsave(&port->lock, flags);
else
- spin_lock(&port->lock);
+ spin_lock_irqsave(&port->lock, flags);
/* Save and disable interrupts, enable the transmitter */
old_cr1 = readl_relaxed(port->membase + ofs->cr1);
@@ -1374,8 +1382,7 @@ static void stm32_usart_console_write(struct console *co,
const char *s,
writel_relaxed(old_cr1, port->membase + ofs->cr1);
if (locked)
- spin_unlock(&port->lock);
- local_irq_restore(flags);
+ spin_unlock_irqrestore(&port->lock, flags);
}
static int stm32_usart_console_setup(struct console *co, char *options)
--
2.7.4
