Re: [PATCH] ata: libata-core: Add NOLPM quirk for PNY 1TB SATA SSD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ata: libata-core: overly broad 1.5Gbps and NONCQ quirks for PNY 1TB SATA SSD
--

commit 0d9a8ef67d5864f02d54ceedd7d99487c051f554
Author: Thierry Vignaud <tvignaud@redhat.com>

ata: libata-core: Add NOLPM quirk for PNY 1TB SATA SSD

This commit adds an entry to __ata_dev_quirks[] for the "PNY 1TB SATA SSD"
drive with ATA_QUIRK_NOLPM, ATA_QUIRK_1_5_GBPS, and ATA_QUIRK_NONCQ across
all firmware revisions.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index dadcaf9ff4622..cf36f2638fab3 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4398,6 +4398,11 @@ static const struct ata_dev_quirks_entry __ata_dev_quirks[] = {
>  	/* PNY CS900 (Phison PS3111-S11, DRAM-less) drops the link on DIPM */
>  	{ "PNY CS900 1TB SSD",		NULL,	ATA_QUIRK_NOLPM },
>  
> +	/* PNY CS900 (variant) drops the link on DIPM and NCQ */
> +	{ "PNY 1TB SATA SSD",		NULL,	ATA_QUIRK_NOLPM |
> +						ATA_QUIRK_1_5_GBPS |
> +						ATA_QUIRK_NONCQ },

[Severity: High]
Could applying ATA_QUIRK_1_5_GBPS and ATA_QUIRK_NONCQ across all firmware
revisions (NULL) of "PNY 1TB SATA SSD" cause a severe performance regression
for other users of this drive?

During ata_dev_configure() -> ata_dev_quirks(), every "PNY 1TB SATA SSD" will
match this entry regardless of firmware revision. In ata_do_link_spd_quirk(),
ATA_QUIRK_1_5_GBPS clamps the physical SATA link speed limit to 1.5 Gbps:

drivers/ata/libata-core.c:ata_do_link_spd_quirk() {
	if (dev->quirks & ATA_QUIRK_1_5_GBPS)
		target = 1;
	else
		return 0;

	target_limit = (1 << target) - 1;

	/* if already on stricter limit, no need to push further */
	if (plink->sata_spd_limit <= target_limit)
		return 0;

	plink->sata_spd_limit = target_limit;
}

Unlike ATA_QUIRK_NOLPM and ATA_QUIRK_NONCQ, force_tbl[] has no on/off entry
for ATA_QUIRK_1_5_GBPS, so users with working drives cannot override this
1.5 Gbps cap via libata.force. Meanwhile, ata_dev_config_ncq() also disables
NCQ completely when ATA_QUIRK_NONCQ is set.

Additionally, the commit title and opening description state:

  For a year, I workaround it by disabling NCQ & reducing the speed by
  booting with "libata.force=0:3.0Gbps,0:noncq".
  It turns out disabling LPM fixes it without the huge performance hit.

while the embedded "v2:" changelog notes and the code itself instead drop the
speed to 1.5 Gbps, disable NCQ, and remove the firmware revision filter, and
the code comment above the entry does not mention the 1.5 Gbps limit.

Note also that in ata_port_alloc(), port IDs (ap->print_id) are allocated
starting at 1:

drivers/ata/libata-core.c:ata_port_alloc() {
	id = ida_alloc_min(&ata_ida, 1, GFP_KERNEL);
	if (id < 0) {
		kfree(ap);
		return NULL;
	}
	ap->print_id = id;
}

Because port numbering starts at 1, "libata.force=0:3.0Gbps,0:noncq" never
matched any port in ata_force_link_limits() or ata_force_quirks(), which matches
the commit message logs on sda still showing queued commands (tag#2 through
tag#9).

If a specific drive requires 1.5 Gbps, no NCQ, and no LPM simultaneously to
avoid link timeouts, could that indicate a local cable, controller, or hardware
unit issue rather than a quirk that should be applied unconditionally to all
"PNY 1TB SATA SSD" drives and firmware revisions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028124328.597603-1-tvignaud@redhat.com?part=1