| Message ID | 1497419551-21834-12-git-send-email-varada@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Varada, On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: > It's possible for a SPI transaction to complete and get another > interrupt and have it processed on the same spi_transfer before the > transfer_one can set it to NULL. > > This masks unexpected interrupts, so let's set the spi_transfer to > NULL in the interrupt once the transaction is done. So we can > properly detect these bad interrupts and print warning messages. > > Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org> > Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> > --- > drivers/spi/spi-qup.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > index bd53e82..1a2a9d9 100644 > --- a/drivers/spi/spi-qup.c > +++ b/drivers/spi/spi-qup.c > @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) > struct spi_qup *controller = dev_id; > struct spi_transfer *xfer; > u32 opflags, qup_err, spi_err; > - unsigned long flags; > int error = 0; > + bool done = 0; > > - spin_lock_irqsave(&controller->lock, flags); > + spin_lock(&controller->lock); > xfer = controller->xfer; > controller->xfer = NULL; > - spin_unlock_irqrestore(&controller->lock, flags); > + spin_unlock(&controller->lock); Why change the locking here ? > > qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); > spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); > @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) > spi_qup_write(controller, xfer); > } > > - spin_lock_irqsave(&controller->lock, flags); > - controller->error = error; > - controller->xfer = xfer; > - spin_unlock_irqrestore(&controller->lock, flags); > - > /* re-read opflags as flags may have changed due to actions above */ > opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); > > if ((controller->rx_bytes == xfer->len && > (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) > + done = true; > + > + spin_lock(&controller->lock); > + controller->error = error; > + controller->xfer = done ? NULL : xfer; > + spin_unlock(&controller->lock); > + > + if (done) > complete(&controller->done); > Its not clear, why the driver is setting the controller->xfer = NULL and restoring it inside the irq. This patch seems to fix things on top of that. Regards, Sricharan
On Wed, Jun 14, 2017 at 12:57:25PM +0530, Sricharan R wrote: > Hi Varada, > > On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: > > It's possible for a SPI transaction to complete and get another > > interrupt and have it processed on the same spi_transfer before the > > transfer_one can set it to NULL. > > > > This masks unexpected interrupts, so let's set the spi_transfer to > > NULL in the interrupt once the transaction is done. So we can > > properly detect these bad interrupts and print warning messages. > > > > Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org> > > Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> > > --- > > drivers/spi/spi-qup.c | 20 +++++++++++--------- > > 1 file changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > > index bd53e82..1a2a9d9 100644 > > --- a/drivers/spi/spi-qup.c > > +++ b/drivers/spi/spi-qup.c > > @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) > > struct spi_qup *controller = dev_id; > > struct spi_transfer *xfer; > > u32 opflags, qup_err, spi_err; > > - unsigned long flags; > > int error = 0; > > + bool done = 0; > > > > - spin_lock_irqsave(&controller->lock, flags); > > + spin_lock(&controller->lock); > > xfer = controller->xfer; > > controller->xfer = NULL; > > - spin_unlock_irqrestore(&controller->lock, flags); > > + spin_unlock(&controller->lock); > > Why change the locking here ? > > > > > qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); > > spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); > > @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) > > spi_qup_write(controller, xfer); > > } > > > > - spin_lock_irqsave(&controller->lock, flags); > > - controller->error = error; > > - controller->xfer = xfer; > > - spin_unlock_irqrestore(&controller->lock, flags); > > - > > /* re-read opflags as flags may have changed due to actions above */ > > opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); > > > > if ((controller->rx_bytes == xfer->len && > > (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) > > + done = true; > > + > > + spin_lock(&controller->lock); > > + controller->error = error; > > + controller->xfer = done ? NULL : xfer; > > + spin_unlock(&controller->lock); > > + > > + if (done) > > complete(&controller->done); > > > Its not clear, why the driver is setting the controller->xfer = NULL > and restoring it inside the irq. This patch seems to fix things on > top of that. I think the original intent was to make sure that the irqhandler knew that there was no outstanding transaction. This begs the question of why that would ever be necessary. I think it would suffice to rework all of that to remove that behavior and perhaps enable/disable the irq as we need to during transactions. I've never been a fan of the controller->xfer being set to NULL. Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 14, 2017 at 2:59 PM, Andy Gross <andy.gross@linaro.org> wrote: > On Wed, Jun 14, 2017 at 12:57:25PM +0530, Sricharan R wrote: >> Hi Varada, >> >> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: >> > It's possible for a SPI transaction to complete and get another >> > interrupt and have it processed on the same spi_transfer before the >> > transfer_one can set it to NULL. >> > >> > This masks unexpected interrupts, so let's set the spi_transfer to >> > NULL in the interrupt once the transaction is done. So we can >> > properly detect these bad interrupts and print warning messages. >> > >> > Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org> >> > Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> >> > --- >> > drivers/spi/spi-qup.c | 20 +++++++++++--------- >> > 1 file changed, 11 insertions(+), 9 deletions(-) >> > >> > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c >> > index bd53e82..1a2a9d9 100644 >> > --- a/drivers/spi/spi-qup.c >> > +++ b/drivers/spi/spi-qup.c >> > @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) >> > struct spi_qup *controller = dev_id; >> > struct spi_transfer *xfer; >> > u32 opflags, qup_err, spi_err; >> > - unsigned long flags; >> > int error = 0; >> > + bool done = 0; >> > >> > - spin_lock_irqsave(&controller->lock, flags); >> > + spin_lock(&controller->lock); >> > xfer = controller->xfer; >> > controller->xfer = NULL; >> > - spin_unlock_irqrestore(&controller->lock, flags); >> > + spin_unlock(&controller->lock); >> >> Why change the locking here ? >> >> > >> > qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); >> > spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); >> > @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) >> > spi_qup_write(controller, xfer); >> > } >> > >> > - spin_lock_irqsave(&controller->lock, flags); >> > - controller->error = error; >> > - controller->xfer = xfer; >> > - spin_unlock_irqrestore(&controller->lock, flags); >> > - >> > /* re-read opflags as flags may have changed due to actions above */ >> > opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); >> > >> > if ((controller->rx_bytes == xfer->len && >> > (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) >> > + done = true; >> > + >> > + spin_lock(&controller->lock); >> > + controller->error = error; >> > + controller->xfer = done ? NULL : xfer; >> > + spin_unlock(&controller->lock); >> > + >> > + if (done) >> > complete(&controller->done); >> > >> Its not clear, why the driver is setting the controller->xfer = NULL >> and restoring it inside the irq. This patch seems to fix things on >> top of that. > > I think the original intent was to make sure that the irqhandler knew that there > was no outstanding transaction. This begs the question of why that would ever > be necessary. I think it would suffice to rework all of that to remove that > behavior and perhaps enable/disable the irq as we need to during transactions. > > I've never been a fan of the controller->xfer being set to NULL. Also, this patch presumably fixes an issue seen on Dakota SoCs, doesn't add or remote the NULL bits. -M -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, On 6/15/2017 1:29 AM, Andy Gross wrote: > On Wed, Jun 14, 2017 at 12:57:25PM +0530, Sricharan R wrote: >> Hi Varada, >> >> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: >>> It's possible for a SPI transaction to complete and get another >>> interrupt and have it processed on the same spi_transfer before the >>> transfer_one can set it to NULL. >>> >>> This masks unexpected interrupts, so let's set the spi_transfer to >>> NULL in the interrupt once the transaction is done. So we can >>> properly detect these bad interrupts and print warning messages. >>> >>> Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org> >>> Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> >>> --- >>> drivers/spi/spi-qup.c | 20 +++++++++++--------- >>> 1 file changed, 11 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c >>> index bd53e82..1a2a9d9 100644 >>> --- a/drivers/spi/spi-qup.c >>> +++ b/drivers/spi/spi-qup.c >>> @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) >>> struct spi_qup *controller = dev_id; >>> struct spi_transfer *xfer; >>> u32 opflags, qup_err, spi_err; >>> - unsigned long flags; >>> int error = 0; >>> + bool done = 0; >>> >>> - spin_lock_irqsave(&controller->lock, flags); >>> + spin_lock(&controller->lock); >>> xfer = controller->xfer; >>> controller->xfer = NULL; >>> - spin_unlock_irqrestore(&controller->lock, flags); >>> + spin_unlock(&controller->lock); >> >> Why change the locking here ? >> >>> >>> qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); >>> spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); >>> @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) >>> spi_qup_write(controller, xfer); >>> } >>> >>> - spin_lock_irqsave(&controller->lock, flags); >>> - controller->error = error; >>> - controller->xfer = xfer; >>> - spin_unlock_irqrestore(&controller->lock, flags); >>> - >>> /* re-read opflags as flags may have changed due to actions above */ >>> opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); >>> >>> if ((controller->rx_bytes == xfer->len && >>> (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) >>> + done = true; >>> + >>> + spin_lock(&controller->lock); >>> + controller->error = error; >>> + controller->xfer = done ? NULL : xfer; >>> + spin_unlock(&controller->lock); >>> + >>> + if (done) >>> complete(&controller->done); >>> >> Its not clear, why the driver is setting the controller->xfer = NULL >> and restoring it inside the irq. This patch seems to fix things on >> top of that. > > I think the original intent was to make sure that the irqhandler knew that there > was no outstanding transaction. This begs the question of why that would ever > be necessary. I think it would suffice to rework all of that to remove that > behavior and perhaps enable/disable the irq as we need to during transactions. > > I've never been a fan of the controller->xfer being set to NULL. Agree, that part needs fixing in the original code. Also patch #10 changing the behavior to acknowledge the interrupts only when its spurious does not look correct. Trying to fix the original should simplify or avoid the spurious case itself. Regards, Sricharan
diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c index bd53e82..1a2a9d9 100644 --- a/drivers/spi/spi-qup.c +++ b/drivers/spi/spi-qup.c @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) struct spi_qup *controller = dev_id; struct spi_transfer *xfer; u32 opflags, qup_err, spi_err; - unsigned long flags; int error = 0; + bool done = 0; - spin_lock_irqsave(&controller->lock, flags); + spin_lock(&controller->lock); xfer = controller->xfer; controller->xfer = NULL; - spin_unlock_irqrestore(&controller->lock, flags); + spin_unlock(&controller->lock); qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) spi_qup_write(controller, xfer); } - spin_lock_irqsave(&controller->lock, flags); - controller->error = error; - controller->xfer = xfer; - spin_unlock_irqrestore(&controller->lock, flags); - /* re-read opflags as flags may have changed due to actions above */ opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); if ((controller->rx_bytes == xfer->len && (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) + done = true; + + spin_lock(&controller->lock); + controller->error = error; + controller->xfer = done ? NULL : xfer; + spin_unlock(&controller->lock); + + if (done) complete(&controller->done); return IRQ_HANDLED; @@ -765,7 +768,6 @@ static int spi_qup_transfer_one(struct spi_master *master, exit: spi_qup_set_state(controller, QUP_STATE_RESET); spin_lock_irqsave(&controller->lock, flags); - controller->xfer = NULL; if (!ret) ret = controller->error; spin_unlock_irqrestore(&controller->lock, flags);