dmaengine Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] dmaengine: ti: edma: add missed pm_runtime_disable
@ 2019-11-18  7:37 Chuhong Yuan
  2019-11-22  5:23 ` Vinod Koul
  2019-11-22  7:50 ` Peter Ujfalusi
  0 siblings, 2 replies; 5+ messages in thread
From: Chuhong Yuan @ 2019-11-18  7:37 UTC (permalink / raw)
  To: unlisted-recipients:; (no To-header on input)
  Cc: Vinod Koul, Dan Williams, dmaengine, linux-kernel, Chuhong Yuan

The driver forgets to call pm_runtime_disable in probe failure and
remove.
Add the calls and modify probe failure handling to fix it.

Fixes: 2b6b3b742019 ("ARM/dmaengine: edma: Merge the two drivers under drivers/dma/")
Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
 drivers/dma/ti/edma.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c
index ba7c4f07fcd6..8be32fd9f762 100644
--- a/drivers/dma/ti/edma.c
+++ b/drivers/dma/ti/edma.c
@@ -2282,16 +2282,18 @@ static int edma_probe(struct platform_device *pdev)
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0) {
 		dev_err(dev, "pm_runtime_get_sync() failed\n");
-		return ret;
+		goto err_disable_pm;
 	}
 
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (ret)
-		return ret;
+		goto err_disable_pm;
 
 	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
-	if (!ecc)
-		return -ENOMEM;
+	if (!ecc) {
+		ret = -ENOMEM;
+		goto err_disable_pm;
+	}
 
 	ecc->dev = dev;
 	ecc->id = pdev->id;
@@ -2306,30 +2308,37 @@ static int edma_probe(struct platform_device *pdev)
 		mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 		if (!mem) {
 			dev_err(dev, "no mem resource?\n");
-			return -ENODEV;
+			ret = -ENODEV;
+			goto err_disable_pm;
 		}
 	}
 	ecc->base = devm_ioremap_resource(dev, mem);
-	if (IS_ERR(ecc->base))
-		return PTR_ERR(ecc->base);
+	if (IS_ERR(ecc->base)) {
+		ret = PTR_ERR(ecc->base);
+		goto err_disable_pm;
+	}
 
 	platform_set_drvdata(pdev, ecc);
 
 	/* Get eDMA3 configuration from IP */
 	ret = edma_setup_from_hw(dev, info, ecc);
 	if (ret)
-		return ret;
+		goto err_disable_pm;
 
 	/* Allocate memory based on the information we got from the IP */
 	ecc->slave_chans = devm_kcalloc(dev, ecc->num_channels,
 					sizeof(*ecc->slave_chans), GFP_KERNEL);
-	if (!ecc->slave_chans)
-		return -ENOMEM;
+	if (!ecc->slave_chans) {
+		ret = -ENOMEM;
+		goto err_disable_pm;
+	}
 
 	ecc->slot_inuse = devm_kcalloc(dev, BITS_TO_LONGS(ecc->num_slots),
 				       sizeof(unsigned long), GFP_KERNEL);
-	if (!ecc->slot_inuse)
-		return -ENOMEM;
+	if (!ecc->slot_inuse) {
+		ret = -ENOMEM;
+		goto err_disable_pm;
+	}
 
 	ecc->default_queue = info->default_queue;
 
@@ -2368,7 +2377,7 @@ static int edma_probe(struct platform_device *pdev)
 				       ecc);
 		if (ret) {
 			dev_err(dev, "CCINT (%d) failed --> %d\n", irq, ret);
-			return ret;
+			goto err_disable_pm;
 		}
 		ecc->ccint = irq;
 	}
@@ -2384,7 +2393,7 @@ static int edma_probe(struct platform_device *pdev)
 				       ecc);
 		if (ret) {
 			dev_err(dev, "CCERRINT (%d) failed --> %d\n", irq, ret);
-			return ret;
+			goto err_disable_pm;
 		}
 		ecc->ccerrint = irq;
 	}
@@ -2392,7 +2401,8 @@ static int edma_probe(struct platform_device *pdev)
 	ecc->dummy_slot = edma_alloc_slot(ecc, EDMA_SLOT_ANY);
 	if (ecc->dummy_slot < 0) {
 		dev_err(dev, "Can't allocate PaRAM dummy slot\n");
-		return ecc->dummy_slot;
+		ret = ecc->dummy_slot;
+		goto err_disable_pm;
 	}
 
 	queue_priority_mapping = info->queue_priority_mapping;
