Re: [PATCH v1] ATA: ahci: Add Intel PCH-S RAID device ID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ata: ahci: Nova Lake PCH-S RAID entry uses legacy board_ahci_pcs_quirk
--

commit 2589d3b85f965fdaa155298cf5da322c956d8de2
Author: Roman Gardzinski <romanx.gardzinski@linux.intel.com>

ATA: ahci: Add Intel PCH-S RAID device ID

This commit adds the Intel Nova Lake PCH-S RAID PCI device ID (0x6e66) to
ahci_pci_tbl in drivers/ata/ahci.c.

> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 3c38ee3314ee9..f4b6235452dbd 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -274,6 +274,10 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  		/* Comet Lake PCH-H RAID */
>  		PCI_VDEVICE(INTEL, 0x06d6),
>  		.driver_data = board_ahci_pcs_quirk,
> +	}, {
> +		/* Nova Lake PCH-S RAID */
> +		PCI_VDEVICE(INTEL, 0x6e66),
> +		.driver_data = board_ahci_pcs_quirk,

[Severity: High]
Should this new Nova Lake PCH-S RAID entry in ahci_pci_tbl use board_ahci
instead of board_ahci_pcs_quirk?

In drivers/ata/ahci.c, enum board_ids notes that board_ahci_pcs_quirk is
only for legacy Intel platforms:

	/*
	 * board_ahci_pcs_quirk is for legacy Intel platforms.
	 * Modern Intel platforms should use board_ahci instead.
	 * (Some modern Intel platforms might have been added with
	 * board_ahci_pcs_quirk, however, we cannot change them to board_ahci
	 * without testing that the platform actually works without the quirk.)
	 */
	board_ahci_pcs_quirk,

Using board_ahci_pcs_quirk sets AHCI_HFLAG_INTEL_PCS_QUIRK in
ahci_port_info[]. When the controller is probed, resumed, or reset via:

  ahci_init_one() / ahci_pci_device_runtime_resume() / ahci_pci_device_resume()
    -> ahci_pci_reset_controller()
      -> ahci_intel_pcs_quirk()

ahci_intel_pcs_quirk() reads and writes hpriv->port_map into the legacy
6-port PCS_6 PCI configuration register (offset 0x92):

	pci_read_config_word(pdev, PCS_6, &tmp16);
	if ((tmp16 & hpriv->port_map) != hpriv->port_map) {
		tmp16 |= hpriv->port_map;
		pci_write_config_word(pdev, PCS_6, tmp16);
	}

On modern Intel platforms (and PCH-S controllers where the legacy 6-port
PCS_6 layout does not apply), applying this quirk has previously caused
regressions that broke SATA drive detection, as seen in commit 7edbb6059274
and commit 9e2f46cd8747. When the same Nova Lake controller is in AHCI mode,
it matches PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_SATA_AHCI, 0xffffff) and uses
board_ahci.

Also, the commit message states "No functional changes", even though adding a
new PCI device ID enables driver binding for this hardware. Could the entry
be updated to use board_ahci and the commit message adjusted accordingly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027141240.7850-1-romanx.gardzinski@linux.intel.com?part=1