[PATCH 0/4] spi: spi-qcom-qspi: Follow-up patches to DMA mode support

[PATCH 0/4] spi: spi-qcom-qspi: Follow-up patches to DMA mode support

[Vijaya Krishna Nivarthi]

This patch series adds 4 follow-up changes to DMA mode support.
1. Handles write failure in some cases by averting a race condition
2. Handles static checker warning
3. Adds a memory barrier to avoid a possible data out of sync case
4. Book keeping change

Vijaya Krishna Nivarthi (4):
  spi: spi-qcom-qspi: Ignore disabled interrupts' status in isr
  spi: spi-qcom-qspi: Use GFP_ATOMIC flag while allocating for
    descriptor
  spi: spi-qcom-qspi: Call dma_wmb() after setting up descriptors
  spi: spi-qcom-qspi: Add DMA_CHAIN_DONE to ALL_IRQS

 drivers/spi/spi-qcom-qspi.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 

[PATCH 1/4] spi: spi-qcom-qspi: Ignore disabled interrupts' status in isr

[Vijaya Krishna Nivarthi]

During FIFO/DMA modes dynamic switching, only corresponding interrupts are
enabled. However its possible that FIFO related interrupt status registers
get set during DMA mode. For example WR_FIFO_EMPTY bit is set during DMA
TX.

Ignore such status bits so that they don't trip unwanted operations.

Suggested-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
---
 drivers/spi/spi-qcom-qspi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index a0ad980..b995542 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -603,6 +603,9 @@ static irqreturn_t qcom_qspi_irq(int irq, void *dev_id)
 	int_status = readl(ctrl->base + MSTR_INT_STATUS);
 	writel(int_status, ctrl->base + MSTR_INT_STATUS);
 
+	/* Ignore disabled interrupts */
+	int_status &= readl(ctrl->base + MSTR_INT_EN);
+
 	/* PIO mode handling */
 	if (ctrl->xfer.dir == QSPI_WRITE) {
 		if (int_status & WR_FIFO_EMPTY)
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

[PATCH 2/4] spi: spi-qcom-qspi: Use GFP_ATOMIC flag while allocating for descriptor

[Vijaya Krishna Nivarthi]

While allocating for DMA descriptor, GFP_KERNEL flag is being used and
this allocation happens within critical section with spinlock acquired.
This generates a static checker warning.

Use GFP_ATOMIC to prevent sleeping; and since this increases chances of
allocation failure, add handling accordingly.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/abc223e8-44af-40bb-a0bd-9865b393f435@moroto.mountain/
Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
---
 drivers/spi/spi-qcom-qspi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index b995542..b938908 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -308,9 +308,11 @@ static int qcom_qspi_alloc_desc(struct qcom_qspi *ctrl, dma_addr_t dma_ptr,
 	dma_addr_t dma_cmd_desc;
 
 	/* allocate for dma cmd descriptor */
-	virt_cmd_desc = dma_pool_alloc(ctrl->dma_cmd_pool, GFP_KERNEL | __GFP_ZERO, &dma_cmd_desc);
-	if (!virt_cmd_desc)
-		return -ENOMEM;
+	virt_cmd_desc = dma_pool_alloc(ctrl->dma_cmd_pool, GFP_ATOMIC | __GFP_ZERO, &dma_cmd_desc);
+	if (!virt_cmd_desc) {
+		dev_warn_once(ctrl->dev, "Couldn't find memory for descriptor\n");
+		return -EAGAIN;
+	}
 
 	ctrl->virt_cmd_desc[ctrl->n_cmd_desc] = virt_cmd_desc;
 	ctrl->dma_cmd_desc[ctrl->n_cmd_desc] = dma_cmd_desc;
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

[PATCH 3/4] spi: spi-qcom-qspi: Call dma_wmb() after setting up descriptors

[Vijaya Krishna Nivarthi]

After setting up dma descriptors and before initiaiting dma transfer, call
dma_wmb() to ensure all writes go through.
This doesn't fix any reported problem but is added for safety.

Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
---
 drivers/spi/spi-qcom-qspi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index b938908..d75234d 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -443,8 +443,10 @@ static int qcom_qspi_transfer_one(struct spi_master *master,
 
 		ret = qcom_qspi_setup_dma_desc(ctrl, xfer);
 		if (ret != -EAGAIN) {
-			if (!ret)
+			if (!ret) {
+				dma_wmb();
 				qcom_qspi_dma_xfer(ctrl);
+			}
 			goto exit;
 		}
 		dev_warn_once(ctrl->dev, "DMA failure, falling back to PIO\n");
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

[PATCH 4/4] spi: spi-qcom-qspi: Add DMA_CHAIN_DONE to ALL_IRQS

[Vijaya Krishna Nivarthi]

Add latest added DMA_CHAIN_DONE irq to QSPI_ALL_IRQS that encompasses all
of the qspi IRQs.

Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
---
 drivers/spi/spi-qcom-qspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index d75234d..b56cb16 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -69,7 +69,7 @@
 				 WR_FIFO_OVERRUN)
 #define QSPI_ALL_IRQS		(QSPI_ERR_IRQS | RESP_FIFO_RDY | \
 				 WR_FIFO_EMPTY | WR_FIFO_FULL | \
-				 TRANSACTION_DONE)
+				 TRANSACTION_DONE | DMA_CHAIN_DONE)
 
 #define PIO_XFER_CTRL		0x0014
 #define REQUEST_COUNT_MSK	0xffff
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Re: [PATCH 1/4] spi: spi-qcom-qspi: Ignore disabled interrupts' status in isr

