| Message ID | 1618202061-8243-1-git-send-email-dillon.minfei@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: stm32: optimize spin lock usage | expand |
On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.minfei@gmail.com wrote: > From: dillon min <dillon.minfei@gmail.com> > > 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 <dillon.minfei@gmail.com> > --- > 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
Hi Greg， Thanks for the quick response, please ignore the last private mail. On Mon, Apr 12, 2021 at 1:52 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.minfei@gmail.com wrote: > > From: dillon min <dillon.minfei@gmail.com> > > > > 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 <dillon.minfei@gmail.com> > > --- > > 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
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 <lkp@intel.com>
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,
225 threaded)) {
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 }
261
262 sr &= port->read_status_mask;
263
264 if (sr & USART_SR_PE) {
265 flag = TTY_PARITY;
266 } else if (sr & USART_SR_FE) {
267 if (!c)
268 flag = TTY_BREAK;
269 else
270 flag = TTY_FRAME;
271 }
272 }
273
274 if (uart_handle_sysrq_char(port, c))
275 continue;
276 uart_insert_char(port, sr, USART_SR_ORE, c, flag);
277 }
278
279 if (threaded)
> 280 spin_unlock_irqrestore(&port->lock, flags);
281 else
282 spin_unlock(&port->lock);
283
284 tty_flip_buffer_push(tport);
285
286 if (threaded)
287 spin_lock_irqsave(&port->lock, flags);
288 else
289 spin_lock(&port->lock);
290 }
291
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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 <lkp@intel.com> 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 <lkp@intel.com> > > 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, > 225 threaded)) { > 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 } > 261 > 262 sr &= port->read_status_mask; > 263 > 264 if (sr & USART_SR_PE) { > 265 flag = TTY_PARITY; > 266 } else if (sr & USART_SR_FE) { > 267 if (!c) > 268 flag = TTY_BREAK; > 269 else > 270 flag = TTY_FRAME; > 271 } > 272 } > 273 > 274 if (uart_handle_sysrq_char(port, c)) > 275 continue; > 276 uart_insert_char(port, sr, USART_SR_ORE, c, flag); > 277 } > 278 > 279 if (threaded) > > 280 spin_unlock_irqrestore(&port->lock, flags); > 281 else > 282 spin_unlock(&port->lock); > 283 > 284 tty_flip_buffer_push(tport); > 285 > 286 if (threaded) > 287 spin_lock_irqsave(&port->lock, flags); > 288 else > 289 spin_lock(&port->lock); > 290 } > 291 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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 <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.minfei@gmail.com wrote: > > > From: dillon min <dillon.minfei@gmail.com> > > > > > > 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 <dillon.minfei@gmail.com> > > > --- > > > 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
Hi Greg, On Mon, Apr 12, 2021 at 4:25 PM Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.minfei@gmail.com wrote: > > > > From: dillon min <dillon.minfei@gmail.com> > > > > > > > > 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 <dillon.minfei@gmail.com> > > > > --- > > > > 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
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 <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> wrote: >>>> >>>> On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.minfei@gmail.com wrote: >>>>> From: dillon min <dillon.minfei@gmail.com> >>>>> >>>>> 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 <dillon.minfei@gmail.com> >>>>> --- >>>>> 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-stm32@st-md-mailman.stormreply.com > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32 >
On 4/12/21, Erwan LE RAY <erwan.leray@foss.st.com> 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 <gregkh@linuxfoundation.org> >> 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 <gregkh@linuxfoundation.org> >>>> wrote: >>>>> >>>>> On Mon, Apr 12, 2021 at 12:34:21PM +0800, dillon.minfei@gmail.com >>>>> wrote: >>>>>> From: dillon min <dillon.minfei@gmail.com> >>>>>> >>>>>> 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 <dillon.minfei@gmail.com> >>>>>> --- >>>>>> 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-stm32@st-md-mailman.stormreply.com >> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32 >> >
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)