Re: [PATCH v6 04/11] ata: libata: Print quirks applied to devices

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

On 7/30/24 19:09, Geert Uytterhoeven wrote:
>  	Hi Damien,
> 
> On Fri, 26 Jul 2024, Damien Le Moal wrote:
>> Introduce the function ata_dev_print_quirks() to print the quirk flags
>> that will be applied to a scanned device. This new function is called
>> from ata_dev_quirks() when a match on a device model or device model
>> and revision is found for a device in the __ata_dev_quirks array.
>>
>> To implement this function, the ATA_QUIRK_ flags are redefined using
>> the new enum ata_quirk which defines the bit shift for each quirk
>> flag. The array of strings ata_quirk_names is used to define the name
>> of each flag, which are printed by ata_dev_print_quirks().
>>
>> Example output for a device listed in the __ata_dev_quirks array and
>> which has the ATA_QUIRK_DISABLE flag applied:
>>
>> [10193.461270] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>> [10193.469190] ata1.00: Model 'ASMT109x- Config', rev '2143 5', applying quirks: disable
>> [10193.469195] ata1.00: unsupported device, disabling
>> [10193.481564] ata1.00: disable device
>>
>> enum ata_quirk also defines the __ATA_QUIRK_MAX value as one plus the
>> last quirk flag defined. This value is used in ata_dev_quirks() to add a
>> build time check that all quirk flags fit within the unsigned int
>> (32-bits) quirks field of struct ata_device.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> 
> Thanks for your patch, which is now commit 58157d607aecb4e0 ("ata:
> libata: Print quirks applied to devices") in libata/for-next.
> 
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -4273,15 +4336,18 @@ static unsigned int ata_dev_quirks(const struct ata_device *dev)
>> 	unsigned char model_rev[ATA_ID_FW_REV_LEN + 1];
>> 	const struct ata_dev_quirks_entry *ad = __ata_dev_quirks;
>>
>> +	/* dev->quirks is an unsigned int. */
>> +	BUILD_BUG_ON(__ATA_QUIRK_MAX > 32);
>> +
>> 	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));
>>
>> 	while (ad->model_num) {
>> -		if (glob_match(ad->model_num, model_num)) {
>> -			if (ad->model_rev == NULL)
>> -				return ad->quirks;
>> -			if (glob_match(ad->model_rev, model_rev))
>> -				return ad->quirks;
>> +		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;
>> 		}
>> 		ad++;
>> 	}
> 
> During boot-up on Salvator-XS (using rcar-sata), the quirk info is
> printed not once, but four times.  Is that intentional?

Not at all. I tested on x86 with AHCI and see this message only once. So it
could be that different drivers may need some tweaks to avoid this spamming.
Though it is strange that the initialization or resume path takes this path 4
times, meaning that the quirks are applied 4 times. Need to look into that.
What is the driver for rcar-sata ? Compatible string for it would be fine.

> 
>      ata1: link resume succeeded after 1 retries
>     +rcar-du feb00000.display: [drm] fb0: rcar-dudrmfb frame buffer device
>      input: keys as /devices/platform/keys/input/input0
>      ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>     +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq
>      ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133
>      ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used)
>     +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq
>     +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq
>     +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq
>      ata1.00: configured for UDMA/133
>      scsi 0:0:0:0: Direct-Access     ATA      Maxtor 6L160M0   1G10 PQ: 0 ANSI: 5
>      sd 0:0:0:0: [sda] 320173056 512-byte logical blocks: (164 GB/153 GiB)
>      sd 0:0:0:0: [sda] Write Protect is off
>      sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
>      sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
>      sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes
>       sda: sda1
>      sd 0:0:0:0: [sda] Attached SCSI disk
> 
> During resume from s2idle or s2ram, the same info is printed again,
> fourfold:
> 
>      ata1: link resume succeeded after 1 retries
>      ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>     +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq
>     +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq
>     +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq
>     +ata1.00: Model 'Maxtor 6L160M0', rev 'BANC1G10', applying quirks: noncq
>      ata1.00: configured for UDMA/133
>      ata1.00: Entering active power mode
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>  						Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>  							    -- Linus Torvalds

-- 
Damien Le Moal
Western Digital Research