Messages in this thread | | | From | Doug Anderson <> | Date | Thu, 18 Jun 2020 17:34:55 -0700 | Subject | Re: [PATCH v4 5/5] spi: spi-geni-qcom: Don't keep a local state variable |
| |
Hi,
On Thu, Jun 18, 2020 at 4:37 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Doug Anderson (2020-06-18 15:00:10) > > On Thu, Jun 18, 2020 at 2:52 PM Stephen Boyd <swboyd@chromium.org> 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
| |