[PATCH] serial: stm32: optimize spin lock usage

[PATCH] serial: stm32: optimize spin lock usage

[dillon.minfei]

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);
+
 	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

Re: [PATCH] serial: stm32: optimize spin lock usage

[Greg KH]

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

Re: [PATCH] serial: stm32: optimize spin lock usage

[dillon min]

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

Re: [PATCH] serial: stm32: optimize spin lock usage

[Greg KH]

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

Re: [PATCH] serial: stm32: optimize spin lock usage

[dillon min]

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

Re: [Linux-stm32] [PATCH] serial: stm32: optimize spin lock usage

[Erwan LE RAY]

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
> 

Re: [Linux-stm32] [PATCH] serial: stm32: optimize spin lock usage

[dillon min]

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
>>
>

Re: [PATCH] serial: stm32: optimize spin lock usage

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5016 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36623 bytes --]

Re: [PATCH] serial: stm32: optimize spin lock usage

[dillon min]

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