lkml.org 
[lkml]   [2022]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v2 2/2] ata: ahci: Protect users from setting policies their drives don't support
    Date
    [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
    \
     
     \ /
      Last update: 2022-04-05 00:43    [W:8.137 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site