Re: [PATCH] ata: libata-core: Fix NO_FUA quirks shadowed by earlier quirk entries

[Niklas Cassel <cassel@kernel.org>]

On Wed, Oct 14, 2026 at 04:34:55PM +0200, Damien Le Moal wrote:
> On 2026/10/13 21:39, Niklas Cassel wrote:
> > ata_dev_quirks() uses the first entry in __ata_dev_quirks[] that
> > matches the device and ignores all later entries. Commit 38d431229bfb
> > ("ata: libata: exclude FUA support for known buggy drives") appended
> > three ATA_QUIRK_NO_FUA entries to the end of the table, with model
> > patterns meant to cover whole product lines. Earlier entries already
> > match some or all of these drives, so NO_FUA is not always applied:
> > 
> > - "INTEL*SSDSC2CT*" never takes effect. Every model it matches also
> >   matches the earlier "INTEL*SSD*" entry, so these drives only get
> >   ATA_QUIRK_ZERO_AFTER_TRIM.
> > 
> > - "WDC*WD2500J*" does not take effect for "WDC WD2500JD-*" drives,
> >   which match the earlier ATA_QUIRK_WD_BROKEN_LPM entry.
> > 
> > - "OCZ-VERTEX*" does not take effect for "OCZ-VERTEX" drives with
> >   firmware "1.30", which match the earlier ATA_QUIRK_BROKEN_HPA entry.
> > 
> > Move the Intel entry in front of "INTEL*SSD*" and keep
> > ATA_QUIRK_ZERO_AFTER_TRIM in it, so these drives stay on the
> > ZERO_AFTER_TRIM whitelist. Add ATA_QUIRK_NO_FUA to the WD2500JD and
> > OCZ Vertex 1.30 entries. The quirks of all other drives are unchanged.
> > 
> > libata only uses FUA when it is enabled with libata.fua=1, so systems
> > using the default setting are not affected.
> > 
> > Fixes: 38d431229bfb ("ata: libata: exclude FUA support for known buggy drives")
> > Assisted-by: LLM
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> Your solution works, but I think it is very fragile. We will most likely again
> make the same mistake with the oredering. So what about something like this:
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 92233fb25051..980a6a6256b1 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4515,6 +4515,7 @@ static u64 ata_dev_quirks(const struct ata_device *dev)
>         unsigned char model_num[ATA_ID_PROD_LEN + 1];
>         unsigned char model_rev[ATA_ID_FW_REV_LEN + 1];
>         const struct ata_dev_quirks_entry *ad = __ata_dev_quirks;
> +       u64 quirks = 0;
> 
>         /* dev->quirks is an u64. */
>         BUILD_BUG_ON(__ATA_QUIRK_MAX > 64);
> @@ -4527,11 +4528,11 @@ static u64 ata_dev_quirks(const struct ata_device *dev)
>                     (!ad->model_rev || glob_match(ad->model_rev, model_rev))) {
>                         ata_dev_print_quirks(dev, model_num, model_rev,
>                                              ad->quirks);
> -                       return ad->quirks;
> +                       quirks |= ad->quirks;
>                 }
>                 ad++;
>         }
> -       return 0;
> +       return quirks;
>  }

I like the idea.

Send it as a real patch :)


Kind regards,
Niklas