[PATCH 0/2] Add external_port_map module parameter

[PATCH 0/2] Add external_port_map module parameter

[Niklas Cassel]

Hello there,

This adds a module parameter 'external_port_map' to the 'ahci' driver.

Normally we do not enable LPM on ports that are marked as external by
firmware. However, as always, firmware can have bugs and can have forgotten
to mark a port as external.

This module parameter gives those users a way to still have their ports
hotplug capable, without modifying the 'mobile_lpm_policy', which will
affect the LPM policy for all ports.

This module parameter allows you to just mark a single port as external,
such that you will still get power saving on the ports that are not marked
as external.


One example where this module parameter can also be used, is for buggy
devices such as the "HL-DT-ST BD-RE BU40N" Blu-Ray player, which does not
reply to a regular COMRESET (the controller sees nothing as connected),
instead it will send a hotplug event when, and only when the user presses
the tray open button.

Not replying to a COMRESET is not spec compliant. If a port does not
detect any device on a port, and LPM is enabled on that port, and the
port is not marked as hotplug capable, then there should be no way that
a device can be hotplugged later, so libata powers off the port/PHY to
save power.

This module parameter will give users a way to handles such non spec
compliant devices in a more fine grained way (rather than using the big
'mobile_lpm_policy' hammer).

There does also exist a per port link_power_management_policy sysfs
attribute, however, for many people, a kernel module is more convenient
compared to writing udev rules.


Kind regards,
Niklas


Niklas Cassel (2):
  ata: ahci: Create a ahci_get_port_map_helper() helper
  ata: ahci: Add external_port_map module parameter

 drivers/ata/ahci.c | 89 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 72 insertions(+), 17 deletions(-)

-- 
2.48.0

[PATCH 1/2] ata: ahci: Create a ahci_get_port_map_helper() helper

[Niklas Cassel]

Create a ahci_get_port_map_helper() helper so that this code can be reused
by other module parameters that are saved in a port bitmap.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 48 +++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 8d27c567be1c..92b08d3a0c3c 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -676,35 +676,27 @@ MODULE_PARM_DESC(mask_port_map,
 		 "where <pci_dev> is the PCI ID of an AHCI controller in the "
 		 "form \"domain:bus:dev.func\"");
 
-static void ahci_apply_port_map_mask(struct device *dev,
-				     struct ahci_host_priv *hpriv, char *mask_s)
-{
-	unsigned int mask;
-
-	if (kstrtouint(mask_s, 0, &mask)) {
-		dev_err(dev, "Invalid port map mask\n");
-		return;
-	}
-
-	hpriv->mask_port_map = mask;
-}
+typedef void (*port_map_callback_t)(struct device *dev,
+				    struct ahci_host_priv *hpriv, char *mask_s);
 
-static void ahci_get_port_map_mask(struct device *dev,
-				   struct ahci_host_priv *hpriv)
+static void ahci_get_port_map_helper(struct device *dev,
+				     struct ahci_host_priv *hpriv,
+				     const char *module_str,
+				     port_map_callback_t apply_cb)
 {
 	char *param, *end, *str, *mask_s;
 	char *name;
 
-	if (!strlen(ahci_mask_port_map))
+	if (!strlen(module_str))
 		return;
 
-	str = kstrdup(ahci_mask_port_map, GFP_KERNEL);
+	str = kstrdup(module_str, GFP_KERNEL);
 	if (!str)
 		return;
 
 	/* Handle single mask case */
 	if (!strchr(str, '=')) {
-		ahci_apply_port_map_mask(dev, hpriv, str);
+		apply_cb(dev, hpriv, str);
 		goto free;
 	}
 
@@ -739,13 +731,33 @@ static void ahci_get_port_map_mask(struct device *dev,
 			param++;
 		}
 
-		ahci_apply_port_map_mask(dev, hpriv, mask_s);
+		apply_cb(dev, hpriv, mask_s);
 	}
 
 free:
 	kfree(str);
 }
 
