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

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 3056 bytes --]

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. 🤷

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]