[PATCH] ata: libata-core: always apply all quirks matching for a device

[PATCH] ata: libata-core: always apply all quirks matching for a device

[Damien Le Moal]

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.

Avoid this particular issue and make ata_dev_quirks() more solid by
getting all quirk flags from all entries of the __ata_dev_quirks array
matching the device model and firmware revision. With this change,
regardless of the order in which entries are added to the __ata_dev_quirks
array, all required quirk flags will always be applied to a device.

Fixes: 38d431229bfb ("ata: libata: exclude FUA support for known buggy drives")
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 682c58889715..e53a410a817e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4514,6 +4514,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);
@@ -4521,16 +4522,18 @@ static u64 ata_dev_quirks(const struct ata_device *dev)
 	ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 	ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev));
 
+	/* Get the quirks for all matching entries */
 	while (ad->model_num) {
 		if (glob_match(ad->model_num, model_num) &&
 		    (!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)
-- 
2.55.0

Re: [PATCH] ata: libata-core: always apply all quirks matching for a device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ata: ata_dev_quirks() accumulation breaks specific quirk exclusions
- [Medium] ata: ata_dev_print_quirks() inside loop suppresses subsequent match logs
--

commit 8f4a558f46757e1ace36c8ad018b708424fefd18
Author: Damien Le Moal <dlemoal@kernel.org>

ata: libata-core: always apply all quirks matching for a device

This commit changes ata_dev_quirks() to accumulate all matching quirks from
the __ata_dev_quirks array rather than returning on the first match. This
is intended to address issues where appended ATA_QUIRK_NO_FUA entries were
ignored if earlier patterns also matched the drive.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c

[ ... ]

> @@ -4521,16 +4522,18 @@ static u64 ata_dev_quirks(const struct ata_device *dev)
>  	ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
>  	ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev));
>  
> +	/* Get the quirks for all matching entries */
>  	while (ad->model_num) {
>  		if (glob_match(ad->model_num, model_num) &&
>  		    (!ad->model_rev || glob_match(ad->model_rev, model_rev))) {
>  			ata_dev_print_quirks(dev, model_num, model_rev,
>  					     ad->quirks);

[Severity: Medium]
Since ata_dev_print_quirks() sets ATA_EHI_DID_PRINT_QUIRKS on its first run
and suppresses subsequent printing attempts, won't it fail to log the quirks
from any second or subsequent matches?

Looking at ata_dev_print_quirks():

    if (!ata_dev_print_info(dev) || ehc->i.flags & ATA_EHI_DID_PRINT_QUIRKS)
        return;

    ehc->i.flags |= ATA_EHI_DID_PRINT_QUIRKS;

Should the print function be called exactly once, outside the loop, passing
the fully accumulated quirks mask instead of ad->quirks?

> -			return ad->quirks;
> +			quirks |= ad->quirks;

[Severity: High]
Does accumulating the quirks here break the ability for specific entries to
shadow and exclude broader entries?

For example, earlier in the __ata_dev_quirks table, a specific entry is used
to explicitly exclude a buggy drive:

    /*
     * The intel 510 drive has buggy DRAT/RZAT. Explicitly exclude
     * that model before whitelisting all other intel SSDs.
     */
    { "INTEL*SSDSC2MH*",        NULL,   0 },

If the loop continues, it will match the broader "INTEL*SSD*" pattern and
apply the exact quirks the earlier specific entry was meant to avoid. Can
this lead to functional regressions for devices that relied on earlier 
entries to bypass buggy hardware features?

>  		}
>  		ad++;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014150340.2410073-1-dlemoal@kernel.org?part=1

Re: [PATCH] ata: libata-core: always apply all quirks matching for a device

[Damien Le Moal]

On 2026/10/14 17:11, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] ata: ata_dev_quirks() accumulation breaks specific quirk exclusions
> - [Medium] ata: ata_dev_print_quirks() inside loop suppresses subsequent match logs
> --
> 
> commit 8f4a558f46757e1ace36c8ad018b708424fefd18
> Author: Damien Le Moal <dlemoal@kernel.org>
> 
> ata: libata-core: always apply all quirks matching for a device
> 
> This commit changes ata_dev_quirks() to accumulate all matching quirks from
> the __ata_dev_quirks array rather than returning on the first match. This
> is intended to address issues where appended ATA_QUIRK_NO_FUA entries were
> ignored if earlier patterns also matched the drive.
> 
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
> 
> [ ... ]
> 
>> @@ -4521,16 +4522,18 @@ static u64 ata_dev_quirks(const struct ata_device *dev)
>>  	ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
>>  	ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev));
>>  
>> +	/* Get the quirks for all matching entries */
>>  	while (ad->model_num) {
>>  		if (glob_match(ad->model_num, model_num) &&
>>  		    (!ad->model_rev || glob_match(ad->model_rev, model_rev))) {
>>  			ata_dev_print_quirks(dev, model_num, model_rev,
>>  					     ad->quirks);
> 
> [Severity: Medium]
> Since ata_dev_print_quirks() sets ATA_EHI_DID_PRINT_QUIRKS on its first run
> and suppresses subsequent printing attempts, won't it fail to log the quirks
> from any second or subsequent matches?
> 
> Looking at ata_dev_print_quirks():
> 
>     if (!ata_dev_print_info(dev) || ehc->i.flags & ATA_EHI_DID_PRINT_QUIRKS)
>         return;
> 
>     ehc->i.flags |= ATA_EHI_DID_PRINT_QUIRKS;
> 
> Should the print function be called exactly once, outside the loop, passing
> the fully accumulated quirks mask instead of ad->quirks?
> 
>> -			return ad->quirks;
>> +			quirks |= ad->quirks;
> 
> [Severity: High]
> Does accumulating the quirks here break the ability for specific entries to
> shadow and exclude broader entries?
> 
> For example, earlier in the __ata_dev_quirks table, a specific entry is used
> to explicitly exclude a buggy drive:
> 
>     /*
>      * The intel 510 drive has buggy DRAT/RZAT. Explicitly exclude
>      * that model before whitelisting all other intel SSDs.
>      */
>     { "INTEL*SSDSC2MH*",        NULL,   0 },
> 
> If the loop continues, it will match the broader "INTEL*SSD*" pattern and
> apply the exact quirks the earlier specific entry was meant to avoid. Can
> this lead to functional regressions for devices that relied on earlier 
> entries to bypass buggy hardware features?


Hmmm... OK. I think Sashiko has a good point here. We cannot use my idea as that
works only for accumulating quirks, not for restricting them.
So I guess your patch is the better one.


-- 
Damien Le Moal
Western Digital Research