Re: [PATCH] spi: spi-dw: Add lock protect dw_spi rx/tx to prevent concurrent calls

Re: [PATCH] spi: spi-dw: Add lock protect dw_spi rx/tx to prevent concurrent calls

[Mark Brown]

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

On Thu, Dec 19, 2019 at 10:56:20PM +0800, wuxu.wu wrote:

> 	[ 1025.321405]  default_idle_call+0x30/0x4c
> 	[ 1025.321415]  do_idle+0x22c/0x300
> 	[ 1025.321417]  cpu_startup_entry+0x28/0x30
> 	[ 1025.321426]  secondary_start_kernel+0x17c/0x1c8
> ---
>  drivers/spi/spi-dw.c | 17 ++++++++++++++---
>  drivers/spi/spi-dw.h |  1 +

You haven't provided a signoff so I can't do anything with this, see
submitting-patches.rst for details on what this is and why it is
essential.  That also has some other information on formatting
submissions.

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative then it's
usually better to pull out the relevant sections.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: spi-dw: Add lock protect dw_spi rx/tx to prevent concurrent calls

[Mark Brown]

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

On Fri, Dec 27, 2019 at 11:51:09AM +0800, wuxu.wu wrote:

>  static void dw_writer(struct dw_spi *dws)
>  {
> -	u32 max = tx_max(dws);
> +	u32 max
>  	u16 txw = 0;

This is going to break the build, there's a missing ; in the
declaration of max.  Otherwise this looks good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: spi-dw: Add lock protect dw_spi rx/tx to prevent concurrent calls

[Mark Brown]

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

On Sat, Dec 21, 2019 at 11:48:19AM +0800, wuxu.wu wrote:
> 	dw_spi_irq()nd dw_spi_transfer_one concurrent calls.
> 
> 	I find a panic in dw_writer(): txw = *(u8 *)(dws->tx);

Please fix the indentation of your commit log, it's indented by a
tab - look at other patches people are posting for examples...

> 	[ 1025.321302] Call trace:
> 	[ 1025.321309]  machine_kexec+0x58/0x3e8
> 	[ 1025.321319]  __crash_kexec+0x98/0x148

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative then it's
usually better to pull out the relevant sections.

> 	[ 1025.321365]  dw_spi_irq+0x48/0x70
> 	[ 1025.321377]  __handle_irq_event_percpu+0x74/0x278
> 
> Signed-off-by: wuxu.wu <wuxu.wu@huawei.com>
> ---

The Signed-off-by here has the right indenttion for the
changelog.

>  static void dw_writer(struct dw_spi *dws)
>  {
> -	u32 max = tx_max(dws);
> +	u32 max
>  	u16 txw = 0;
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&dws->buf_lock, flags);

We shouldn't use spin_lock_irqsave() here since this is called
from interrupt context by interrupt_transfer() - we should use
plain old spin_lock() here.  Otherwise the code change looks
good, well spotted!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]