[PATCH] dmaengine: axi-dmac: move to device managed probe

[PATCH] dmaengine: axi-dmac: move to device managed probe

[Nuno Sa]

In axi_dmac_probe(), there's a mix in using device managed APIs and
explicitly cleaning things in the driver .remove() hook. Move to use
device managed APIs and thus drop the .remove() hook.

While doing this move request_irq() before of_dma_controller_register()
so the previous cleanup order in the .remove() hook is preserved.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/dma/dma-axi-dmac.c | 80 ++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 45 deletions(-)

diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index 4e339c04fc1e..24c911dc3161 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -1002,6 +1002,16 @@ static int axi_dmac_detect_caps(struct axi_dmac *dmac, unsigned int version)
 	return 0;
 }
 
+static void axi_dmac_tasklet_kill(void *task)
+{
+	tasklet_kill(task);
+}
+
+static void axi_dmac_free_dma_controller(void *of_node)
+{
+	of_dma_controller_free(of_node);
+}
+
 static int axi_dmac_probe(struct platform_device *pdev)
 {
 	struct dma_device *dma_dev;
@@ -1025,14 +1035,10 @@ static int axi_dmac_probe(struct platform_device *pdev)
 	if (IS_ERR(dmac->base))
 		return PTR_ERR(dmac->base);
 
-	dmac->clk = devm_clk_get(&pdev->dev, NULL);
+	dmac->clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(dmac->clk))
 		return PTR_ERR(dmac->clk);
 
-	ret = clk_prepare_enable(dmac->clk);
-	if (ret < 0)
-		return ret;
-
 	version = axi_dmac_read(dmac, ADI_AXI_REG_VERSION);
 
 	if (version >= ADI_AXI_PCORE_VER(4, 3, 'a'))
@@ -1041,7 +1047,7 @@ static int axi_dmac_probe(struct platform_device *pdev)
 		ret = axi_dmac_parse_dt(&pdev->dev, dmac);
 
 	if (ret < 0)
-		goto err_clk_disable;
+		return ret;
 
 	INIT_LIST_HEAD(&dmac->chan.active_descs);
 
@@ -1072,7 +1078,7 @@ static int axi_dmac_probe(struct platform_device *pdev)
 
 	ret = axi_dmac_detect_caps(dmac, version);
 	if (ret)
-		goto err_clk_disable;
+		return ret;
 
 	dma_dev->copy_align = (dmac->chan.address_align_mask + 1);
 
@@ -1088,57 +1094,42 @@ static int axi_dmac_probe(struct platform_device *pdev)
 		    !AXI_DMAC_DST_COHERENT_GET(ret)) {
 			dev_err(dmac->dma_dev.dev,
 				"Coherent DMA not supported in hardware");
-			ret = -EINVAL;
-			goto err_clk_disable;
+			return -EINVAL;
 		}
 	}
 
-	ret = dma_async_device_register(dma_dev);
+	ret = dmaenginem_async_device_register(dma_dev);
+	if (ret)
+		return ret;
+
+	/*
+	 * Put the action in here so it get's done before unregistering the DMA
+	 * device.
+	 */
+	ret = devm_add_action_or_reset(&pdev->dev, axi_dmac_tasklet_kill,
+				       &dmac->chan.vchan.task);
+	if (ret)
+		return ret;
+
+	ret = devm_request_irq(&pdev->dev, dmac->irq, axi_dmac_interrupt_handler,
+			       IRQF_SHARED, dev_name(&pdev->dev), dmac);
 	if (ret)
-		goto err_clk_disable;
+		return ret;
 
 	ret = of_dma_controller_register(pdev->dev.of_node,
 		of_dma_xlate_by_chan_id, dma_dev);
 	if (ret)
-		goto err_unregister_device;
+		return ret;
 
-	ret = request_irq(dmac->irq, axi_dmac_interrupt_handler, IRQF_SHARED,
-		dev_name(&pdev->dev), dmac);
+	ret = devm_add_action_or_reset(&pdev->dev, axi_dmac_free_dma_controller,
+				       pdev->dev.of_node);
 	if (ret)
-		goto err_unregister_of;
-
-	platform_set_drvdata(pdev, dmac);
+		return ret;
 
 	regmap = devm_regmap_init_mmio(&pdev->dev, dmac->base,
 		 &axi_dmac_regmap_config);
