Re: [PATCH 2/2] ahci: clean up ahci_broken_devslp quirk

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

On 2/13/24 22:07, Niklas Cassel wrote:
> Most quirks are applied using a specific board type board_ahci_no*
> (e.g. board_ahci_nomsi, board_ahci_noncq), which then sets a flag
> representing the specific quirk.
> 
> ahci_pci_tbl (which is the table of all supported PCI devices), then
> uses that board type for the PCI vendor and device IDs which need to
> be quirked.
> 
> The ahci_broken_devslp quirk is not implemented in this standard way.
> 
> Modify the ahci_broken_devslp quirk to be implemented like the other
> quirks. This way, we will not have the same PCI device and vendor ID
> scattered over ahci.c. It will simply be defined in a single location.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index c28ad3f4b59e..1e1533d01803 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -51,6 +51,7 @@ enum board_ids {
>  	board_ahci_43bit_dma,
>  	board_ahci_ign_iferr,
>  	board_ahci_no_debounce_delay,
> +	board_ahci_no_devslp_pcs_quirk,
>  	board_ahci_nomsi,
>  	board_ahci_noncq,
>  	board_ahci_nosntf_pcs_quirk,
> @@ -151,6 +152,14 @@ static const struct ata_port_info ahci_port_info[] = {
>  		.udma_mask	= ATA_UDMA6,
>  		.port_ops	= &ahci_ops,
>  	},
> +	[board_ahci_no_devslp_pcs_quirk] = {
> +		AHCI_HFLAGS	(AHCI_HFLAG_NO_DEVSLP |
> +				 AHCI_HFLAG_INTEL_PCS_QUIRK),
> +		.flags		= AHCI_FLAG_COMMON,
> +		.pio_mask	= ATA_PIO4,
> +		.udma_mask	= ATA_UDMA6,
> +		.port_ops	= &ahci_ops,
> +	},
>  	[board_ahci_nomsi] = {
>  		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
>  		.flags		= AHCI_FLAG_COMMON,
> @@ -420,7 +429,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  	{ PCI_VDEVICE(INTEL, 0x06d7), board_ahci_pcs_quirk }, /* Comet Lake-H RAID */
>  	{ PCI_VDEVICE(INTEL, 0xa386), board_ahci_pcs_quirk }, /* Comet Lake PCH-V RAID */
>  	{ PCI_VDEVICE(INTEL, 0x0f22), board_ahci_pcs_quirk }, /* Bay Trail AHCI */
> -	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_pcs_quirk }, /* Bay Trail AHCI */
> +	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_no_devslp_pcs_quirk }, /* Bay Trail AHCI */

Same nit as the previous patch: let's name this board_ahci_pcs_quirk_no_devslp.

With that, looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

>  	{ PCI_VDEVICE(INTEL, 0x22a3), board_ahci_pcs_quirk }, /* Cherry Tr. AHCI */
>  	{ PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_pcs_quirk }, /* ApolloLake AHCI */
>  	{ PCI_VDEVICE(INTEL, 0x34d3), board_ahci_pcs_quirk }, /* Ice Lake LP AHCI */
> @@ -1420,17 +1429,6 @@ static bool ahci_broken_online(struct pci_dev *pdev)
>  	return pdev->bus->number == (val >> 8) && pdev->devfn == (val & 0xff);
>  }
>  
> -static bool ahci_broken_devslp(struct pci_dev *pdev)
> -{
> -	/* device with broken DEVSLP but still showing SDS capability */
> -	static const struct pci_device_id ids[] = {
> -		{ PCI_VDEVICE(INTEL, 0x0f23)}, /* Valleyview SoC */
> -		{}
> -	};
> -
> -	return pci_match_id(ids, pdev);
> -}
> -
>  #ifdef CONFIG_ATA_ACPI
>  static void ahci_gtf_filter_workaround(struct ata_host *host)
>  {
> @@ -1823,10 +1821,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  				&dev_attr_remapped_nvme.attr,
>  				NULL);
>  
> -	/* must set flag prior to save config in order to take effect */
> -	if (ahci_broken_devslp(pdev))
> -		hpriv->flags |= AHCI_HFLAG_NO_DEVSLP;
> -
>  #ifdef CONFIG_ARM64
>  	if (pdev->vendor == PCI_VENDOR_ID_HUAWEI &&
>  	    pdev->device == 0xa235 &&

-- 
Damien Le Moal
Western Digital Research