On 9/6/23 18:22, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> 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@xxxxxxx>
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
