Message ID | 1577352088-35856-1-git-send-email-kong.kongxinwei@hisilicon.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | spi: dw: use "smp_mb()" to avoid sending spi data error | expand |
On Thu, Dec 26, 2019 at 05:21:28PM +0800, Xinwei Kong wrote: > this patch will add memory barrier to ensure this "struct dw_spi *dws" > to complete data setting before enabling this SPI hardware interrupt. > eg: > it will fix to this following low possibility error in testing environment > which using SPI control to connect TPM Modules > --- a/drivers/spi/spi-dw.c > +++ b/drivers/spi/spi-dw.c > @@ -288,6 +288,8 @@ static int dw_spi_transfer_one(struct spi_controller *master, > dws->rx_end = dws->rx + transfer->len; > dws->len = transfer->len; > > + smp_mb(); > + > spi_enable_chip(dws, 0); I'd be much more comfortable here if I understood what this was supposed to be syncing - what exactly gets flushed here and why is a memory barrier enough to ensure it's synced? A comment in the code would be especially good so anyone modifying the code understands this in future.
On 2019/12/27 8:22, Mark Brown wrote: > On Thu, Dec 26, 2019 at 05:21:28PM +0800, Xinwei Kong wrote: >> this patch will add memory barrier to ensure this "struct dw_spi *dws" >> to complete data setting before enabling this SPI hardware interrupt. >> eg: >> it will fix to this following low possibility error in testing environment >> which using SPI control to connect TPM Modules > >> --- a/drivers/spi/spi-dw.c >> +++ b/drivers/spi/spi-dw.c >> @@ -288,6 +288,8 @@ static int dw_spi_transfer_one(struct spi_controller *master, >> dws->rx_end = dws->rx + transfer->len; >> dws->len = transfer->len; >> >> + smp_mb(); >> + >> spi_enable_chip(dws, 0); > > I'd be much more comfortable here if I understood what this was > supposed to be syncing - what exactly gets flushed here and why > is a memory barrier enough to ensure it's synced? A comment in > the code would be especially good so anyone modifying the code > understands this in future. > Because of out-of-order execution about some CPU architecture, In this debug stage we find Completing spi interrupt enable -> prodrucing TXEI interrupt -> running "interrupt_transfer" function will prior to set "dw->rx and dws->rx_end" data, so it will result in SPI sending error
On Sat, Dec 28, 2019 at 04:31:53PM +0800, kongxinwei wrote: > > I'd be much more comfortable here if I understood what this was > > supposed to be syncing - what exactly gets flushed here and why > > is a memory barrier enough to ensure it's synced? A comment in > > the code would be especially good so anyone modifying the code > > understands this in future. > Because of out-of-order execution about some CPU architecture, > In this debug stage we find Completing spi interrupt enable -> > prodrucing TXEI interrupt -> running "interrupt_transfer" function > will prior to set "dw->rx and dws->rx_end" data, so it will result > in SPI sending error Could you update the commit message to say that, and ideally also add a comment saying something like "Ensure dw->rx and dw->rx_end are visible" please?
On 2020/1/3 9:04, Mark Brown wrote: > On Sat, Dec 28, 2019 at 04:31:53PM +0800, kongxinwei wrote: > >>> I'd be much more comfortable here if I understood what this was >>> supposed to be syncing - what exactly gets flushed here and why >>> is a memory barrier enough to ensure it's synced? A comment in >>> the code would be especially good so anyone modifying the code >>> understands this in future. > >> Because of out-of-order execution about some CPU architecture, >> In this debug stage we find Completing spi interrupt enable -> >> prodrucing TXEI interrupt -> running "interrupt_transfer" function >> will prior to set "dw->rx and dws->rx_end" data, so it will result >> in SPI sending error > > Could you update the commit message to say that, and ideally also > add a comment saying something like "Ensure dw->rx and dw->rx_end > are visible" please? > OK, i WILL update it.
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index a92aa5c..d0d77a2 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -288,6 +288,8 @@ static int dw_spi_transfer_one(struct spi_controller *master, dws->rx_end = dws->rx + transfer->len; dws->len = transfer->len; + smp_mb(); + spi_enable_chip(dws, 0); /* Handle per transfer options for bpw and speed */