@@ -2473,6 +2483,8 @@ static int edma_probe(struct platform_device *pdev)
 
 err_reg1:
 	edma_free_slot(ecc, ecc->dummy_slot);
+err_disable_pm:
+	pm_runtime_disable(dev);
 	return ret;
 }
 
@@ -2503,6 +2515,7 @@ static int edma_remove(struct platform_device *pdev)
 	if (ecc->dma_memcpy)
 		dma_async_device_unregister(ecc->dma_memcpy);
 	edma_free_slot(ecc, ecc->dummy_slot);
+	pm_runtime_disable(dev);
 
 	return 0;
 }
-- 
2.24.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] dmaengine: ti: edma: add missed pm_runtime_disable
  2019-11-18  7:37 [PATCH 1/2] dmaengine: ti: edma: add missed pm_runtime_disable Chuhong Yuan
@ 2019-11-22  5:23 ` Vinod Koul
  2019-11-22  5:39   ` Chuhong Yuan
  2019-11-22  7:50 ` Peter Ujfalusi
  1 sibling, 1 reply; 5+ messages in thread
From: Vinod Koul @ 2019-11-22  5:23 UTC (permalink / raw)
  To: Chuhong Yuan; +Cc: Dan Williams, dmaengine, linux-kernel

On 18-11-19, 15:37, Chuhong Yuan wrote:
> The driver forgets to call pm_runtime_disable in probe failure and
> remove.
> Add the calls and modify probe failure handling to fix it.
> 
> Fixes: 2b6b3b742019 ("ARM/dmaengine: edma: Merge the two drivers under drivers/dma/")
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> ---
>  drivers/dma/ti/edma.c | 43 ++++++++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c
> index ba7c4f07fcd6..8be32fd9f762 100644
> --- a/drivers/dma/ti/edma.c
> +++ b/drivers/dma/ti/edma.c
> @@ -2282,16 +2282,18 @@ static int edma_probe(struct platform_device *pdev)
>  	ret = pm_runtime_get_sync(dev);
>  	if (ret < 0) {
>  		dev_err(dev, "pm_runtime_get_sync() failed\n");
> -		return ret;
> +		goto err_disable_pm;

Why would you disable here when sync has returned error?

>  	}
>  
>  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>  	if (ret)
> -		return ret;
> +		goto err_disable_pm;
>  
>  	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
> -	if (!ecc)
> -		return -ENOMEM;
> +	if (!ecc) {
> +		ret = -ENOMEM;
> +		goto err_disable_pm;
> +	}
>  
>  	ecc->dev = dev;
>  	ecc->id = pdev->id;
> @@ -2306,30 +2308,37 @@ static int edma_probe(struct platform_device *pdev)
>  		mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  		if (!mem) {
>  			dev_err(dev, "no mem resource?\n");
> -			return -ENODEV;
> +			ret = -ENODEV;
> +			goto err_disable_pm;
>  		}
>  	}
>  	ecc->base = devm_ioremap_resource(dev, mem);
> -	if (IS_ERR(ecc->base))
> -		return PTR_ERR(ecc->base);
> +	if (IS_ERR(ecc->base)) {
> +		ret = PTR_ERR(ecc->base);
> +		goto err_disable_pm;
> +	}
>  
>  	platform_set_drvdata(pdev, ecc);
>  
>  	/* Get eDMA3 configuration from IP */
>  	ret = edma_setup_from_hw(dev, info, ecc);
>  	if (ret)
> -		return ret;
> +		goto err_disable_pm;
>  
>  	/* Allocate memory based on the information we got from the IP */
>  	ecc->slave_chans = devm_kcalloc(dev, ecc->num_channels,
>  					sizeof(*ecc->slave_chans), GFP_KERNEL);
> -	if (!ecc->slave_chans)
> -		return -ENOMEM;
> +	if (!ecc->slave_chans) {
> +		ret = -ENOMEM;
> +		goto err_disable_pm;
> +	}
>  
>  	ecc->slot_inuse = devm_kcalloc(dev, BITS_TO_LONGS(ecc->num_slots),
>  				       sizeof(unsigned long), GFP_KERNEL);
> -	if (!ecc->slot_inuse)
> -		return -ENOMEM;
> +	if (!ecc->slot_inuse) {
> +		ret = -ENOMEM;
> +		goto err_disable_pm;
> +	}
>  
>  	ecc->default_queue = info->default_queue;
>  
> @@ -2368,7 +2377,7 @@ static int edma_probe(struct platform_device *pdev)
>  				       ecc);
>  		if (ret) {
>  			dev_err(dev, "CCINT (%d) failed --> %d\n", irq, ret);
> -			return ret;
> +			goto err_disable_pm;
>  		}
>  		ecc->ccint = irq;
>  	}
> @@ -2384,7 +2393,7 @@ static int edma_probe(struct platform_device *pdev)
>  				       ecc);
>  		if (ret) {
>  			dev_err(dev, "CCERRINT (%d) failed --> %d\n", irq, ret);
> -			return ret;
> +			goto err_disable_pm;
>  		}
>  		ecc->ccerrint = irq;
>  	}
> @@ -2392,7 +2401,8 @@ static int edma_probe(struct platform_device *pdev)
>  	ecc->dummy_slot = edma_alloc_slot(ecc, EDMA_SLOT_ANY);
>  	if (ecc->dummy_slot < 0) {
>  		dev_err(dev, "Can't allocate PaRAM dummy slot\n");
> -		return ecc->dummy_slot;
> +		ret = ecc->dummy_slot;
> +		goto err_disable_pm;
>  	}
>  
>  	queue_priority_mapping = info->queue_priority_mapping;
> @@ -2473,6 +2483,8 @@ static int edma_probe(struct platform_device *pdev)
>  
>  err_reg1:
>  	edma_free_slot(ecc, ecc->dummy_slot);
> +err_disable_pm:
> +	pm_runtime_disable(dev);
>  	return ret;
>  }
>  
> @@ -2503,6 +2515,7 @@ static int edma_remove(struct platform_device *pdev)
>  	if (ecc->dma_memcpy)
>  		dma_async_device_unregister(ecc->dma_memcpy);
>  	edma_free_slot(ecc, ecc->dummy_slot);
> +	pm_runtime_disable(dev);
>  
>  	return 0;
>  }
> -- 
> 2.24.0

-- 
~Vinod

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] dmaengine: ti: edma: add missed pm_runtime_disable
  2019-11-22  5:23 ` Vinod Koul