+static void ahci_apply_port_map_mask(struct device *dev,
+				     struct ahci_host_priv *hpriv, char *mask_s)
+{
+	unsigned int mask;
+
+	if (kstrtouint(mask_s, 0, &mask)) {
+		dev_err(dev, "Invalid port map mask\n");
+		return;
+	}
+
+	hpriv->mask_port_map = mask;
+}
+
+static void ahci_get_port_map_mask(struct device *dev,
+				   struct ahci_host_priv *hpriv)
+{
+	ahci_get_port_map_helper(dev, hpriv, ahci_mask_port_map,
+				 ahci_apply_port_map_mask);
+}
+
 static void ahci_pci_save_initial_config(struct pci_dev *pdev,
 					 struct ahci_host_priv *hpriv)
 {
-- 
2.48.0

[PATCH 2/2] ata: ahci: Add external_port_map module parameter

[Niklas Cassel]

Commit ae1f3db006b7 ("ata: ahci: do not enable LPM on external ports")
changed so that LPM is not enabled on external ports (hotplug-capable or
eSATA ports).

This is because hotplug and LPM are mutually exclusive, see 7.3.1 Hot Plug
Removal Detection and Power Management Interaction in AHCI 1.3.1.

This does require that firmware has set the appropate bits (HPCP or ESP)
in PxCMD (which is a per port register in the AHCI controller).

If the firmware has failed to mark a port as hotplug-capable or eSATA in
PxCMD, then there is currently not much the user can do.

If LPM is enabled on the port, hotplug insertions and removals will not be
detected on that port.

In order to allow a user to fix up broken firmware, add a module parameter
'external_port_map' for the 'ahci' driver.

The external_port_map parameter accepts 2 different formats:
 - external_port_map=<map>
   This applies the same map to all AHCI controllers
   present in the system. This format is convenient for small systems
   that have only a single AHCI controller.
 - external_port_map=<pci_dev>=<map>,<pci_dev>=map,...
   This applies the specified maps only to the PCI device listed. The
   <pci_dev> field is a regular PCI device ID (domain:bus:dev.func).
   This ID can be seen following "ahci" in the kernel messages. E.g.
   for "ahci 0000:01:00.0: 2/2 ports implemented (port mask 0x3)", the
   <pci_dev> field is "0000:01:00.0".

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 92b08d3a0c3c..ec2bc5f17b96 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -676,6 +676,17 @@ MODULE_PARM_DESC(mask_port_map,
 		 "where <pci_dev> is the PCI ID of an AHCI controller in the "
 		 "form \"domain:bus:dev.func\"");
 
+static char *ahci_external_port_map;
+module_param_named(external_port_map, ahci_external_port_map, charp, 0444);
+MODULE_PARM_DESC(external_port_map,
+		 "32-bits port map to force set one or more ports as external. "
+		 "Valid values are: "
+		 "\"<map>\" to apply the same map to all AHCI controller "
+		 "devices, and \"<pci_dev>=<map>,<pci_dev>=<map>,...\" to "
+		 "specify different maps for the controllers specified, "
+		 "where <pci_dev> is the PCI ID of an AHCI controller in the "
+		 "form \"domain:bus:dev.func\"");
+
 typedef void (*port_map_callback_t)(struct device *dev,
 				    struct ahci_host_priv *hpriv, char *mask_s);
 
@@ -758,6 +769,34 @@ static void ahci_get_port_map_mask(struct device *dev,
 				 ahci_apply_port_map_mask);
 }
 
