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 <[email protected]>
---
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)
hidma_free_msis(dmadev);
- hidma_debug_uninit(dmadev);
hidma_ll_uninit(dmadev->lldev);
dmafree:
if (dmadev)
--
2.25.1
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
--
~Vinod
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 <[email protected]>
> ---
> 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.