Re: [PATCHv4 0/4] ata: sata_dwc_460ex: cleanups

On Wed, Aug 12, 2026 at 2:41 AM Uwe Kleine-König
<[email protected]> wrote:
>
> Hello,
>
> On Mon, Jul 13, 2026 at 04:31:53PM +0900, Damien Le Moal wrote:
> > On 7/13/26 06:37, Rosen Penev wrote:
> > > Fix various issues flagged by Sashiko against the original submission of this driver.
> > >
> > > v4: remove interrupt fix
> > > v3: Shrink series to Fixes on the initial commit.
> > > v2: sashiko fixes.
> > >
> > > Rosen Penev (4):
> > >   ata: sata_dwc_460ex: use platform_get_irq()
> > >   ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is
> > >     registered
> > >   ata: sata_dwc_460ex: fix clear_interrupt_bit() clearing all pending
> > >     interrupts
> > >   ata: sata_dwc_460ex: fix infinite loop in NCQ tag completion
> > >     bit-scanning
> > >
> > >  drivers/ata/sata_dwc_460ex.c | 38 ++++++++++++------------------------
> > >  1 file changed, 12 insertions(+), 26 deletions(-)
> >
> > I applied this to for-7.2-fixes, but I reversed the first 2 patches.
> > Thanks!
> >
> > (if you have time, please send further cleanups to address the other issues
> > that sashiko signaled).
>
> I think the analysis for the fourth patch is wrong (or incomplete), the
> original code was (a bit simplified):
>
>         unsigned char tag;
>         unsigned int tag_mask;
>         ...
>         tag_mask = ...;
>         ...
>         tag = 0;
>         while (tag_mask) {
>                 while (!(tag_mask & 0x1)) {
>                         tag++;
>                         tag_mask <<= 1;
>                 }
>
>                 tag_mask &= ~0x1;
>                 ...
>         }
>
> Given that tag_mask is shifted left (and not right) the inner while loop
> yields an endless loop whenever tag_mask's least significant bit isn't
> set initially. Given the outer loop this results in a hang if tag_mask
> != 1. So the issue doesn't only trigger for tag_mask = 0x80000000.
>
> Either this never worked, or the problem doesn't trigger reaching that
> code with tag_mask != 1 easily. And I also wonder if the change's
> urgency was considered carefully enough to justify a commit in -rc4 to
> fix a bug that is already roughly 16 years old.
>
> And similar for the 3 parents of that change
> (c2130f6553f4a5cbdc259de069600117a995f197):
>
> For 4bbc16a353a98023e5ddfca7c1fc0e49971cf4d0 I wonder: Does
> ata_host_activate() already need the irqs enabled? If yes, the commit
> is wrong.
>
> For a4af122106f73ea510bb35a9ea1dedd980fc0db7 I think it's bold to claim
> "Also fix unused variable when CONFIG_SATA_DWC_OLD_DMA is disabled."
> given that the unused variable warning (I guess about np) was only
> introduced during development of this patch.
>
> For 66c4e310ad71f41e41736d33dd8a1fb5eaaec7f3 it disturbs me that the
> commit log has: "If INTPR uses standard Write-1-to-Clear semantics,
> [...]". Without that the justification of the patch goes away, nobody
> checked that?
>
> All four commits have an Assisted-by tag, and I have the impression that
> nobody involved in these commits has the hardware or even the hardware
> documentation. But maybe I'm just to picky about changes that enter the
> mainline in the stabilization phase. 🤷
I personally do not have the hardware. I know of one OpenWrt user that
does. I'm sure I'll get notified if something breaks.
>
> Best regards
> Uwe