Message ID | 20220225181030.980223-2-mario.limonciello@amd.com |
---|---|
State | New |
Headers | show |
Series | [RFC,1/2] ata: ahci: Drop low power policy board type | expand |
Hi, On 2/25/22 19:10, 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> > --- > drivers/ata/ahci.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index 17d757ad7111..af8999453084 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -1584,8 +1584,16 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports, > static void ahci_update_initial_lpm_policy(struct ata_port *ap, > struct ahci_host_priv *hpriv) > { > + struct pci_dev *pdev = to_pci_dev(ap->host->dev); > int policy = CONFIG_SATA_LPM_POLICY; > > + if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL && > + !(hpriv->cap & HOST_CAP_SSC)) { > + dev_warn(&pdev->dev, > + "This drive doesn't support slumber; ignoring default SATA policy\n"); > + return; > + } > + Don't the capabilties get checked later when the policy gets applied ? At least I think they do get checked later, but I have not looked at this code for years ... ? Regards, Hans > /* user modified policy via module param */ > if (mobile_lpm_policy != -1) { > policy = mobile_lpm_policy;
[AMD Official Use Only] > -----Original Message----- > From: Hans de Goede <hdegoede@redhat.com> > Sent: Friday, February 25, 2022 15:20 > To: Limonciello, Mario <Mario.Limonciello@amd.com>; Damien Le Moal > <damien.lemoal@opensource.wdc.com> > Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux- > ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org> > Subject: Re: [RFC 2/2] ata: ahci: Protect users from setting policies their > drives don't support > > Hi, > > On 2/25/22 19:10, 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> > > --- > > drivers/ata/ahci.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > > index 17d757ad7111..af8999453084 100644 > > --- a/drivers/ata/ahci.c > > +++ b/drivers/ata/ahci.c > > @@ -1584,8 +1584,16 @@ static int ahci_init_msi(struct pci_dev *pdev, > unsigned int n_ports, > > static void ahci_update_initial_lpm_policy(struct ata_port *ap, > > struct ahci_host_priv *hpriv) > > { > > + struct pci_dev *pdev = to_pci_dev(ap->host->dev); > > int policy = CONFIG_SATA_LPM_POLICY; > > > > + if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL && > > + !(hpriv->cap & HOST_CAP_SSC)) { > > + dev_warn(&pdev->dev, > > + "This drive doesn't support slumber; ignoring default > SATA policy\n"); > > + return; > > + } > > + > > Don't the capabilties get checked later when the policy gets applied ? > > At least I think they do get checked later, but I have not looked > at this code for years ... ? There's a bunch of layers of indirection so I might have missed something, but I didn't see anything in sata_link_scr_lpm or anywhere else for that matter that actually checked HOST_CAP_SSC. > > Regards, > > Hans > > > > /* user modified policy via module param */ > > if (mobile_lpm_policy != -1) { > > policy = mobile_lpm_policy;
Hi, On 2/25/22 22:24, Limonciello, Mario wrote: > [AMD Official Use Only] > > > >> -----Original Message----- >> From: Hans de Goede <hdegoede@redhat.com> >> Sent: Friday, February 25, 2022 15:20 >> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Damien Le Moal >> <damien.lemoal@opensource.wdc.com> >> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux- >> ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org> >> Subject: Re: [RFC 2/2] ata: ahci: Protect users from setting policies their >> drives don't support >> >> Hi, >> >> On 2/25/22 19:10, 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> >>> --- >>> drivers/ata/ahci.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >>> index 17d757ad7111..af8999453084 100644 >>> --- a/drivers/ata/ahci.c >>> +++ b/drivers/ata/ahci.c >>> @@ -1584,8 +1584,16 @@ static int ahci_init_msi(struct pci_dev *pdev, >> unsigned int n_ports, >>> static void ahci_update_initial_lpm_policy(struct ata_port *ap, >>> struct ahci_host_priv *hpriv) >>> { >>> + struct pci_dev *pdev = to_pci_dev(ap->host->dev); >>> int policy = CONFIG_SATA_LPM_POLICY; >>> >>> + if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL && >>> + !(hpriv->cap & HOST_CAP_SSC)) { >>> + dev_warn(&pdev->dev, >>> + "This drive doesn't support slumber; ignoring default >> SATA policy\n"); >>> + return; >>> + } >>> + >> >> Don't the capabilties get checked later when the policy gets applied ? >> >> At least I think they do get checked later, but I have not looked >> at this code for years ... ? > > There's a bunch of layers of indirection so I might have missed something, > but I didn't see anything in sata_link_scr_lpm or anywhere else for that > matter that actually checked HOST_CAP_SSC. Hmm, ok. Note that the user can still change the policy with an echo to sysfs. So I think it would be better to do a fix where HOST_CAP_SSC gets checked when the features are actually being enabled. Or at least also at a HOST_CAP_SSC check to the sysfs write functions. Regards, Hans >>> /* user modified policy via module param */ >>> if (mobile_lpm_policy != -1) { >>> policy = mobile_lpm_policy;
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 17d757ad7111..af8999453084 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -1584,8 +1584,16 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports, static void ahci_update_initial_lpm_policy(struct ata_port *ap, struct ahci_host_priv *hpriv) { + struct pci_dev *pdev = to_pci_dev(ap->host->dev); int policy = CONFIG_SATA_LPM_POLICY; + if (policy >= ATA_LPM_MIN_POWER_WITH_PARTIAL && + !(hpriv->cap & HOST_CAP_SSC)) { + dev_warn(&pdev->dev, + "This drive doesn't support slumber; ignoring default SATA policy\n"); + return; + } + /* user modified policy via module param */ if (mobile_lpm_policy != -1) { policy = mobile_lpm_policy;
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> --- drivers/ata/ahci.c | 8 ++++++++ 1 file changed, 8 insertions(+)