@ 2019-11-22  5:39   ` Chuhong Yuan
  2019-11-22  5:48     ` Vinod Koul
  0 siblings, 1 reply; 5+ messages in thread
From: Chuhong Yuan @ 2019-11-22  5:39 UTC (permalink / raw)
  To: Vinod Koul; +Cc: Dan Williams, dmaengine, LKML

On Fri, Nov 22, 2019 at 1:23 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 18-11-19, 15:37, Chuhong Yuan wrote:
> > The driver forgets to call pm_runtime_disable in probe failure and
> > remove.
> > Add the calls and modify probe failure handling to fix it.
> >
> > Fixes: 2b6b3b742019 ("ARM/dmaengine: edma: Merge the two drivers under drivers/dma/")
> > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > ---
> >  drivers/dma/ti/edma.c | 43 ++++++++++++++++++++++++++++---------------
> >  1 file changed, 28 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c
> > index ba7c4f07fcd6..8be32fd9f762 100644
> > --- a/drivers/dma/ti/edma.c
> > +++ b/drivers/dma/ti/edma.c
> > @@ -2282,16 +2282,18 @@ static int edma_probe(struct platform_device *pdev)
> >       ret = pm_runtime_get_sync(dev);
> >       if (ret < 0) {
> >               dev_err(dev, "pm_runtime_get_sync() failed\n");
> > -             return ret;
> > +             goto err_disable_pm;
>
> Why would you disable here when sync has returned error?
>

Since pm_runtime_enable is called before sync, we cannot directly return
without disabling it as the original implementation does.

> >       }
> >
> >       ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> >       if (ret)
> > -             return ret;
> > +             goto err_disable_pm;
> >
> >       ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
> > -     if (!ecc)
> > -             return -ENOMEM;
> > +     if (!ecc) {
> > +             ret = -ENOMEM;
> > +             goto err_disable_pm;
> > +     }
> >
> >       ecc->dev = dev;
> >       ecc->id = pdev->id;
> > @@ -2306,30 +2308,37 @@ static int edma_probe(struct platform_device *pdev)
> >               mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >               if (!mem) {
> >                       dev_err(dev, "no mem resource?\n");
> > -                     return -ENODEV;
> > +                     ret = -ENODEV;
> > +                     goto err_disable_pm;
> >               }
> >       }
> >       ecc->base = devm_ioremap_resource(dev, mem);
> > -     if (IS_ERR(ecc->base))
> > -             return PTR_ERR(ecc->base);
> > +     if (IS_ERR(ecc->base)) {
> > +             ret = PTR_ERR(ecc->base);
> > +             goto err_disable_pm;
> > +     }
> >
> >       platform_set_drvdata(pdev, ecc);
> >
> >       /* Get eDMA3 configuration from IP */
> >       ret = edma_setup_from_hw(dev, info, ecc);
> >       if (ret)
> > -             return ret;
> > +             goto err_disable_pm;
> >
> >       /* Allocate memory based on the information we got from the IP */
> >       ecc->slave_chans = devm_kcalloc(dev, ecc->num_channels,
> >                                       sizeof(*ecc->slave_chans), GFP_KERNEL);
> > -     if (!ecc->slave_chans)
> > -             return -ENOMEM;
> > +     if (!ecc->slave_chans) {
> > +             ret = -ENOMEM;
> > +             goto err_disable_pm;
> > +     }
> >
> >       ecc->slot_inuse = devm_kcalloc(dev, BITS_TO_LONGS(ecc->num_slots),
> >                                      sizeof(unsigned long), GFP_KERNEL);
> > -     if (!ecc->slot_inuse)
> > -             return -ENOMEM;
> > +     if (!ecc->slot_inuse) {
> > +             ret = -ENOMEM;
> > +             goto err_disable_pm;
> > +     }
> >
> >       ecc->default_queue = info->default_queue;
> >
> > @@ -2368,7 +2377,7 @@ static int edma_probe(struct platform_device *pdev)
> >                                      ecc);
> >               if (ret) {
> >                       dev_err(dev, "CCINT (%d) failed --> %d\n", irq, ret);
> > -                     return ret;
> > +                     goto err_disable_pm;
> >               }
> >               ecc->ccint = irq;
> >       }
> > @@ -2384,7 +2393,7 @@ static int edma_probe(struct platform_device *pdev)
> >                                      ecc);
> >               if (ret) {
> >                       dev_err(dev, "CCERRINT (%d) failed --> %d\n", irq, ret);
> > -                     return ret;
> > +                     goto err_disable_pm;
> >               }
> >               ecc->ccerrint = irq;
> >       }
> > @@ -2392,7 +2401,8 @@ static int edma_probe(struct platform_device *pdev)
> >       ecc->dummy_slot = edma_alloc_slot(ecc, EDMA_SLOT_ANY);
> >       if (ecc->dummy_slot < 0) {
> >               dev_err(dev, "Can't allocate PaRAM dummy slot\n");
> > -             return ecc->dummy_slot;
> > +             ret = ecc->dummy_slot;
> > +             goto err_disable_pm;
> >       }
> >
> >       queue_priority_mapping = info->queue_priority_mapping;
> > @@ -2473,6 +2483,8 @@ static int edma_probe(struct platform_device *pdev)
> >
> >  err_reg1:
> >       edma_free_slot(ecc, ecc->dummy_slot);
> > +err_disable_pm:
> > +     pm_runtime_disable(dev);
> >       return ret;
> >  }
> >
> > @@ -2503,6 +2515,7 @@ static int edma_remove(struct platform_device *pdev)
> >       if (ecc->dma_memcpy)
> >               dma_async_device_unregister(ecc->dma_memcpy);
> >       edma_free_slot(ecc, ecc->dummy_slot);
> > +     pm_runtime_disable(dev);
> >
> >       return 0;
> >  }
> > --
> > 2.24.0
>
> --
> ~Vinod

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] dmaengine: ti: edma: add missed pm_runtime_disable
  2019-11-22  5:39   ` Chuhong Yuan
@ 2019-11-22  5:48     ` Vinod Koul
  0 siblings, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2019-11-22  5:48 UTC (permalink / raw)
  To: Chuhong Yuan, Peter Ujfalusi; +Cc: Dan Williams, dmaengine, LKML

