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

[Rosen Penev]

On Mon, Jun 29, 2026 at 7:06 PM Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> > ---
> >  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.
OK. Will do for v2.

I did it this way to reduce indentation.
>
> >
> > -     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