+static void ahci_apply_external_port_map(struct device *dev,
+					 struct ahci_host_priv *hpriv,
+					 char *mask_s)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	unsigned long port_map;
+	unsigned int map;
+	int i;
+
+	if (kstrtouint(mask_s, 0, &map)) {
+		dev_err(dev, "Invalid external port map\n");
+		return;
+	}
+
+	port_map = map;
+	for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
+		if (i < host->n_ports)
+			host->ports[i]->pflags |= ATA_PFLAG_EXTERNAL;
+	}
+}
+
+static void ahci_get_external_port_map(struct device *dev,
+				       struct ahci_host_priv *hpriv)
+{
+	ahci_get_port_map_helper(dev, hpriv, ahci_external_port_map,
+				 ahci_apply_external_port_map);
+}
+
 static void ahci_pci_save_initial_config(struct pci_dev *pdev,
 					 struct ahci_host_priv *hpriv)
 {
@@ -2020,6 +2059,10 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (pi.flags & ATA_FLAG_EM)
 		ahci_reset_em(host);
 
+	/* Handle external port map passed as module parameter. */
+	if (ahci_external_port_map)
+		ahci_get_external_port_map(&pdev->dev, hpriv);
+
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-- 
2.48.0

Re: [PATCH 2/2] ata: ahci: Add external_port_map module parameter

[Damien Le Moal]

On 1/16/25 23:36, Niklas Cassel wrote:
> Commit ae1f3db006b7 ("ata: ahci: do not enable LPM on external ports")
> changed so that LPM is not enabled on external ports (hotplug-capable or
> eSATA ports).
> 
> This is because hotplug and LPM are mutually exclusive, see 7.3.1 Hot Plug
> Removal Detection and Power Management Interaction in AHCI 1.3.1.
> 
> This does require that firmware has set the appropate bits (HPCP or ESP)
> in PxCMD (which is a per port register in the AHCI controller).
> 
> If the firmware has failed to mark a port as hotplug-capable or eSATA in
> PxCMD, then there is currently not much the user can do.
> 
> If LPM is enabled on the port, hotplug insertions and removals will not be
> detected on that port.
> 
> In order to allow a user to fix up broken firmware, add a module parameter
> 'external_port_map' for the 'ahci' driver.

Maybe we should call the parameter "force_external_port_map" to make it clear
that this is a user forced setup ?

Otherwise looks good to me.

> 
> The external_port_map parameter accepts 2 different formats:
>  - external_port_map=<map>
>    This applies the same map to all AHCI controllers
>    present in the system. This format is convenient for small systems
>    that have only a single AHCI controller.
>  - external_port_map=<pci_dev>=<map>,<pci_dev>=map,...
>    This applies the specified maps only to the PCI device listed. The
>    <pci_dev> field is a regular PCI device ID (domain:bus:dev.func).
>    This ID can be seen following "ahci" in the kernel messages. E.g.
>    for "ahci 0000:01:00.0: 2/2 ports implemented (port mask 0x3)", the
>    <pci_dev> field is "0000:01:00.0".
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 92b08d3a0c3c..ec2bc5f17b96 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -676,6 +676,17 @@ MODULE_PARM_DESC(mask_port_map,
>  		 "where <pci_dev> is the PCI ID of an AHCI controller in the "
>  		 "form \"domain:bus:dev.func\"");
>  
> +static char *ahci_external_port_map;
> +module_param_named(external_port_map, ahci_external_port_map, charp, 0444);
> +MODULE_PARM_DESC(external_port_map,
> +		 "32-bits port map to force set one or more ports as external. "
> +		 "Valid values are: "
> +		 "\"<map>\" to apply the same map to all AHCI controller "
> +		 "devices, and \"<pci_dev>=<map>,<pci_dev>=<map>,...\" to "
> +		 "specify different maps for the controllers specified, "
> +		 "where <pci_dev> is the PCI ID of an AHCI controller in the "
> +		 "form \"domain:bus:dev.func\"");
> +
>  typedef void (*port_map_callback_t)(struct device *dev,
>  				    struct ahci_host_priv *hpriv, char *mask_s);
>  
> @@ -758,6 +769,34 @@ static void ahci_get_port_map_mask(struct device *dev,
>  				 ahci_apply_port_map_mask);
>  }
>  
> +static void ahci_apply_external_port_map(struct device *dev,
> +					 struct ahci_host_priv *hpriv,
> +					 char *mask_s)
> +{
> +	struct ata_host *host = dev_get_drvdata(dev);
> +	unsigned long port_map;
> +	unsigned int map;
> +	int i;
> +
> +	if (kstrtouint(mask_s, 0, &map)) {
> +		dev_err(dev, "Invalid external port map\n");
> +		return;
> +	}
> +
> +	port_map = map;
> +	for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
> +		if (i < host->n_ports)
> +			host->ports[i]->pflags |= ATA_PFLAG_EXTERNAL;
> +	}
> +}
> +
> +static void ahci_get_external_port_map(struct device *dev,
> +				       struct ahci_host_priv *hpriv)
> +{
> +	ahci_get_port_map_helper(dev, hpriv, ahci_external_port_map,
> +				 ahci_apply_external_port_map);
> +}
> +
>  static void ahci_pci_save_initial_config(struct pci_dev *pdev,
>  					 struct ahci_host_priv *hpriv)
>  {
> @@ -2020,6 +2059,10 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (pi.flags & ATA_FLAG_EM)
>  		ahci_reset_em(host);
>  
> +	/* Handle external port map passed as module parameter. */
> +	if (ahci_external_port_map)
> +		ahci_get_external_port_map(&pdev->dev, hpriv);
> +
>  	for (i = 0; i < host->n_ports; i++) {
>  		struct ata_port *ap = host->ports[i];
>  


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] ata: ahci: Create a ahci_get_port_map_helper() helper

[Damien Le Moal]

On 1/16/25 23:36, Niklas Cassel wrote:
> Create a ahci_get_port_map_helper() helper so that this code can be reused
> by other module parameters that are saved in a port bitmap.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.c | 48 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 8d27c567be1c..92b08d3a0c3c 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -676,35 +676,27 @@ MODULE_PARM_DESC(mask_port_map,
>  		 "where <pci_dev> is the PCI ID of an AHCI controller in the "
>  		 "form \"domain:bus:dev.func\"");
>  
> -static void ahci_apply_port_map_mask(struct device *dev,
> -				     struct ahci_host_priv *hpriv, char *mask_s)
> -{
> -	unsigned int mask;
> -
> -	if (kstrtouint(mask_s, 0, &mask)) {
> -		dev_err(dev, "Invalid port map mask\n");
> -		return;
> -	}
> -
> -	hpriv->mask_port_map = mask;
> -}
> +typedef void (*port_map_callback_t)(struct device *dev,
> +				    struct ahci_host_priv *hpriv, char *mask_s);
>  
> -static void ahci_get_port_map_mask(struct device *dev,
> -				   struct ahci_host_priv *hpriv)
> +static void ahci_get_port_map_helper(struct device *dev,

Can we drop the "_helper" at the end of the name ?

> +				     struct ahci_host_priv *hpriv,
> +				     const char *module_str,
> +				     port_map_callback_t apply_cb)

And maybe change this to "apply_port_map_mask_cb" to be clear about what is
being applied ?


-- 
Damien Le Moal
Western Digital Research