diff mbox series

ata: ahci: stop using saved_port_map for quircks

Message ID bc76towgdmpv3xilmhlerrshpubrd7feecl2tu6xanwtbkv3ze@zwizzrmcu43w
State New
Headers show
Series ata: ahci: stop using saved_port_map for quircks | expand

Commit Message

Andrey Jr. Melnikov Feb. 25, 2024, 9:55 a.m. UTC 
Stop using saved_port_map for masking port quirks, use force_port_map
instead.

Signed-off-by: Andrey Jr. Melnikov <temnota.am@gmail.com>

Comments

Sergey Shtylyov Feb. 26, 2024, 8:42 a.m. UTC | #1 
Hello!

   s/quircks/quirks/ in the subject... :-)

On 2/25/24 12:55 PM, 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>

[...]

MBR, Sergey
Niklas Cassel Feb. 26, 2024, 9:57 a.m. UTC | #2 
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
Serge Semin Feb. 26, 2024, 10:30 a.m. UTC | #3 
Hi Niklas

On Mon, Feb 26, 2024 at 10:57:08AM +0100, Niklas Cassel wrote:
> 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?

Thanks for Cc-ing me. Sorry, I am a bit busy right now. I'll have a
closer look on this patch later on this week.

-Serge(y)

> 
> 
> Kind regards,
> Niklas
Serge Semin March 1, 2024, 9:32 a.m. UTC | #4 
Hi Niklas, Andrey

On Mon, Feb 26, 2024 at 10:57:08AM +0100, Niklas Cassel wrote:
> 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?

Absolutely right. The change breaks the saved_port_map semantics. The
main purpose of all the ahci_host_priv::saved_* fields is to
save-restore the controller capabilities across device resets
(suspend/resume). About two years ago the semantics of
ahci_host_priv::{saved_cap,saved_port_map,saved_port_cap[*]} was
extended so the fields could be pre-initialized with data by the
low-level drivers thus overriding malfunction or uninitialized
controller ports mapping and capabilities. Andrey's patch breaks the
last part by force-writing to the ahci_host_priv::saved_port_map not
taking into account that it could have been pre-initialized. Moreover
ahci_host_priv:mask_port_map semantics is different from what is
implied by ahci_host_priv::saved_port_map:
1. saved_port_map defines the ports actually available on the host
controller.
2. mask_port_map masks out the unused ports if it's initialized,
otherwise all available ports will be initialized and utilized.

So the later field can't be that easily replaced with the former one.
Thus if this patch is applied:

1. The "ports-implemented" DT-property will no longer work since it's
primary goal is to override the data in normal circumstance retrieved
from the PI register.

2. The ahci_pci_save_initial_config() will be broken for Asmedia and
JMicron, since the number of available ports will be retrieved from
the PI register while the original semantics implied forcing it to a
particular value.

-Serge(y)

> 
> 
> Kind regards,
> Niklas
diff mbox series

Patch

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,