| Message ID | 20220303034912.3615390-2-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] ata: ahci: Drop low power policy board type | expand |
On 2022/03/03 5:49, 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 policies > dependent upon slumber (`min_power` or `min_power_with_partial`) affect the > disk. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > Changes from v1->v2: > * Move deeper into codepaths > * Reset to MED_POWER rather than ignore > drivers/ata/libata-sata.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index 071158c0c44c..0dc03888c62b 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,20 @@ 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)) { Weird indentation. > + dev_warn(ap->host->dev, > + "This drive doesn't support slumber; restting policy to MED_POWER\n"); Typo. I would change this to: "This device does not support slumber; defaulting to MED_POWER policy\n" > + policy = ATA_LPM_MED_POWER; > + } > + > rc = sata_scr_read(link, SCR_CONTROL, &scontrol); > if (rc) > return rc;
On 3/3/22 12:49, 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 policies > dependent upon slumber (`min_power` or `min_power_with_partial`) affect the > disk. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> Mario, Can you resend a rebased version of this, on top of libata for-5.19 branch ? > --- > Changes from v1->v2: > * Move deeper into codepaths > * Reset to MED_POWER rather than ignore > drivers/ata/libata-sata.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index 071158c0c44c..0dc03888c62b 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,20 @@ 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)) { > + dev_warn(ap->host->dev, > + "This drive doesn't support slumber; restting policy to MED_POWER\n"); Typo here: s/restting/resetting. Also, s/doesn't/does not. > + policy = ATA_LPM_MED_POWER; Here, shouldn't we use the default policy defined by CONFIG_SATA_LPM_POLICY ? > + } > + > rc = sata_scr_read(link, SCR_CONTROL, &scontrol); > if (rc) > return rc;
[AMD Official Use Only] > -----Original Message----- > From: Damien Le Moal <damien.lemoal@opensource.wdc.com> > Sent: Sunday, April 3, 2022 20:11 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux- > ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; > hdegoede@redhat.com > Subject: Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their > drives don't support > > On 3/3/22 12:49, 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 policies > > dependent upon slumber (`min_power` or `min_power_with_partial`) affect > the > > disk. > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > Mario, > > Can you resend a rebased version of this, on top of libata for-5.19 branch ? OK. > > > --- > > Changes from v1->v2: > > * Move deeper into codepaths > > * Reset to MED_POWER rather than ignore > > drivers/ata/libata-sata.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > > index 071158c0c44c..0dc03888c62b 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,20 @@ 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)) { > > + dev_warn(ap->host->dev, > > + "This drive doesn't support slumber; restting policy to > MED_POWER\n"); > > Typo here: s/restting/resetting. Also, s/doesn't/does not. > > > + policy = ATA_LPM_MED_POWER; > > Here, shouldn't we use the default policy defined by > CONFIG_SATA_LPM_POLICY ? If they set it too aggressively we still don't want to honor it if the drive can't do slumber I would expect. > > > + } > > + > > rc = sata_scr_read(link, SCR_CONTROL, &scontrol); > > if (rc) > > return rc; > > > -- > Damien Le Moal > Western Digital Research
On 4/5/22 04:39, Limonciello, Mario wrote: > [AMD Official Use Only] > > > >> -----Original Message----- >> From: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> Sent: Sunday, April 3, 2022 20:11 >> To: Limonciello, Mario <Mario.Limonciello@amd.com> >> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux- >> ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; >> hdegoede@redhat.com >> Subject: Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their >> drives don't support >> >> On 3/3/22 12:49, 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 policies >>> dependent upon slumber (`min_power` or `min_power_with_partial`) affect >> the >>> disk. >>> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> >> Mario, >> >> Can you resend a rebased version of this, on top of libata for-5.19 branch ? > > > OK. > >> >>> --- >>> Changes from v1->v2: >>> * Move deeper into codepaths >>> * Reset to MED_POWER rather than ignore >>> drivers/ata/libata-sata.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c >>> index 071158c0c44c..0dc03888c62b 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,20 @@ 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)) { >>> + dev_warn(ap->host->dev, >>> + "This drive doesn't support slumber; restting policy to >> MED_POWER\n"); >> >> Typo here: s/restting/resetting. Also, s/doesn't/does not. >> >>> + policy = ATA_LPM_MED_POWER; >> >> Here, shouldn't we use the default policy defined by >> CONFIG_SATA_LPM_POLICY ? > > If they set it too aggressively we still don't want to honor it if the drive > can't do slumber I would expect. True. But if the default is set to a higher performance mode, we should not fall back to the med-power mode. We should either (1) fallback to the closest higher performance policy supported, or (2) not change the current policy at all. no ? See what ahci_update_initial_lpm_policy() does to check the possible "initial" (the default ?) policy. > >> >>> + } >>> + >>> rc = sata_scr_read(link, SCR_CONTROL, &scontrol); >>> if (rc) >>> return rc; >> >> >> -- >> Damien Le Moal >> Western Digital Research
[AMD Official Use Only] > -----Original Message----- > From: Damien Le Moal <damien.lemoal@opensource.wdc.com> > Sent: Monday, April 4, 2022 18:31 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux- > ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; > hdegoede@redhat.com > Subject: Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their > drives don't support > > On 4/5/22 04:39, Limonciello, Mario wrote: > > [AMD Official Use Only] > > > > > > > >> -----Original Message----- > >> From: Damien Le Moal <damien.lemoal@opensource.wdc.com> > >> Sent: Sunday, April 3, 2022 20:11 > >> To: Limonciello, Mario <Mario.Limonciello@amd.com> > >> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux- > >> ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; > >> hdegoede@redhat.com > >> Subject: Re: [PATCH v2 2/2] ata: ahci: Protect users from setting policies > their > >> drives don't support > >> > >> On 3/3/22 12:49, 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 policies > >>> dependent upon slumber (`min_power` or `min_power_with_partial`) > affect > >> the > >>> disk. > >>> > >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >> > >> Mario, > >> > >> Can you resend a rebased version of this, on top of libata for-5.19 branch > ? > > > > > > OK. > > > >> > >>> --- > >>> Changes from v1->v2: > >>> * Move deeper into codepaths > >>> * Reset to MED_POWER rather than ignore > >>> drivers/ata/libata-sata.c | 11 +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > >>> index 071158c0c44c..0dc03888c62b 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,20 @@ 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)) { > >>> + dev_warn(ap->host->dev, > >>> + "This drive doesn't support slumber; restting policy to > >> MED_POWER\n"); > >> > >> Typo here: s/restting/resetting. Also, s/doesn't/does not. > >> > >>> + policy = ATA_LPM_MED_POWER; > >> > >> Here, shouldn't we use the default policy defined by > >> CONFIG_SATA_LPM_POLICY ? > > > > If they set it too aggressively we still don't want to honor it if the drive > > can't do slumber I would expect. > > True. But if the default is set to a higher performance mode, we should > not fall back to the med-power mode. > > We should either (1) fallback to the closest higher performance policy > supported, or (2) not change the current policy at all. no ? > > See what ahci_update_initial_lpm_policy() does to check the possible > "initial" (the default ?) policy. OK - take a look what I did in the resubmission: https://lore.kernel.org/all/20220404194510.9206-2-mario.limonciello@amd.com/ > > > > > > >> > >>> + } > >>> + > >>> rc = sata_scr_read(link, SCR_CONTROL, &scontrol); > >>> if (rc) > >>> return rc; > >> > >> > >> -- > >> Damien Le Moal > >> Western Digital Research > > > -- > Damien Le Moal > Western Digital Research
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 071158c0c44c..0dc03888c62b 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,20 @@ 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)) { + dev_warn(ap->host->dev, + "This drive doesn't support slumber; restting policy to MED_POWER\n"); + policy = ATA_LPM_MED_POWER; + } + 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 policies dependent upon slumber (`min_power` or `min_power_with_partial`) affect the disk. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- Changes from v1->v2: * Move deeper into codepaths * Reset to MED_POWER rather than ignore drivers/ata/libata-sata.c | 11 +++++++++++ 1 file changed, 11 insertions(+)