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

[sashiko-bot@kernel.org]

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