| Message ID | 20200427111043.70218-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 920c5974f0d3aff91b5751e7568b54752fb5d6da |
| Headers | show |
| Series | dmaengine: qcom_hidma: Simplify error handling path in hidma_probe | expand |
On 27-04-20, 13:10, Christophe JAILLET wrote: > There is no need to call 'hidma_debug_uninit()' in the error handling > path. 'hidma_debug_init()' has not been called yet. Applied, thanks
On Mon, Apr 27, 2020 at 01:10:43PM +0200, Christophe JAILLET wrote: > There is no need to call 'hidma_debug_uninit()' in the error handling > path. 'hidma_debug_init()' has not been called yet. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/dma/qcom/hidma.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c > index 411f91fde734..87490e125bc3 100644 > --- a/drivers/dma/qcom/hidma.c > +++ b/drivers/dma/qcom/hidma.c > @@ -897,7 +897,6 @@ static int hidma_probe(struct platform_device *pdev) > if (msi) ^^^ This test doesn't work. It will call free hidma_free_msis() if the hidma_request_msi() call fails. We should do: if (msi) { rc = hidma_request_msi(dmadev, pdev); msi = false; } if (!msi) { hidma_ll_setup_irq(dmadev->lldev, false); rc = devm_request_irq(&pdev->dev, chirq, hidma_chirq_handler, 0, "qcom-hidma", dmadev->lldev); if (rc) goto uninit; } > hidma_free_msis(dmadev); > > - hidma_debug_uninit(dmadev); > hidma_ll_uninit(dmadev->lldev); > dmafree: > if (dmadev) ^^^^^^ This test isn't necessary and should be deleted. regards, dan carpenter
On 4/28/2020 8:54 AM, Dan Carpenter wrote: >> @@ -897,7 +897,6 @@ static int hidma_probe(struct platform_device *pdev) >> if (msi) > ^^^ > This test doesn't work. It will call free hidma_free_msis() if the > hidma_request_msi() call fails. We should do: > > if (msi) { > rc = hidma_request_msi(dmadev, pdev); > msi = false; > } > > if (!msi) { > hidma_ll_setup_irq(dmadev->lldev, false); > rc = devm_request_irq(&pdev->dev, chirq, hidma_chirq_handler, > 0, "qcom-hidma", dmadev->lldev); > if (rc) > goto uninit; > } > > Let me clarify how this works. MSI capability is not present on all platforms. Therefore, this is detected by an ACPI/DTS parameter called HIDMA_MSI_CAP. msi = hidma_test_capability(&pdev->dev, HIDMA_MSI_CAP); Therefore, 1. Code will request MSI capability if it is present. 2. Code will fallback to plain IRQ, if MSI allocation also fails. I hope this helps. We need both #1 and #2 to be supported.
I apologize, I wrote my code hurriedly and did no explain the bug well. I understood what the code is doing, but my fix was missing an if condition. On Tue, Apr 28, 2020 at 12:01:15PM -0400, Sinan Kaya wrote: > On 4/28/2020 8:54 AM, Dan Carpenter wrote: > >> @@ -897,7 +897,6 @@ static int hidma_probe(struct platform_device *pdev) > >> if (msi) > > ^^^ > > This test doesn't work. It will call free hidma_free_msis() if the > > hidma_request_msi() call fails. We should do: > > > > if (msi) { > > rc = hidma_request_msi(dmadev, pdev); > > msi = false; What I meant to say here was: if (msi) { rc = hidma_request_msi(dmadev, pdev); if (rc) msi = false; Otherwise we end up checking freeing the msi in the error handling code when we did not take it. Hopefully, that clears things up? regards, dan carpenter
On 4/28/2020 1:21 PM, Dan Carpenter wrote: > What I meant to say here was: > > if (msi) { > rc = hidma_request_msi(dmadev, pdev); > if (rc) > msi = false; > > Otherwise we end up checking freeing the msi in the error handling > code when we did not take it. > > Hopefully, that clears things up? Yes, that works. However, I'd rather use a different flag for this in order not to mix the meaning of msi capability vs. msi allocation failure.
diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c index 411f91fde734..87490e125bc3 100644 --- a/drivers/dma/qcom/hidma.c +++ b/drivers/dma/qcom/hidma.c @@ -897,7 +897,6 @@ static int hidma_probe(struct platform_device *pdev) if (msi) hidma_free_msis(dmadev); - hidma_debug_uninit(dmadev); hidma_ll_uninit(dmadev->lldev); dmafree: if (dmadev)
There is no need to call 'hidma_debug_uninit()' in the error handling path. 'hidma_debug_init()' has not been called yet. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/dma/qcom/hidma.c | 1 - 1 file changed, 1 deletion(-)