* [PATCH] dmaengine: qcom_hidma: Simplify error handling path in hidma_probe
@ 2020-04-27 11:10 Christophe JAILLET
2020-04-27 16:08 ` Vinod Koul
2020-04-28 12:54 ` Dan Carpenter
0 siblings, 2 replies; 6+ messages in thread
From: Christophe JAILLET @ 2020-04-27 11:10 UTC (permalink / raw)
To: okaya, agross, bjorn.andersson, vkoul, dan.j.williams
Cc: linux-arm-kernel, linux-arm-msm, dmaengine, linux-kernel,
kernel-janitors, Christophe JAILLET
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)
hidma_free_msis(dmadev);
- hidma_debug_uninit(dmadev);
hidma_ll_uninit(dmadev->lldev);
dmafree:
if (dmadev)
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: qcom_hidma: Simplify error handling path in hidma_probe
2020-04-27 11:10 [PATCH] dmaengine: qcom_hidma: Simplify error handling path in hidma_probe Christophe JAILLET
@ 2020-04-27 16:08 ` Vinod Koul
2020-04-28 12:54 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Vinod Koul @ 2020-04-27 16:08 UTC (permalink / raw)
To: Christophe JAILLET
Cc: okaya, agross, bjorn.andersson, dan.j.williams, linux-arm-kernel,
linux-arm-msm, dmaengine, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: qcom_hidma: Simplify error handling path in hidma_probe
2020-04-27 11:10 [PATCH] dmaengine: qcom_hidma: Simplify error handling path in hidma_probe Christophe JAILLET
2020-04-27 16:08 ` Vinod Koul
@ 2020-04-28 12:54 ` Dan Carpenter
2020-04-28 16:01 ` Sinan Kaya
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2020-04-28 12:54 UTC (permalink / raw)
To: Christophe JAILLET
Cc: okaya, agross, bjorn.andersson, vkoul, dan.j.williams,
linux-arm-kernel, linux-arm-msm, dmaengine, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: qcom_hidma: Simplify error handling path in hidma_probe
2020-04-28 12:54 ` Dan Carpenter
@ 2020-04-28 16:01 ` Sinan Kaya
2020-04-28 17:21 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Sinan Kaya @ 2020-04-28 16:01 UTC (permalink / raw)
To: Dan Carpenter, Christophe JAILLET
Cc: agross, bjorn.andersson, vkoul, dan.j.williams, linux-arm-kernel,
linux-arm-msm, dmaengine, linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: qcom_hidma: Simplify error handling path in hidma_probe
2020-04-28 16:01 ` Sinan Kaya
@ 2020-04-28 17:21 ` Dan Carpenter
2020-04-28 20:29 ` Sinan Kaya
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2020-04-28 17:21 UTC (permalink / raw)
To: Sinan Kaya
Cc: Christophe JAILLET, agross, bjorn.andersson, vkoul,
dan.j.williams, linux-arm-kernel, linux-arm-msm, dmaengine,
linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: qcom_hidma: Simplify error handling path in hidma_probe
2020-04-28 17:21 ` Dan Carpenter
@ 2020-04-28 20:29 ` Sinan Kaya
0 siblings, 0 replies; 6+ messages in thread
From: Sinan Kaya @ 2020-04-28 20:29 UTC (permalink / raw)
To: Dan Carpenter
Cc: Christophe JAILLET, agross, bjorn.andersson, vkoul,
dan.j.williams, linux-arm-kernel, linux-arm-msm, dmaengine,
linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-28 20:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-27 11:10 [PATCH] dmaengine: qcom_hidma: Simplify error handling path in hidma_probe Christophe JAILLET
2020-04-27 16:08 ` Vinod Koul
2020-04-28 12:54 ` Dan Carpenter
2020-04-28 16:01 ` Sinan Kaya
2020-04-28 17:21 ` Dan Carpenter
2020-04-28 20:29 ` Sinan Kaya
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).