On 22-11-19, 13:39, Chuhong Yuan wrote:
> On Fri, Nov 22, 2019 at 1:23 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 18-11-19, 15:37, Chuhong Yuan wrote:
> > > The driver forgets to call pm_runtime_disable in probe failure and
> > > remove.
> > > Add the calls and modify probe failure handling to fix it.
> > >
> > > Fixes: 2b6b3b742019 ("ARM/dmaengine: edma: Merge the two drivers under drivers/dma/")
> > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > > ---
> > >  drivers/dma/ti/edma.c | 43 ++++++++++++++++++++++++++++---------------
> > >  1 file changed, 28 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c
> > > index ba7c4f07fcd6..8be32fd9f762 100644
> > > --- a/drivers/dma/ti/edma.c
> > > +++ b/drivers/dma/ti/edma.c
> > > @@ -2282,16 +2282,18 @@ static int edma_probe(struct platform_device *pdev)
> > >       ret = pm_runtime_get_sync(dev);
> > >       if (ret < 0) {
> > >               dev_err(dev, "pm_runtime_get_sync() failed\n");
> > > -             return ret;
> > > +             goto err_disable_pm;
> >
> > Why would you disable here when sync has returned error?
> >
> 
> Since pm_runtime_enable is called before sync, we cannot directly return
> without disabling it as the original implementation does.

Ok You should have cced Peter and TI folks on this. get_mainatiner.pl is
your friend, use it!

Peter care to test/review this. 
> 
> > >       }
> > >
> > >       ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > >       if (ret)
> > > -             return ret;
> > > +             goto err_disable_pm;
> > >
> > >       ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
> > > -     if (!ecc)
> > > -             return -ENOMEM;
> > > +     if (!ecc) {
> > > +             ret = -ENOMEM;
> > > +             goto err_disable_pm;
> > > +     }
> > >
> > >       ecc->dev = dev;
> > >       ecc->id = pdev->id;
> > > @@ -2306,30 +2308,37 @@ static int edma_probe(struct platform_device *pdev)
> > >               mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >               if (!mem) {
> > >                       dev_err(dev, "no mem resource?\n");
> > > -                     return -ENODEV;
> > > +                     ret = -ENODEV;
> > > +                     goto err_disable_pm;
> > >               }
> > >       }
> > >       ecc->base = devm_ioremap_resource(dev, mem);
> > > -     if (IS_ERR(ecc->base))
> > > -             return PTR_ERR(ecc->base);
> > > +     if (IS_ERR(ecc->base)) {
> > > +             ret = PTR_ERR(ecc->base);
> > > +             goto err_disable_pm;
> > > +     }
> > >
> > >       platform_set_drvdata(pdev, ecc);
> > >
> > >       /* Get eDMA3 configuration from IP */
> > >       ret = edma_setup_from_hw(dev, info, ecc);
> > >       if (ret)
> > > -             return ret;
> > > +             goto err_disable_pm;
> > >
> > >       /* Allocate memory based on the information we got from the IP */
> > >       ecc->slave_chans = devm_kcalloc(dev, ecc->num_channels,
> > >                                       sizeof(*ecc->slave_chans), GFP_KERNEL);
> > > -     if (!ecc->slave_chans)
> > > -             return -ENOMEM;
> > > +     if (!ecc->slave_chans) {
> > > +             ret = -ENOMEM;
> > > +             goto err_disable_pm;
> > > +     }
> > >
> > >       ecc->slot_inuse = devm_kcalloc(dev, BITS_TO_LONGS(ecc->num_slots),
> > >                                      sizeof(unsigned long), GFP_KERNEL);
> > > -     if (!ecc->slot_inuse)
> > > -             return -ENOMEM;
> > > +     if (!ecc->slot_inuse) {
> > > +             ret = -ENOMEM;
> > > +             goto err_disable_pm;
> > > +     }
> > >
> > >       ecc->default_queue = info->default_queue;
> > >
> > > @@ -2368,7 +2377,7 @@ static int edma_probe(struct platform_device *pdev)
> > >                                      ecc);
> > >               if (ret) {
> > >                       dev_err(dev, "CCINT (%d) failed --> %d\n", irq, ret);
> > > -                     return ret;
> > > +                     goto err_disable_pm;
> > >               }
> > >               ecc->ccint = irq;
> > >       }
> > > @@ -2384,7 +2393,7 @@ static int edma_probe(struct platform_device *pdev)
> > >                                      ecc);
> > >               if (ret) {
> > >                       dev_err(dev, "CCERRINT (%d) failed --> %d\n", irq, ret);
> > > -                     return ret;
> > > +                     goto err_disable_pm;
> > >               }
> > >               ecc->ccerrint = irq;
> > >       }
> > > @@ -2392,7 +2401,8 @@ static int edma_probe(struct platform_device *pdev)
> > >       ecc->dummy_slot = edma_alloc_slot(ecc, EDMA_SLOT_ANY);
> > >       if (ecc->dummy_slot < 0) {
> > >               dev_err(dev, "Can't allocate PaRAM dummy slot\n");
> > > -             return ecc->dummy_slot;
> > > +             ret = ecc->dummy_slot;
> > > +             goto err_disable_pm;
> > >       }
> > >
> > >       queue_priority_mapping = info->queue_priority_mapping;
> > > @@ -2473,6 +2483,8 @@ static int edma_probe(struct platform_device *pdev)
> > >
> > >  err_reg1:
> > >       edma_free_slot(ecc, ecc->dummy_slot);
> > > +err_disable_pm:
> > > +     pm_runtime_disable(dev);
> > >       return ret;
> > >  }
> > >
> > > @@ -2503,6 +2515,7 @@ static int edma_remove(struct platform_device *pdev)
> > >       if (ecc->dma_memcpy)
> > >               dma_async_device_unregister(ecc->dma_memcpy);
> > >       edma_free_slot(ecc, ecc->dummy_slot);
> > > +     pm_runtime_disable(dev);
> > >
> > >       return 0;
> > >  }
> > > --
> > > 2.24.0
> >
> > --
> > ~Vinod

