Re: [PATCH] ata: sata_dwc_460ex: use devm for old DMA resource lifetime management

[Damien Le Moal <dlemoal@kernel.org>]

On 6/30/26 09:17, Rosen Penev wrote:
> Convert sata_dwc_dma_exit_old() to a devm action callback and register
> it via devm_add_action_or_reset() after dw_dma_probe() succeeds.  This
> lets the devm framework handle DMA controller teardown automatically on
> probe failure and driver removal.
> 
> As a result the probe function can use plain return-on-error for all
> failure paths — the dma_initialized flag, the goto labels, and the
> explicit sata_dwc_dma_exit_old() call in sata_dwc_remove() are all
> eliminated.
> 
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/ata/sata_dwc_460ex.c | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 35aa7f9acdf7..a1e319deb0b4 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -221,10 +221,18 @@ static int sata_dwc_dma_get_channel_old(struct sata_dwc_device_port *hsdevp)
>  	return 0;
>  }
>  
> +static void sata_dwc_dma_exit_old(void *data)
> +{
> +	struct sata_dwc_device *hsdev = data;
> +
> +	dw_dma_remove(hsdev->dma);
> +}
> +
>  static int sata_dwc_dma_init_old(struct platform_device *pdev,
>  				 struct sata_dwc_device *hsdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	int err;
>  
>  	hsdev->dma = devm_kzalloc(dev, sizeof(*hsdev->dma), GFP_KERNEL);
>  	if (!hsdev->dma)
> @@ -244,15 +252,11 @@ static int sata_dwc_dma_init_old(struct platform_device *pdev,
>  		return PTR_ERR(hsdev->dma->regs);
>  
>  	/* Initialize AHB DMAC */
> -	return dw_dma_probe(hsdev->dma);
> -}
> -
> -static void sata_dwc_dma_exit_old(struct sata_dwc_device *hsdev)
> -{
> -	if (!hsdev->dma)
> -		return;
> +	err = dw_dma_probe(hsdev->dma);
> +	if (err)
> +		return err;
>  
> -	dw_dma_remove(hsdev->dma);
> +	return devm_add_action_or_reset(dev, sata_dwc_dma_exit_old, hsdev);
>  }
>  
>  #endif
> @@ -1187,7 +1191,7 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  
>  	err = phy_init(hsdev->phy);
>  	if (err)
> -		goto error_out;
> +		return err;
>  
>  	/*
>  	 * Now, register with libATA core, this will also initiate the
> @@ -1195,12 +1199,10 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  	 * error_handler() to execute a dummy Softreset EH session
>  	 */
>  	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
> -	if (err)
> -		dev_err(dev, "failed to activate host");
> +	if (!err)
> +		return err;

What about the generic error check pattern:

	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
	if (err) {
		dev_err(dev, "failed to activate host");
		phy_exit(hsdev->phy);
		return err;
	}

	return 0;

Which I find personally much nicer to read.

>  
> -	return 0;
> -
> -error_out:
> +	dev_err(dev, "failed to activate host");
>  	phy_exit(hsdev->phy);
>  	return err;
>  }
> @@ -1215,11 +1217,6 @@ static void sata_dwc_remove(struct platform_device *ofdev)
>  
>  	phy_exit(hsdev->phy);
>  
> -#ifdef CONFIG_SATA_DWC_OLD_DMA
> -	/* Free SATA DMA resources */
> -	sata_dwc_dma_exit_old(hsdev);
> -#endif
> -
>  	dev_dbg(dev, "done\n");
>  }
>  


-- 
Damien Le Moal
Western Digital Research