Hi, On Thu, Jun 18, 2020 at 4:37 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > Quoting Doug Anderson (2020-06-18 15:00:10) > > On Thu, Jun 18, 2020 at 2:52 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > > > > > -----8<---- > > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > > > index d8f03ffb8594..670f83793aa4 100644 > > > --- a/drivers/spi/spi-geni-qcom.c > > > +++ b/drivers/spi/spi-geni-qcom.c > > > @@ -121,6 +121,10 @@ static void handle_fifo_timeout(struct spi_master *spi, > > > spin_lock_irq(&mas->lock); > > > reinit_completion(&mas->cancel_done); > > > writel(0, se->base + SE_GENI_TX_WATERMARK_REG); > > > + /* > > > + * Make sure we don't finalize a spi transfer that timed out but > > > + * came in while cancelling. > > > + */ > > > mas->cur_xfer = NULL; > > > mas->tx_rem_bytes = mas->rx_rem_bytes = 0; > > > geni_se_cancel_m_cmd(se); > > > > Sure. It gets the point across, though > > spi_finalize_current_transfer() is actually pretty harmless if you > > call it while cancelling. It just calls a completion. I'd rather say > > something like "If we're here because the SPI controller was calling > > handle_err() then the transfer is done and we shouldn't hold onto it > > anymore". > > > > Agreed it's mostly harmless. I thought the concern was that 'cur_xfer' > may reference a freed piece of memory so it's best to remove ownership > of the pointer from here so that the irq handler doesn't try to finalize > a transfer that may no longer exist. "Shouldn't hold onto it anymore" > doesn't tell us why it shouldn't be held onto, leaving it to the reader > to figure out why, which isn't good. Right. The point is that 'cur_xfer' isn't valid anymore after handle_err() finishes so we shouldn't hold the pointer. I'm OK with your wording and am happy if Mark squashes it when he applies or I can send out a new version soon. -Doug