-- 
~Vinod

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] dmaengine: ti: edma: add missed pm_runtime_disable
  2019-11-18  7:37 [PATCH 1/2] dmaengine: ti: edma: add missed pm_runtime_disable Chuhong Yuan
  2019-11-22  5:23 ` Vinod Koul
@ 2019-11-22  7:50 ` Peter Ujfalusi
  1 sibling, 0 replies; 5+ messages in thread
From: Peter Ujfalusi @ 2019-11-22  7:50 UTC (permalink / raw)
  To: Chuhong Yuan; +Cc: Vinod Koul, Dan Williams, dmaengine, linux-kernel

Hi,

On 18/11/2019 9.37, Chuhong Yuan wrote:
> The driver forgets to call pm_runtime_disable in probe failure and
> remove.
> Add the calls and modify probe failure handling to fix it.
> 
> Fixes: 2b6b3b742019 ("ARM/dmaengine: edma: Merge the two drivers under drivers/dma/")
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> ---
>  drivers/dma/ti/edma.c | 43 ++++++++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c
> index ba7c4f07fcd6..8be32fd9f762 100644
> --- a/drivers/dma/ti/edma.c
> +++ b/drivers/dma/ti/edma.c
> @@ -2282,16 +2282,18 @@ static int edma_probe(struct platform_device *pdev)

