Re: [PATCH] ata: ahci: stop using saved_port_map for quircks

[Niklas Cassel <cassel@kernel.org>]

Hello Andrey,

On Sun, Feb 25, 2024 at 12:55:42PM +0300, Andrey Jr. Melnikov wrote:
> 
> Stop using saved_port_map for masking port quirks, use force_port_map
> instead.
> 
> Signed-off-by: Andrey Jr. Melnikov <temnota.am@gmail.com>
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 682ff550ccfb..066e3118801c 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -675,18 +675,18 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev,
>  		switch (pdev->device) {
>  		case 0x1166:
>  			dev_info(&pdev->dev, "ASM1166 has only six ports\n");
> -			hpriv->saved_port_map = 0x3f;
> +			hpriv->mask_port_map = 0x3f;
>  			break;
>  		case 0x1064:
>  			dev_info(&pdev->dev, "ASM1064 has only four ports\n");
> -			hpriv->saved_port_map = 0xf;
> +			hpriv->mask_port_map = 0xf;
>  			break;
>  		}
>  	}
>  
>  	if (pdev->vendor == PCI_VENDOR_ID_JMICRON && pdev->device == 0x2361) {
>  		dev_info(&pdev->dev, "JMB361 has only one port\n");
> -		hpriv->saved_port_map = 1;
> +		hpriv->mask_port_map = 1;
>  	}
>  
>  	/*
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 1a63200ea437..cc705d3bdc50 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -531,16 +531,10 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>  		cap &= ~HOST_CAP_SXS;
>  	}
>  
> -	/* Override the HBA ports mapping if the platform needs it */
>  	port_map = readl(mmio + HOST_PORTS_IMPL);
> -	if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) {
> -		dev_info(dev, "forcing port_map 0x%lx -> 0x%x\n",
> -			 port_map, hpriv->saved_port_map);
> -		port_map = hpriv->saved_port_map;
> -	} else {
> -		hpriv->saved_port_map = port_map;
> -	}
> +	hpriv->saved_port_map = port_map;
>  
> +	/* Override the HBA ports mapping if the platform needs it */
>  	if (hpriv->mask_port_map) {
>  		dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n",
>  			port_map,
>  

Before this patch, if there was a quirk, e.g. saved_port_map was set in
ahci_pci_save_initial_config():

then in ahci_save_initial_config(),
we would not store/overwrite saved_port_map with readl(HOST_PORTS_IMPL).

After this patch, saved_port_map will contain ports that might
have been "disabled" by a quirk.


Have you verified that this logical change is okay in all the
places where saved_port_map is used?

E.g.
drivers/ata/ahci_dwc.c:ahci_dwc_check_cap() seems to iterate over:
unsigned long port_map = hpriv->saved_port_map | hpriv->mask_port_map;

which would be different before and after this patch.

Serge, any comment?



Also ahci_platform_get_firmware() seems to set
saved_port_map based of device tree property "ports-implemented".

Before this patch, saved_port map would still contain that value from
device tree, after this patch, that saved_port_map will be overwritten
with readl(HOST_PORTS_IMPL).

Again, this code is authored by Serge. Serge, comments?


Kind regards,
Niklas