[Doug Anderson]

Hi,

On Tue, Jul 25, 2023 at 4:48 AM Vijaya Krishna Nivarthi
<quic_vnivarth@quicinc.com> wrote:
>
> During FIFO/DMA modes dynamic switching, only corresponding interrupts are
> enabled. However its possible that FIFO related interrupt status registers
> get set during DMA mode. For example WR_FIFO_EMPTY bit is set during DMA
> TX.
>
> Ignore such status bits so that they don't trip unwanted operations.
>
> Suggested-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
> ---
>  drivers/spi/spi-qcom-qspi.c | 3 +++
>  1 file changed, 3 insertions(+)

Things are pretty broken without this fix, so it definitely needs a
Fixes tag so it can find its way to mainline as soon as possible:

Fixes: b5762d95607e ("spi: spi-qcom-qspi: Add DMA mode support")

Looks good to me and also works fixes the problem for me. Thus I'm
happy with any of these tags:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/4] spi: spi-qcom-qspi: Use GFP_ATOMIC flag while allocating for descriptor

[Doug Anderson]

Hi,

On Tue, Jul 25, 2023 at 4:48 AM Vijaya Krishna Nivarthi
<quic_vnivarth@quicinc.com> wrote:
>
> While allocating for DMA descriptor, GFP_KERNEL flag is being used and
> this allocation happens within critical section with spinlock acquired.
> This generates a static checker warning.
>
> Use GFP_ATOMIC to prevent sleeping; and since this increases chances of
> allocation failure, add handling accordingly.
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/abc223e8-44af-40bb-a0bd-9865b393f435@moroto.mountain/
> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
> ---
>  drivers/spi/spi-qcom-qspi.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Fixes: b5762d95607e ("spi: spi-qcom-qspi: Add DMA mode support")
Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 3/4] spi: spi-qcom-qspi: Call dma_wmb() after setting up descriptors

[Doug Anderson]

Hi,

On Tue, Jul 25, 2023 at 4:48 AM Vijaya Krishna Nivarthi
<quic_vnivarth@quicinc.com> wrote:
>
> After setting up dma descriptors and before initiaiting dma transfer, call
> dma_wmb() to ensure all writes go through.
> This doesn't fix any reported problem but is added for safety.
>
> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
> ---
>  drivers/spi/spi-qcom-qspi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

This looks right to me.

Fixes: b5762d95607e ("spi: spi-qcom-qspi: Add DMA mode support")
Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 4/4] spi: spi-qcom-qspi: Add DMA_CHAIN_DONE to ALL_IRQS

[Doug Anderson]

Hi,

On Tue, Jul 25, 2023 at 4:48 AM Vijaya Krishna Nivarthi
<quic_vnivarth@quicinc.com> wrote:
>
> Add latest added DMA_CHAIN_DONE irq to QSPI_ALL_IRQS that encompasses all
> of the qspi IRQs.
>
> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
> ---
>  drivers/spi/spi-qcom-qspi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I don't think this really does anything since the interrupts shouldn't
really be "hanging around" when a PIO transfer starts and that's the
only place it's used. I suspect we could actually fully remove that
bit of "Ack any previous interrupts that might be hanging around" code
and everything would be fine.

In any case, I don't have any huge objections, thus:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 0/4] spi: spi-qcom-qspi: Follow-up patches to DMA mode support

[Mark Brown]

On Tue, 25 Jul 2023 17:18:05 +0530, Vijaya Krishna Nivarthi wrote:
> This patch series adds 4 follow-up changes to DMA mode support.
> 1. Handles write failure in some cases by averting a race condition
> 2. Handles static checker warning
> 3. Adds a memory barrier to avoid a possible data out of sync case
> 4. Book keeping change
> 
> Vijaya Krishna Nivarthi (4):
>   spi: spi-qcom-qspi: Ignore disabled interrupts' status in isr
>   spi: spi-qcom-qspi: Use GFP_ATOMIC flag while allocating for
>     descriptor
>   spi: spi-qcom-qspi: Call dma_wmb() after setting up descriptors
>   spi: spi-qcom-qspi: Add DMA_CHAIN_DONE to ALL_IRQS
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/4] spi: spi-qcom-qspi: Ignore disabled interrupts' status in isr
      commit: 17aaf9ea07b656016316dc37716e987742b3e296
[2/4] spi: spi-qcom-qspi: Use GFP_ATOMIC flag while allocating for descriptor
      commit: f7ba36d399c4558f36886adff9400be591b245f6
[3/4] spi: spi-qcom-qspi: Call dma_wmb() after setting up descriptors
      commit: cfb81f2243b25a0d79accc6510ad66c5c5ad99ba
[4/4] spi: spi-qcom-qspi: Add DMA_CHAIN_DONE to ALL_IRQS
      commit: 916a4edf3daed845b1e5d6cf0578a7e43c6f520e

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark