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

[Damien Le Moal <dlemoal@kernel.org>]

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;
 }

 static u64 ata_dev_get_max_sec_quirk_value(struct ata_device *dev)

With this, all quirks that apply to a device will apply, even if there are
multiple matching entries. That will solve the issue and make things a lot more
solid.

> ---
>  drivers/ata/libata-core.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 92233fb25051..cff4ededafad 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4323,7 +4323,8 @@ static const struct ata_dev_quirks_entry __ata_dev_quirks[] = {
>  	{ "MAXTOR 6L080L4",	"A93.0500",	ATA_QUIRK_BROKEN_HPA },
>  
>  	/* this one allows HPA unlocking but fails IOs on the area */
> -	{ "OCZ-VERTEX",		    "1.30",	ATA_QUIRK_BROKEN_HPA },
> +	{ "OCZ-VERTEX",		    "1.30",	ATA_QUIRK_BROKEN_HPA |
> +						ATA_QUIRK_NO_FUA },
>  
>  	/* Devices which report 1 sector over size HPA */
>  	{ "ST340823A",		NULL,		ATA_QUIRK_HPA_SIZE },
> @@ -4459,6 +4460,8 @@ static const struct ata_dev_quirks_entry __ata_dev_quirks[] = {
>  	{ "Crucial*",			NULL,	ATA_QUIRK_ZERO_AFTER_TRIM },
>  	{ "INTEL SSDSC2KG480G8", "XCV10120",	ATA_QUIRK_ZERO_AFTER_TRIM |
>  						ATA_QUIRK_MAX_SEC },
> +	{ "INTEL*SSDSC2CT*",		NULL,	ATA_QUIRK_ZERO_AFTER_TRIM |
> +						ATA_QUIRK_NO_FUA },
>  	{ "INTEL*SSD*",			NULL,	ATA_QUIRK_ZERO_AFTER_TRIM },
>  	{ "SSD*INTEL*",			NULL,	ATA_QUIRK_ZERO_AFTER_TRIM },
>  	{ "Samsung*SSD*",		NULL,	ATA_QUIRK_ZERO_AFTER_TRIM },
> @@ -4479,7 +4482,8 @@ static const struct ata_dev_quirks_entry __ata_dev_quirks[] = {
>  	{ "WDC WD1200JD-*",		NULL,	ATA_QUIRK_WD_BROKEN_LPM },
>  	{ "WDC WD1600JD-*",		NULL,	ATA_QUIRK_WD_BROKEN_LPM },
>  	{ "WDC WD2000JD-*",		NULL,	ATA_QUIRK_WD_BROKEN_LPM },
> -	{ "WDC WD2500JD-*",		NULL,	ATA_QUIRK_WD_BROKEN_LPM },
> +	{ "WDC WD2500JD-*",		NULL,	ATA_QUIRK_WD_BROKEN_LPM |
> +						ATA_QUIRK_NO_FUA },
>  	{ "WDC WD3000JD-*",		NULL,	ATA_QUIRK_WD_BROKEN_LPM },
>  	{ "WDC WD3200JD-*",		NULL,	ATA_QUIRK_WD_BROKEN_LPM },
>  
> @@ -4504,7 +4508,6 @@ static const struct ata_dev_quirks_entry __ata_dev_quirks[] = {
>  	{ "Maxtor",		"BANC1G10",	ATA_QUIRK_NO_FUA },
>  	{ "WDC*WD2500J*",	NULL,		ATA_QUIRK_NO_FUA },
>  	{ "OCZ-VERTEX*",	NULL,		ATA_QUIRK_NO_FUA },
> -	{ "INTEL*SSDSC2CT*",	NULL,		ATA_QUIRK_NO_FUA },
>  
>  	/* End Marker */
>  	{ }


-- 
Damien Le Moal
Western Digital Research