[PATCH v2] ata: ahci: print the lpm policy on boot

[PATCH v2] ata: ahci: print the lpm policy on boot

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

The target LPM policy can be set using either a Kconfig or a kernel module
parameter.

However, if the board type is set to anything but board_ahci_low_power,
then the LPM policy will overridden and set to ATA_LPM_UNKNOWN.

Additionally, if the default suspend is suspend to idle, depending on the
hardware capabilities of the HBA, ahci_update_initial_lpm_policy() might
override the LPM policy to either ATA_LPM_MIN_POWER_WITH_PARTIAL or
ATA_LPM_MIN_POWER.

All this means that it is very hard to know which LPM policy a user will
actually be using on a given system.

In order to make it easier to debug LPM related issues, print the LPM
policy on boot.

One common LPM related issue is that the device fails to link up.
Because of that, we cannot add this print to ata_dev_configure(), as that
function is only called after a successful link up. Instead, add the info
using ata_port_desc(), with the help of a new ata_port_desc_misc() helper.
The port description is printed once per port during boot.

Before changes:
ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 irq 170
ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 irq 170

After changes:
ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 irq 170 lpm-pol 4
ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 irq 170 lpm-pol 4

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
V1 -> V2: Moved the lpm-pol print after the irq print. Added a new helper,
ata_port_desc_misc(), to help with this.

 drivers/ata/libahci.c     |  2 +-
 drivers/ata/libata-core.c |  2 +-
 drivers/ata/libata-sff.c  | 10 +++++-----
 drivers/ata/pata_cs5520.c |  2 +-
 include/linux/libata.h    |  5 +++++
 5 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index e2bacedf28ef..5354571a3105 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2719,7 +2719,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
 
 		if (rc)
 			return rc;
-		ata_port_desc(host->ports[i], "irq %d", irq);
+		ata_port_desc_misc(host->ports[i], irq);
 	}
 
 	return ata_host_register(host, sht);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 74314311295f..715a8fb6c3e6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5923,7 +5923,7 @@ int ata_host_activate(struct ata_host *host, int irq,
 		return rc;
 
 	for (i = 0; i < host->n_ports; i++)
-		ata_port_desc(host->ports[i], "irq %d", irq);
+		ata_port_desc_misc(host->ports[i], irq);
 
 	rc = ata_host_register(host, sht);
 	/* if failed, just free the IRQ and leave ports alone */
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 8fcc622fcb3d..95a19c4ef2a1 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2316,7 +2316,7 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 		for (i = 0; i < 2; i++) {
 			if (ata_port_is_dummy(host->ports[i]))
 				continue;
-			ata_port_desc(host->ports[i], "irq %d", pdev->irq);
+			ata_port_desc_misc(host->ports[i], pdev->irq);
 		}
 	} else if (legacy_mode) {
 		if (!ata_port_is_dummy(host->ports[0])) {
@@ -2326,8 +2326,8 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 			if (rc)
 				goto out;
 
-			ata_port_desc(host->ports[0], "irq %d",
-				      ATA_PRIMARY_IRQ(pdev));
+			ata_port_desc_misc(host->ports[0],
+					   ATA_PRIMARY_IRQ(pdev));
 		}
 
 		if (!ata_port_is_dummy(host->ports[1])) {
@@ -2337,8 +2337,8 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 			if (rc)
 				goto out;
 
-			ata_port_desc(host->ports[1], "irq %d",
-				      ATA_SECONDARY_IRQ(pdev));
+			ata_port_desc_misc(host->ports[1],
+					   ATA_SECONDARY_IRQ(pdev));
 		}
 	}
 
diff --git a/drivers/ata/pata_cs5520.c b/drivers/ata/pata_cs5520.c
index 422d42761a1d..38795508c2e9 100644
--- a/drivers/ata/pata_cs5520.c
+++ b/drivers/ata/pata_cs5520.c
@@ -212,7 +212,7 @@ static int cs5520_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 		if (rc)
 			return rc;
 
-		ata_port_desc(ap, "irq %d", irq[i]);
+		ata_port_desc_misc(ap, irq[i]);
 	}
 
 	return ata_host_register(host, &cs5520_sht);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 52d58b13e5ee..f7bfc87b34ff 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1553,6 +1553,11 @@ void ata_port_desc(struct ata_port *ap, const char *fmt, ...);
 extern void ata_port_pbar_desc(struct ata_port *ap, int bar, ssize_t offset,
 			       const char *name);
 #endif
