| Message ID | 20220404194510.9206-2-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] ata: ahci: Drop low power policy board type | expand |
On 4/5/22 04:45, Mario Limonciello wrote: > As the default low power policy applies to more chipsets and drives, it's > important to make sure that drives actually support the policy that a user > selected in their kernel configuration. > > If the drive doesn't support slumber, don't let the default policy for the > ATA port be `min_power` or `min_power_with_partial`. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v2->v3: > * Fix typo > * Try to reset to CONFIG_SATA_LPM_POLICY unless that's too aggressive > then take MED_POWER. > drivers/ata/libata-sata.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index 044a16daa2d4..b41de7e03dce 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -13,6 +13,7 @@ > #include <scsi/scsi_device.h> > #include <linux/libata.h> > > +#include "ahci.h" > #include "libata.h" > #include "libata-transport.h" > > @@ -368,10 +369,21 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy, > bool spm_wakeup) > { > struct ata_eh_context *ehc = &link->eh_context; > + struct ata_port *ap = link->ap; > + struct ahci_host_priv *hpriv; That does not work. This function is used by the ata_piix driver too, which is not ahci... > bool woken_up = false; > u32 scontrol; > int rc; > > + hpriv = ap->host->private_data; > + if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL && > + !(hpriv->cap & HOST_CAP_SSC)) { ...which mean that this flag needs to be moved to struct ata_host to keep this function generic for different LLDDs. > + policy = (CONFIG_SATA_LPM_POLICY < ATA_LPM_MIN_POWER_WITH_PARTIAL) ? > + CONFIG_SATA_LPM_POLICY : ATA_LPM_MED_POWER; I really prefer an explicit "if () else" for readability. Also, please revert here to CONFIG_SATA_MOBILE_LPM_POLICY. > + dev_warn(ap->host->dev, > + "This drive doesn't support slumber; resetting policy to %d\n", policy); s/doesn't/does not > + } > + > rc = sata_scr_read(link, SCR_CONTROL, &scontrol); > if (rc) > return rc; Also, as patch 3 of this series, can you add the patch that changes CONFIG_SATA_MOBILE_LPM_POLICY to being "3" by default ? And please use a cover letter when you have more than 1 patch. The change log can then be in the cover letter instead of in each patch.
On 4/6/22 11:24, Damien Le Moal wrote: > On 4/5/22 04:45, Mario Limonciello wrote: >> As the default low power policy applies to more chipsets and drives, it's >> important to make sure that drives actually support the policy that a >> user >> selected in their kernel configuration. >> >> If the drive doesn't support slumber, don't let the default policy for >> the >> ATA port be `min_power` or `min_power_with_partial`. >> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v2->v3: >> * Fix typo >> * Try to reset to CONFIG_SATA_LPM_POLICY unless that's too aggressive >> then take MED_POWER. >> drivers/ata/libata-sata.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c >> index 044a16daa2d4..b41de7e03dce 100644 >> --- a/drivers/ata/libata-sata.c >> +++ b/drivers/ata/libata-sata.c >> @@ -13,6 +13,7 @@ >> #include <scsi/scsi_device.h> >> #include <linux/libata.h> >> +#include "ahci.h" >> #include "libata.h" >> #include "libata-transport.h" >> @@ -368,10 +369,21 @@ int sata_link_scr_lpm(struct ata_link *link, >> enum ata_lpm_policy policy, >> bool spm_wakeup) >> { >> struct ata_eh_context *ehc = &link->eh_context; >> + struct ata_port *ap = link->ap; >> + struct ahci_host_priv *hpriv; > > That does not work. This function is used by the ata_piix driver too, > which is not ahci... > >> bool woken_up = false; >> u32 scontrol; >> int rc; >> + hpriv = ap->host->private_data; >> + if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL && >> + !(hpriv->cap & HOST_CAP_SSC)) { > > ...which mean that this flag needs to be moved to struct ata_host to > keep this function generic for different LLDDs. Here I meant having a ata_host flag equivalent, indicating the adapter supports slumber state. > >> + policy = (CONFIG_SATA_LPM_POLICY < >> ATA_LPM_MIN_POWER_WITH_PARTIAL) ? >> + CONFIG_SATA_LPM_POLICY : ATA_LPM_MED_POWER; > > I really prefer an explicit "if () else" for readability. > Also, please revert here to CONFIG_SATA_MOBILE_LPM_POLICY. > >> + dev_warn(ap->host->dev, >> + "This drive doesn't support slumber; resetting policy to >> %d\n", policy); > > s/doesn't/does not > >> + } >> + >> rc = sata_scr_read(link, SCR_CONTROL, &scontrol); >> if (rc) >> return rc; > > Also, as patch 3 of this series, can you add the patch that changes > CONFIG_SATA_MOBILE_LPM_POLICY to being "3" by default ? > > And please use a cover letter when you have more than 1 patch. The > change log can then be in the cover letter instead of in each patch. >
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 044a16daa2d4..b41de7e03dce 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -13,6 +13,7 @@ #include <scsi/scsi_device.h> #include <linux/libata.h> +#include "ahci.h" #include "libata.h" #include "libata-transport.h" @@ -368,10 +369,21 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy, bool spm_wakeup) { struct ata_eh_context *ehc = &link->eh_context; + struct ata_port *ap = link->ap; + struct ahci_host_priv *hpriv; bool woken_up = false; u32 scontrol; int rc; + hpriv = ap->host->private_data; + if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL && + !(hpriv->cap & HOST_CAP_SSC)) { + policy = (CONFIG_SATA_LPM_POLICY < ATA_LPM_MIN_POWER_WITH_PARTIAL) ? + CONFIG_SATA_LPM_POLICY : ATA_LPM_MED_POWER; + dev_warn(ap->host->dev, + "This drive doesn't support slumber; resetting policy to %d\n", policy); + } + rc = sata_scr_read(link, SCR_CONTROL, &scontrol); if (rc) return rc;
As the default low power policy applies to more chipsets and drives, it's important to make sure that drives actually support the policy that a user selected in their kernel configuration. If the drive doesn't support slumber, don't let the default policy for the ATA port be `min_power` or `min_power_with_partial`. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v2->v3: * Fix typo * Try to reset to CONFIG_SATA_LPM_POLICY unless that's too aggressive then take MED_POWER. drivers/ata/libata-sata.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)