PCI: epf-mhi: return 0 on success instead of positive jiffies

wait_for_completion_timeout() returns the number of jiffies remaining on success (positive value) or 0 on timeout. The pci_epf_mhi_edma_read() and pci_epf_mhi_edma_write() functions use the return value directly as their own return value, only converting timeout (0) to -ETIMEDOUT.

On success, they return the positive jiffies value. The callers in drivers/bus/mhi/ep/ring.c check for errors with "if (ret < 0)" for read_sync and "if (ret)" for write_sync. This causes write_sync success cases to be incorrectly treated as errors since the positive jiffies value is non-zero.

Fix by setting ret to 0 when wait_for_completion_timeout() succeeds.

Fixes 7b99aaaddabb ("PCI: epf-mhi: Add eDMA support")

Cc [email protected]

Signed-off-by Daniel Hodges <[email protected]>

---
  drivers/pci/endpoint/functions/pci-epf-mhi.c | 4 ++++
  1 file changed, 4 insertions(+)

drivers/pci/endpoint/functions/pci-epf-mhi.c +4 -0

index 6643a88c7a0c..2f077d0b7957 100644
@@ -367,6 +367,8 @@ static int pci_epf_mhi_edma_read(struct mhi_ep_cntrl *mhi_cntrl,
  		dev_err(dev, "DMA transfer timeout\n");
  		dmaengine_terminate_sync(chan);
  		ret = -ETIMEDOUT;
+	} else {
+		ret = 0;
  	}
 
  err_unmap:

@@ -438,6 +440,8 @@ static int pci_epf_mhi_edma_write(struct mhi_ep_cntrl *mhi_cntrl,
  		dev_err(dev, "DMA transfer timeout\n");
  		dmaengine_terminate_sync(chan);
  		ret = -ETIMEDOUT;
+	} else {
+		ret = 0;
  	}
 
  err_unmap:

wait_for_completion_timeout() returns the number of jiffies remaining on success (positive value) or 0 on timeout. The pci_epf_mhi_edma_read() and pci_epf_mhi_edma_write() functions use the return value directly as their own return value, only converting timeout (0) to -ETIMEDOUT.

On success, they return the positive jiffies value. The callers in drivers/bus/mhi/ep/ring.c check for errors with "if (ret < 0)" for read_sync and "if (ret)" for write_sync. This causes write_sync success cases to be incorrectly treated as errors since the positive jiffies value is non-zero.

[...]

wait_for_completion_timeout() returns the number of jiffies remaining on success (positive value) or 0 on timeout. The pci_epf_mhi_edma_read() and pci_epf_mhi_edma_write() functions use the return value directly as their own return value, only converting timeout (0) to -ETIMEDOUT.

On success, they return the positive jiffies value. The callers in drivers/bus/mhi/ep/ring.c check for errors with "if (ret < 0)" for read_sync and "if (ret)" for write_sync. This causes write_sync success cases to be incorrectly treated as errors since the positive jiffies value is non-zero.

Fix by setting ret to 0 when wait_for_completion_timeout() succeeds.

Fixes 7b99aaaddabb ("PCI: epf-mhi: Add eDMA support")

Cc [email protected]

Signed-off-by Daniel Hodges <[email protected]>

---
 drivers/pci/endpoint/functions/pci-epf-mhi.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/pci/endpoint/functions/pci-epf-mhi.c +4 -0

index 6643a88c7a0c..2f077d0b7957 100644
@@ -367,6 +367,8 @@ static int pci_epf_mhi_edma_read(struct mhi_ep_cntrl *mhi_cntrl,
 		dev_err(dev, "DMA transfer timeout\n");
 		dmaengine_terminate_sync(chan);
 		ret = -ETIMEDOUT;
+	} else {
+		ret = 0;
 	}
 
 err_unmap:

@@ -438,6 +440,8 @@ static int pci_epf_mhi_edma_write(struct mhi_ep_cntrl *mhi_cntrl,
 		dev_err(dev, "DMA transfer timeout\n");
 		dmaengine_terminate_sync(chan);
 		ret = -ETIMEDOUT;
+	} else {
+		ret = 0;
 	}
 
 err_unmap:

Signature · 1 line

2.52.0

Quoted 11 lines · Daniel Hodges

wait_for_completion_timeout() returns the number of jiffies remaining on success (positive value) or 0 on timeout. The pci_epf_mhi_edma_read() and pci_epf_mhi_edma_write() functions use the return value directly as their own return value, only converting timeout (0) to -ETIMEDOUT.

On success, they return the positive jiffies value. The callers in drivers/bus/mhi/ep/ring.c check for errors with "if (ret < 0)" for read_sync and "if (ret)" for write_sync. This causes write_sync success cases to be incorrectly treated as errors since the positive jiffies value is non-zero.

Quoted 5 lines

Fix by setting ret to 0 when wait_for_completion_timeout() succeeds.

Fixes 7b99aaaddabb ("PCI: epf-mhi: Add eDMA support")

Cc [email protected]

Signed-off-by Daniel Hodges <[email protected]>

Thanks for the patch!

Two questions: first, is there any reason why __mhi_ep_cache_ring() tests for "ret < 0" but mhi_ep_ring_add_element() tests for "ret" (non-zero)? Could/should they both test just for non-zero, which I think is the typical style?

Second, the subject and commit log are perfectly correct but basically at the level of describing the C code. I propose something along these lines:

PCI: epf-mhi: Return 0, not remaining timeout, when eDMA ops complete

pci_epf_mhi_edma_read() and pci_epf_mhi_edma_write() start DMA operations and wait for completion with a timeout.

On successful completion, they previously returned the remaining timeout, which callers may treat as an error. In particular, mhi_ep_ring_add_element(), which calls pci_epf_mhi_edma_write() via mhi_cntrl->write_sync(), interprets any non-zero return value as failure.

Return 0 on success instead of the remaining timeout to prevent mhi_ep_ring_add_element() from treating successful completion as an error.

Quoted 18 lines · Bjorn Helgaas

Quoted 11 lines · Daniel Hodges

wait_for_completion_timeout() returns the number of jiffies remaining on success (positive value) or 0 on timeout. The pci_epf_mhi_edma_read() and pci_epf_mhi_edma_write() functions use the return value directly as their own return value, only converting timeout (0) to -ETIMEDOUT.

On success, they return the positive jiffies value. The callers in drivers/bus/mhi/ep/ring.c check for errors with "if (ret < 0)" for read_sync and "if (ret)" for write_sync. This causes write_sync success cases to be incorrectly treated as errors since the positive jiffies value is non-zero.

Quoted 5 lines

Fix by setting ret to 0 when wait_for_completion_timeout() succeeds.

Fixes 7b99aaaddabb ("PCI: epf-mhi: Add eDMA support")

Cc [email protected]

Signed-off-by Daniel Hodges <[email protected]>

Quoted 7 lines

Thanks for the patch!

Two questions: first, is there any reason why __mhi_ep_cache_ring() tests for "ret < 0" but mhi_ep_ring_add_element() tests for "ret" (non-zero)? Could/should they both test just for non-zero, which I think is the typical style?

Yes, agree. I've sent a patch to fix this. Thanks for spotting!

Quoted 15 lines

Second, the subject and commit log are perfectly correct but basically at the level of describing the C code. I propose something along these lines:

PCI: epf-mhi: Return 0, not remaining timeout, when eDMA ops complete

pci_epf_mhi_edma_read() and pci_epf_mhi_edma_write() start DMA operations and wait for completion with a timeout.

On successful completion, they previously returned the remaining timeout, which callers may treat as an error. In particular, mhi_ep_ring_add_element(), which calls pci_epf_mhi_edma_write() via mhi_cntrl->write_sync(), interprets any non-zero return value as failure.

Quoted 4 lines

Return 0 on success instead of the remaining timeout to prevent mhi_ep_ring_add_element() from treating successful completion as an error.

Ammended the commit with the above, thanks!

- Mani