Re: [PATCH] nvme-pci: add NVME_QUIRK_NO_SECONDARY_TEMP_THRESH for WD SN740

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nvme-pci: false NVME_QUIRK_NO_SECONDARY_TEMP_THRESH for WD 15b7:5017
--

commit 94ec2f511365e9d402dee1ccba8357982a5690c8
Author: Robson Cassiano <linux@robsoncassiano.software>

nvme-pci: add NVME_QUIRK_NO_SECONDARY_TEMP_THRESH for WD SN740

This commit adds NVME_QUIRK_NO_SECONDARY_TEMP_THRESH for Sandisk/WD NVMe
controllers with PCI ID 15b7:5017 so that secondary temperature threshold
sysfs attributes are hidden when the controller reports 0 and 0xffff Kelvin.

> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 77317e5d00f9f..4f1f10bc13818 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -4170,6 +4170,8 @@ static const struct pci_device_id nvme_id_table[] = {
>  	{ PCI_DEVICE(0x15b7, 0x5009),   /* Sandisk SN550 */
>  		.driver_data = NVME_QUIRK_BROKEN_MSI |
>  				NVME_QUIRK_NO_DEEPEST_PS },
> +	{ PCI_DEVICE(0x15b7, 0x5017),   /* Sandisk SN560/SN740/SN770/SN5000 */
> +		.driver_data = NVME_QUIRK_NO_SECONDARY_TEMP_THRESH, },

[Severity: Medium]
Does setting NVME_QUIRK_NO_SECONDARY_TEMP_THRESH in nvme_id_table cause a
regression by hiding valid, host-writable temp2_min, temp2_max, temp3_min,
and temp3_max attributes on Sandisk SN560, SN740, SN770, and SN5000 drives?

Per the NVM Express Base Specification (Feature Identifier 04h, Temperature
Threshold), 0000h Kelvin (-273.15 C) is the mandatory power-on default for
the Under Temperature Threshold on all implemented sensors, and FFFFh Kelvin
(65261.85 C) is the mandatory power-on default for the Over Temperature
Threshold on all non-composite temperature sensors (TMPSEL > 0, since WCTEMP
in Identify Controller only applies to the Composite Temperature). When
commit 52deba0f02a98c ("nvme: hwmon: provide temperature min and max values
for each sensor") added secondary sensor threshold support, it explicitly
documented low = -273.1 C, high = +65261.8 C as the expected unconfigured
default output.

In drivers/nvme/host/hwmon.c:nvme_hwmon_is_visible(), secondary threshold
attributes are exposed as writable (0644) when this quirk is not set:

drivers/nvme/host/hwmon.c:nvme_hwmon_is_visible() {
	...
	case hwmon_temp_max:
	case hwmon_temp_min:
		if ((!channel && data->ctrl->wctemp) ||
		    (channel && data->log->temp_sensor[channel - 1] &&
		     !(data->ctrl->quirks &
		       NVME_QUIRK_NO_SECONDARY_TEMP_THRESH))) {
			if (data->ctrl->quirks &
			    NVME_QUIRK_NO_TEMP_THRESH_CHANGE)
				return 0444;
			return 0644;
		}
		break;
	...
}

This allows userspace to configure the secondary sensor thresholds via
nvme_hwmon_write() -> nvme_set_temp_thresh().

NVME_QUIRK_NO_SECONDARY_TEMP_THRESH (introduced in commit bd375feeaf3408 and
used in commit 340f4fc5508c29) is intended for broken controllers that fail
NVME_FEAT_TEMP_THRESH commands on secondary sensors with an error (-EIO /
Invalid Field in Command). Applying this quirk to a spec-compliant
controller simply because its unconfigured power-on defaults are 0 and
0xffff Kelvin prevents userspace from reading or configuring secondary
temperature thresholds on all 15b7:5017 devices and conflicts with
Documentation/nvme/feature-and-quirk-policy.rst.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/WnzOhxjlW7w5jwpSIQmUzrvduOStpoX2h3FS@robsoncassiano.software?part=1