+static inline void ata_port_desc_misc(struct ata_port *ap, int irq)
+{
+	ata_port_desc(ap, "irq %d", irq);
+	ata_port_desc(ap, "lpm-pol %d", ap->target_lpm_policy);
+}
 
 static inline bool ata_tag_internal(unsigned int tag)
 {
-- 
2.41.0

Re: [PATCH v2] ata: ahci: print the lpm policy on boot

[Damien Le Moal]

On 9/6/23 18:22, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> The target LPM policy can be set using either a Kconfig or a kernel module
> parameter.
> 
> However, if the board type is set to anything but board_ahci_low_power,
> then the LPM policy will overridden and set to ATA_LPM_UNKNOWN.
> 
> Additionally, if the default suspend is suspend to idle, depending on the
> hardware capabilities of the HBA, ahci_update_initial_lpm_policy() might
> override the LPM policy to either ATA_LPM_MIN_POWER_WITH_PARTIAL or
> ATA_LPM_MIN_POWER.
> 
> All this means that it is very hard to know which LPM policy a user will
> actually be using on a given system.
> 
> In order to make it easier to debug LPM related issues, print the LPM
> policy on boot.
> 
> One common LPM related issue is that the device fails to link up.
> Because of that, we cannot add this print to ata_dev_configure(), as that
> function is only called after a successful link up. Instead, add the info
> using ata_port_desc(), with the help of a new ata_port_desc_misc() helper.
> The port description is printed once per port during boot.
> 
> Before changes:
> ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 irq 170
> ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 irq 170
> 
> After changes:
> ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 irq 170 lpm-pol 4
> ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 irq 170 lpm-pol 4
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

I am confused... Why not simply:

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index cfb5e6bd03f7..194cf4fcb9bb 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5945,6 +5945,7 @@ int ata_host_register(struct ata_host *host, const struct
scsi_host_template *sh
                                              ap->udma_mask);

                if (!ata_port_is_dummy(ap)) {
+                       ata_port_desc(ap, "lpm-pol %d", ap->target_lpm_policy);
                        ata_port_info(ap, "%cATA max %s %s\n",
                                      (ap->flags & ATA_FLAG_SATA) ? 'S' : 'P',
                                      ata_mode_string(xfer_mask),

?

> ---
> V1 -> V2: Moved the lpm-pol print after the irq print. Added a new helper,
> ata_port_desc_misc(), to help with this.
> 
>  drivers/ata/libahci.c     |  2 +-
>  drivers/ata/libata-core.c |  2 +-
>  drivers/ata/libata-sff.c  | 10 +++++-----
>  drivers/ata/pata_cs5520.c |  2 +-
>  include/linux/libata.h    |  5 +++++
>  5 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index e2bacedf28ef..5354571a3105 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2719,7 +2719,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
>  
>  		if (rc)
>  			return rc;
> -		ata_port_desc(host->ports[i], "irq %d", irq);
> +		ata_port_desc_misc(host->ports[i], irq);
>  	}
>  
>  	return ata_host_register(host, sht);
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 74314311295f..715a8fb6c3e6 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5923,7 +5923,7 @@ int ata_host_activate(struct ata_host *host, int irq,
>  		return rc;
>  
>  	for (i = 0; i < host->n_ports; i++)
> -		ata_port_desc(host->ports[i], "irq %d", irq);
> +		ata_port_desc_misc(host->ports[i], irq);
>  
>  	rc = ata_host_register(host, sht);
>  	/* if failed, just free the IRQ and leave ports alone */
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 8fcc622fcb3d..95a19c4ef2a1 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -2316,7 +2316,7 @@ int ata_pci_sff_activate_host(struct ata_host *host,
>  		for (i = 0; i < 2; i++) {
>  			if (ata_port_is_dummy(host->ports[i]))
>  				continue;
> -			ata_port_desc(host->ports[i], "irq %d", pdev->irq);
> +			ata_port_desc_misc(host->ports[i], pdev->irq);
>  		}
>  	} else if (legacy_mode) {
>  		if (!ata_port_is_dummy(host->ports[0])) {
> @@ -2326,8 +2326,8 @@ int ata_pci_sff_activate_host(struct ata_host *host,
>  			if (rc)
>  				goto out;
>  
> -			ata_port_desc(host->ports[0], "irq %d",
> -				      ATA_PRIMARY_IRQ(pdev));
> +			ata_port_desc_misc(host->ports[0],
> +					   ATA_PRIMARY_IRQ(pdev));
>  		}
>  
>  		if (!ata_port_is_dummy(host->ports[1])) {
> @@ -2337,8 +2337,8 @@ int ata_pci_sff_activate_host(struct ata_host *host,
>  			if (rc)
>  				goto out;
>  
> -			ata_port_desc(host->ports[1], "irq %d",
> -				      ATA_SECONDARY_IRQ(pdev));
> +			ata_port_desc_misc(host->ports[1],
> +					   ATA_SECONDARY_IRQ(pdev));
>  		}
>  	}
>  
> diff --git a/drivers/ata/pata_cs5520.c b/drivers/ata/pata_cs5520.c
> index 422d42761a1d..38795508c2e9 100644
> --- a/drivers/ata/pata_cs5520.c
> +++ b/drivers/ata/pata_cs5520.c
> @@ -212,7 +212,7 @@ static int cs5520_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>  		if (rc)
>  			return rc;
>  
> -		ata_port_desc(ap, "irq %d", irq[i]);
> +		ata_port_desc_misc(ap, irq[i]);
>  	}
>  
>  	return ata_host_register(host, &cs5520_sht);
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 52d58b13e5ee..f7bfc87b34ff 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1553,6 +1553,11 @@ void ata_port_desc(struct ata_port *ap, const char *fmt, ...);
>  extern void ata_port_pbar_desc(struct ata_port *ap, int bar, ssize_t offset,
>  			       const char *name);
>  #endif
> +static inline void ata_port_desc_misc(struct ata_port *ap, int irq)
> +{
> +	ata_port_desc(ap, "irq %d", irq);
> +	ata_port_desc(ap, "lpm-pol %d", ap->target_lpm_policy);
> +}
>  
>  static inline bool ata_tag_internal(unsigned int tag)
>  {

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] ata: ahci: print the lpm policy on boot

[Niklas Cassel]

Hello Damien,

On Thu, Sep 07, 2023 at 06:18:24AM +0900, Damien Le Moal wrote:
> On 9/6/23 18:22, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> > 
> > The target LPM policy can be set using either a Kconfig or a kernel module
> > parameter.
> > 
> > However, if the board type is set to anything but board_ahci_low_power,
> > then the LPM policy will overridden and set to ATA_LPM_UNKNOWN.
> > 
> > Additionally, if the default suspend is suspend to idle, depending on the
> > hardware capabilities of the HBA, ahci_update_initial_lpm_policy() might
> > override the LPM policy to either ATA_LPM_MIN_POWER_WITH_PARTIAL or
> > ATA_LPM_MIN_POWER.
> > 
> > All this means that it is very hard to know which LPM policy a user will
> > actually be using on a given system.
> > 
> > In order to make it easier to debug LPM related issues, print the LPM
> > policy on boot.
> > 
> > One common LPM related issue is that the device fails to link up.
> > Because of that, we cannot add this print to ata_dev_configure(), as that
> > function is only called after a successful link up. Instead, add the info
> > using ata_port_desc(), with the help of a new ata_port_desc_misc() helper.
> > The port description is printed once per port during boot.
> > 
> > Before changes:
> > ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 irq 170
> > ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 irq 170
> > 
> > After changes:
> > ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 irq 170 lpm-pol 4
> > ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 irq 170 lpm-pol 4
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> 
> I am confused... Why not simply:
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index cfb5e6bd03f7..194cf4fcb9bb 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5945,6 +5945,7 @@ int ata_host_register(struct ata_host *host, const struct
> scsi_host_template *sh
>                                               ap->udma_mask);
> 
>                 if (!ata_port_is_dummy(ap)) {
> +                       ata_port_desc(ap, "lpm-pol %d", ap->target_lpm_policy);
>                         ata_port_info(ap, "%cATA max %s %s\n",
>                                       (ap->flags & ATA_FLAG_SATA) ? 'S' : 'P',
>                                       ata_mode_string(xfer_mask),
> 
> ?

If AHCI_HFLAG_MULTI_MSI is set, then ahci_host_activate_multi_irqs() will
be called, instead of ata_host_activate():
https://github.com/torvalds/linux/blob/v6.5/drivers/ata/libahci.c#L2755-L2758
so that is why ahci_host_activate_multi_irqs() is also updated.

ata_piix.c implements .set_lpm:
https://github.com/torvalds/linux/blob/v6.5/drivers/ata/ata_piix.c#L1108
and calls ata_pci_sff_activate_host() to activate the host:
https://github.com/torvalds/linux/blob/v6.5/drivers/ata/ata_piix.c#L1746
so that is why ata_pci_sff_activate_host() is also updated.

The only "unnecessary" update is to cs5520_init_one(), which is a pata
driver, and does thus not support any LPM modes. However, in order to be
consistent with all other prints (drivers), print it there as well.
(This specific driver will always print 0, which is technically not a lie.)


Kind regards,
Niklas

Re: [PATCH v2] ata: ahci: print the lpm policy on boot

[Damien Le Moal]

On 9/6/23 18:22, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> The target LPM policy can be set using either a Kconfig or a kernel module
> parameter.
> 
> However, if the board type is set to anything but board_ahci_low_power,
> then the LPM policy will overridden and set to ATA_LPM_UNKNOWN.
> 
> Additionally, if the default suspend is suspend to idle, depending on the
> hardware capabilities of the HBA, ahci_update_initial_lpm_policy() might
> override the LPM policy to either ATA_LPM_MIN_POWER_WITH_PARTIAL or
> ATA_LPM_MIN_POWER.
> 
> All this means that it is very hard to know which LPM policy a user will
> actually be using on a given system.
> 
> In order to make it easier to debug LPM related issues, print the LPM
> policy on boot.
> 
> One common LPM related issue is that the device fails to link up.
> Because of that, we cannot add this print to ata_dev_configure(), as that
> function is only called after a successful link up. Instead, add the info
> using ata_port_desc(), with the help of a new ata_port_desc_misc() helper.
> The port description is printed once per port during boot.
> 
> Before changes:
> ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 irq 170
> ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 irq 170
> 
> After changes:
> ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 irq 170 lpm-pol 4
> ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 irq 170 lpm-pol 4
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

Applied to for-6.7. Thanks !


-- 
Damien Le Moal
Western Digital Research