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

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

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