Please move the pm_runtime_enable/get section just before
edma_setup_from_hw()

>  	ret = pm_runtime_get_sync(dev);
>  	if (ret < 0) {
>  		dev_err(dev, "pm_runtime_get_sync() failed\n");
> -		return ret;
> +		goto err_disable_pm;

Here you need:
	pm_runtime_disable(dev);
	return ret;

>  	}
>  
>  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>  	if (ret)
> -		return ret;
> +		goto err_disable_pm;
>  
>  	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
> -	if (!ecc)
> -		return -ENOMEM;
> +	if (!ecc) {
> +		ret = -ENOMEM;
> +		goto err_disable_pm;
> +	}
>  
>  	ecc->dev = dev;
>  	ecc->id = pdev->id;
> @@ -2306,30 +2308,37 @@ static int edma_probe(struct platform_device *pdev)
>  		mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  		if (!mem) {
>  			dev_err(dev, "no mem resource?\n");
> -			return -ENODEV;
> +			ret = -ENODEV;
> +			goto err_disable_pm;
>  		}
>  	}
>  	ecc->base = devm_ioremap_resource(dev, mem);
> -	if (IS_ERR(ecc->base))
> -		return PTR_ERR(ecc->base);
> +	if (IS_ERR(ecc->base)) {
> +		ret = PTR_ERR(ecc->base);
> +		goto err_disable_pm;
> +	}

