Re: [PATCHv2 4/8] ata: sata_dwc_460ex: drop redundant struct copy of port_info

On Wed, Jul 1, 2026 at 5:13 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Returning success (0) on `ata_host_activate()` failure causes a kernel panic on device removal.
> - [High] Missing cleanup of DMA resources on probe error paths leads to Use-After-Free.
Both fixed later in the series. This thing is still broken when
changing [PATCH] to something else.
> --
>
> commit 1231cd1ac0d6ac3ad6a4b61a9f9cf20df21198d9
> Author: Rosen Penev <[email protected]>
> Subject: ata: sata_dwc_460ex: drop redundant struct copy of port_info
>
> This patch optimizes sata_dwc_probe() by dropping an unnecessary local
> copy of sata_dwc_port_info[0]. It directly points to the static data
> instead, saving stack space and correcting the const qualification.
>
> > diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> > index 4fc22ce4bd9aa..f262b00384ac4 100644
> > --- a/drivers/ata/sata_dwc_460ex.c
> > +++ b/drivers/ata/sata_dwc_460ex.c
> > @@ -1134,8 +1134,8 @@ static int sata_dwc_probe(struct platform_device *ofdev)
> >       int err = 0;
> >       int irq;
> >       struct ata_host *host;
> > -     struct ata_port_info pi = sata_dwc_port_info[0];
> > -     const struct ata_port_info *ppi[] = { &pi, NULL };
> > +     const struct ata_port_info *pi = &sata_dwc_port_info[0];
> > +     const struct ata_port_info *ppi[] = { pi, NULL };
> >       struct resource *res;
> >
> >       /* Allocate DWC SATA device */
>
> [Severity: High]
> This is a pre-existing issue, but does sata_dwc_probe() correctly handle
> ata_host_activate() failures?
>
> Looking further down in drivers/ata/sata_dwc_460ex.c:sata_dwc_probe(), the
> driver logs an error but returns 0 if activation fails:
>
>         err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
>         if (err)
>                 dev_err(dev, "failed to activate host");
>
>         return 0;
>
> If ata_host_activate() fails (for example, if devm_request_irq() or
> scsi_host_alloc() fails), returning 0 falsely reports successful
> initialization to the driver core.
>
> When the device is later unbound or removed, sata_dwc_remove() is invoked
> and calls ata_host_detach() which calls ata_port_detach(). Since the
> scsi_host was never properly allocated or was freed, scsi_remove_host()
> will dereference a null pointer and cause a kernel panic.
>
> Could the return statement be updated to return the error code instead?
>
>
> [Severity: High]
> This is a pre-existing issue, but does sata_dwc_probe() leak dma resources
> on error paths?
>
> If CONFIG_SATA_DWC_OLD_DMA is enabled and no "dmas" property is present,
> sata_dwc_dma_init_old() is called:
>
>         err = sata_dwc_dma_init_old(ofdev, hsdev);
>         if (err)
>                 return err;
>
> This allocates a dma controller via devm_kzalloc, requests an interrupt, and
> registers it with the dma engine subsystem.
>
> If a subsequent probe step fails, such as the phy retrieval:
>
>         if (IS_ERR(hsdev->phy))
>                 return PTR_ERR(hsdev->phy);
>
> or later in the main error path:
>
> error_out:
>         phy_exit(hsdev->phy);
>         return err;
>
> The function returns without calling sata_dwc_dma_exit_old(). The driver core
> will free the devm-managed memory, including the dma controller structures.
> However, because explicit teardown was bypassed, the dma engine subsystem and
> the interrupt layer will retain pointers to this freed memory. This can lead
> to a use-after-free if the shared interrupt fires or the dma subsystem
> accesses the controller.
>
> Should the error paths be updated to call sata_dwc_dma_exit_old() instead?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1