-	if (IS_ERR(regmap)) {
-		ret = PTR_ERR(regmap);
-		goto err_free_irq;
-	}
-
-	return 0;
-
-err_free_irq:
-	free_irq(dmac->irq, dmac);
-err_unregister_of:
-	of_dma_controller_free(pdev->dev.of_node);
-err_unregister_device:
-	dma_async_device_unregister(&dmac->dma_dev);
-err_clk_disable:
-	clk_disable_unprepare(dmac->clk);
-
-	return ret;
-}
-
-static void axi_dmac_remove(struct platform_device *pdev)
-{
-	struct axi_dmac *dmac = platform_get_drvdata(pdev);
 
-	of_dma_controller_free(pdev->dev.of_node);
-	free_irq(dmac->irq, dmac);
-	tasklet_kill(&dmac->chan.vchan.task);
-	dma_async_device_unregister(&dmac->dma_dev);
-	clk_disable_unprepare(dmac->clk);
+	return PTR_ERR_OR_ZERO(regmap);
 }
 
 static const struct of_device_id axi_dmac_of_match_table[] = {
@@ -1153,7 +1144,6 @@ static struct platform_driver axi_dmac_driver = {
 		.of_match_table = axi_dmac_of_match_table,
 	},
 	.probe = axi_dmac_probe,
-	.remove_new = axi_dmac_remove,
 };
 module_platform_driver(axi_dmac_driver);
 

---
base-commit: de7d9cb3b064fdfb2e0e7706d14ffee20b762ad2
change-id: 20240214-axi-dmac-devm-probe-d718ef36fb58
--

Thanks!
- Nuno Sá

Re: [PATCH] dmaengine: axi-dmac: move to device managed probe

[Vinod Koul]

On 14-02-24, 13:29, Nuno Sa wrote:
> In axi_dmac_probe(), there's a mix in using device managed APIs and
> explicitly cleaning things in the driver .remove() hook. Move to use
> device managed APIs and thus drop the .remove() hook.
> 
> While doing this move request_irq() before of_dma_controller_register()
> so the previous cleanup order in the .remove() hook is preserved.

It is good that we are doing this, but moving irq to devm doesnt help a
lot, there exists a race

I would suggest you use the axi_dmac_free_dma_controller() to all free
up irq explicitly, that will ensure things no chance of irq firing and
scheduling tasklet when we are removing

> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/dma/dma-axi-dmac.c | 80 ++++++++++++++++++++--------------------------
>  1 file changed, 35 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
> index 4e339c04fc1e..24c911dc3161 100644
> --- a/drivers/dma/dma-axi-dmac.c
> +++ b/drivers/dma/dma-axi-dmac.c
> @@ -1002,6 +1002,16 @@ static int axi_dmac_detect_caps(struct axi_dmac *dmac, unsigned int version)
>  	return 0;
>  }
>  
> +static void axi_dmac_tasklet_kill(void *task)
> +{
> +	tasklet_kill(task);
> +}
> +
> +static void axi_dmac_free_dma_controller(void *of_node)
> +{
> +	of_dma_controller_free(of_node);
> +}
> +
>  static int axi_dmac_probe(struct platform_device *pdev)
>  {
>  	struct dma_device *dma_dev;
> @@ -1025,14 +1035,10 @@ static int axi_dmac_probe(struct platform_device *pdev)
>  	if (IS_ERR(dmac->base))
>  		return PTR_ERR(dmac->base);
>  
> -	dmac->clk = devm_clk_get(&pdev->dev, NULL);
> +	dmac->clk = devm_clk_get_enabled(&pdev->dev, NULL);
>  	if (IS_ERR(dmac->clk))
>  		return PTR_ERR(dmac->clk);
>  
> -	ret = clk_prepare_enable(dmac->clk);
> -	if (ret < 0)
> -		return ret;
> -
>  	version = axi_dmac_read(dmac, ADI_AXI_REG_VERSION);
>  
>  	if (version >= ADI_AXI_PCORE_VER(4, 3, 'a'))
> @@ -1041,7 +1047,7 @@ static int axi_dmac_probe(struct platform_device *pdev)
>  		ret = axi_dmac_parse_dt(&pdev->dev, dmac);
>  
>  	if (ret < 0)
> -		goto err_clk_disable;
> +		return ret;
>  
>  	INIT_LIST_HEAD(&dmac->chan.active_descs);
>  
> @@ -1072,7 +1078,7 @@ static int axi_dmac_probe(struct platform_device *pdev)
>  
>  	ret = axi_dmac_detect_caps(dmac, version);
>  	if (ret)
> -		goto err_clk_disable;
> +		return ret;
>  
>  	dma_dev->copy_align = (dmac->chan.address_align_mask + 1);
>  
> @@ -1088,57 +1094,42 @@ static int axi_dmac_probe(struct platform_device *pdev)
>  		    !AXI_DMAC_DST_COHERENT_GET(ret)) {
>  			dev_err(dmac->dma_dev.dev,
>  				"Coherent DMA not supported in hardware");
> -			ret = -EINVAL;
> -			goto err_clk_disable;
> +			return -EINVAL;
>  		}
>  	}
>  
> -	ret = dma_async_device_register(dma_dev);
> +	ret = dmaenginem_async_device_register(dma_dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Put the action in here so it get's done before unregistering the DMA
> +	 * device.
> +	 */
> +	ret = devm_add_action_or_reset(&pdev->dev, axi_dmac_tasklet_kill,
> +				       &dmac->chan.vchan.task);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_irq(&pdev->dev, dmac->irq, axi_dmac_interrupt_handler,
> +			       IRQF_SHARED, dev_name(&pdev->dev), dmac);
>  	if (ret)
> -		goto err_clk_disable;
> +		return ret;
>  
>  	ret = of_dma_controller_register(pdev->dev.of_node,
>  		of_dma_xlate_by_chan_id, dma_dev);
>  	if (ret)
> -		goto err_unregister_device;
> +		return ret;
>  
> -	ret = request_irq(dmac->irq, axi_dmac_interrupt_handler, IRQF_SHARED,
> -		dev_name(&pdev->dev), dmac);
> +	ret = devm_add_action_or_reset(&pdev->dev, axi_dmac_free_dma_controller,
> +				       pdev->dev.of_node);
>  	if (ret)
> -		goto err_unregister_of;
> -
> -	platform_set_drvdata(pdev, dmac);
> +		return ret;
>  
>  	regmap = devm_regmap_init_mmio(&pdev->dev, dmac->base,
>  		 &axi_dmac_regmap_config);
> -	if (IS_ERR(regmap)) {
> -		ret = PTR_ERR(regmap);
> -		goto err_free_irq;
> -	}
> -
> -	return 0;
> -
> -err_free_irq:
> -	free_irq(dmac->irq, dmac);
> -err_unregister_of:
> -	of_dma_controller_free(pdev->dev.of_node);
> -err_unregister_device:
> -	dma_async_device_unregister(&dmac->dma_dev);
> -err_clk_disable:
> -	clk_disable_unprepare(dmac->clk);
> -
> -	return ret;
> -}
> -
> -static void axi_dmac_remove(struct platform_device *pdev)
> -{
> -	struct axi_dmac *dmac = platform_get_drvdata(pdev);
>  
> -	of_dma_controller_free(pdev->dev.of_node);
> -	free_irq(dmac->irq, dmac);
> -	tasklet_kill(&dmac->chan.vchan.task);
> -	dma_async_device_unregister(&dmac->dma_dev);
> -	clk_disable_unprepare(dmac->clk);
> +	return PTR_ERR_OR_ZERO(regmap);
>  }
>  
>  static const struct of_device_id axi_dmac_of_match_table[] = {
> @@ -1153,7 +1144,6 @@ static struct platform_driver axi_dmac_driver = {
>  		.of_match_table = axi_dmac_of_match_table,
>  	},
>  	.probe = axi_dmac_probe,
> -	.remove_new = axi_dmac_remove,
>  };
>  module_platform_driver(axi_dmac_driver);
>  
> 
> ---
> base-commit: de7d9cb3b064fdfb2e0e7706d14ffee20b762ad2
> change-id: 20240214-axi-dmac-devm-probe-d718ef36fb58
> --
> 
> Thanks!
> - Nuno Sá

-- 
~Vinod

Re: [PATCH] dmaengine: axi-dmac: move to device managed probe

[Nuno Sá]

On Fri, 2024-02-16 at 17:52 +0530, Vinod Koul wrote:
> On 14-02-24, 13:29, Nuno Sa wrote:
> > In axi_dmac_probe(), there's a mix in using device managed APIs and
> > explicitly cleaning things in the driver .remove() hook. Move to use
> > device managed APIs and thus drop the .remove() hook.
> > 
> > While doing this move request_irq() before of_dma_controller_register()
> > so the previous cleanup order in the .remove() hook is preserved.
> 
> It is good that we are doing this, but moving irq to devm doesnt help a
> lot, there exists a race
> 
> I would suggest you use the axi_dmac_free_dma_controller() to all free
> up irq explicitly, that will ensure things no chance of irq firing and
> scheduling tasklet when we are removing
> 

Hi Vinod,

Hmm wait... Before the patch we had:

static void axi_dmac_remove(struct platform_device *pdev)
{
	struct axi_dmac *dmac = platform_get_drvdata(pdev);

	of_dma_controller_free(pdev->dev.of_node);
	free_irq(dmac->irq, dmac);

So, you mean that there's actually a possible bug/race in the above? The irq is only
being released after of_dma_controller_free(). So, I guess I should not move
request_irq() at all and then I can still use devm_request_irq(). If I do it after
registering the device then it will be released before of_dma_controller_free(). Am I
missing something?

Thanks for the feedback!
- Nuno Sá

>