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

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

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