None of the above changes needed since the pm_runtime initialization is
moved

>  	platform_set_drvdata(pdev, ecc);

here.

>  	/* Get eDMA3 configuration from IP */
>  	ret = edma_setup_from_hw(dev, info, ecc);
>  	if (ret)
> -		return ret;
> +		goto err_disable_pm;
>  
>  	/* Allocate memory based on the information we got from the IP */
>  	ecc->slave_chans = devm_kcalloc(dev, ecc->num_channels,
>  					sizeof(*ecc->slave_chans), GFP_KERNEL);
> -	if (!ecc->slave_chans)
> -		return -ENOMEM;
> +	if (!ecc->slave_chans) {
> +		ret = -ENOMEM;
> +		goto err_disable_pm;
> +	}
>  
>  	ecc->slot_inuse = devm_kcalloc(dev, BITS_TO_LONGS(ecc->num_slots),
>  				       sizeof(unsigned long), GFP_KERNEL);
> -	if (!ecc->slot_inuse)
> -		return -ENOMEM;
> +	if (!ecc->slot_inuse) {
> +		ret = -ENOMEM;
> +		goto err_disable_pm;
> +	}

and this does not apply since we have the
	ecc->channels_mask = devm_kcalloc()
in here.

If you rebase, then I would suggest to combine the memory allocation
checks into one:
if (!ecc->slave_chans || !ecc->slot_inuse | !ecc->channels_mask) {
	ret = -ENOMEM;
	goto err_disable_pm;
}

>  
>  	ecc->default_queue = info->default_queue;
>  
> @@ -2368,7 +2377,7 @@ static int edma_probe(struct platform_device *pdev)
>  				       ecc);
>  		if (ret) {
>  			dev_err(dev, "CCINT (%d) failed --> %d\n", irq, ret);
> -			return ret;
> +			goto err_disable_pm;
>  		}
>  		ecc->ccint = irq;
>  	}
> @@ -2384,7 +2393,7 @@ static int edma_probe(struct platform_device *pdev)
>  				       ecc);
>  		if (ret) {
>  			dev_err(dev, "CCERRINT (%d) failed --> %d\n", irq, ret);
> -			return ret;
> +			goto err_disable_pm;
>  		}
>  		ecc->ccerrint = irq;
>  	}
> @@ -2392,7 +2401,8 @@ static int edma_probe(struct platform_device *pdev)
>  	ecc->dummy_slot = edma_alloc_slot(ecc, EDMA_SLOT_ANY);
>  	if (ecc->dummy_slot < 0) {
>  		dev_err(dev, "Can't allocate PaRAM dummy slot\n");
> -		return ecc->dummy_slot;
> +		ret = ecc->dummy_slot;
> +		goto err_disable_pm;
>  	}
>  
>  	queue_priority_mapping = info->queue_priority_mapping;
> @@ -2473,6 +2483,8 @@ static int edma_probe(struct platform_device *pdev)
>  
>  err_reg1:
>  	edma_free_slot(ecc, ecc->dummy_slot);
> +err_disable_pm:

Please add:
	pm_runtime_put_sync(dev);

> +	pm_runtime_disable(dev);
>  	return ret;
>  }
>  
> @@ -2503,6 +2515,7 @@ static int edma_remove(struct platform_device *pdev)
>  	if (ecc->dma_memcpy)
>  		dma_async_device_unregister(ecc->dma_memcpy);
>  	edma_free_slot(ecc, ecc->dummy_slot);

Here also:
	pm_runtime_put_sync(dev);


> +	pm_runtime_disable(dev);
>  
>  	return 0;
>  }
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-18  7:37 [PATCH 1/2] dmaengine: ti: edma: add missed pm_runtime_disable Chuhong Yuan
2019-11-22  5:23 ` Vinod Koul
2019-11-22  5:39   ` Chuhong Yuan
2019-11-22  5:48     ` Vinod Koul
2019-11-22  7:50 ` Peter Ujfalusi

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org
	public-inbox-index dmaengine

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git