| Message ID | 20250630062637.258329-3-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improve link power management | expand |
On Mon, Jun 30, 2025 at 03:26:29PM +0900, Damien Le Moal wrote: > Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a > device in ata_dev_configure() to the function ata_dev_config_lpm(). > This allows having all LPM related settings in one place to facilitate > maintenance. > > No functional changes. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-by: Niklas Cassel <cassel@kernel.org>
On 6/30/25 08:26, Damien Le Moal wrote: > Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a > device in ata_dev_configure() to the function ata_dev_config_lpm(). > This allows having all LPM related settings in one place to facilitate > maintenance. > > No functional changes. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/libata-core.c | 43 +++++++++++++++++++++++---------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 0d85474f6640..fdce96fd3ffa 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -2798,6 +2798,32 @@ static void ata_dev_config_lpm(struct ata_device *dev) > struct ata_port *ap = dev->link->ap; > unsigned int err_mask; > > + if (ap->flags & ATA_FLAG_NO_LPM) { > + /* > + * When the port does not support LPM, we cannot support it on > + * the device either. > + */ > + dev->quirks |= ATA_QUIRK_NOLPM; > + } else { > + /* > + * Some WD SATA-1 drives have issues with LPM, turn on NOLPM for > + * them. > + */ > + if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) && > + (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2) > + dev->quirks |= ATA_QUIRK_NOLPM; > + > + /* ATI specific quirk */ > + if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) && > + ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) > + dev->quirks |= ATA_QUIRK_NOLPM; > + } > + > + if (dev->quirks & ATA_QUIRK_NOLPM) { > + ata_dev_warn(dev, "LPM support broken, forcing max_power\n"); > + ap->target_lpm_policy = ATA_LPM_MAX_POWER; > + } > + > /* > * If the device port does not support Device Initiated Power Management > * (DIPM), and the device supports this feature, disable it. > @@ -2881,23 +2907,6 @@ int ata_dev_configure(struct ata_device *dev) > if (rc) > return rc; > > - /* some WD SATA-1 drives have issues with LPM, turn on NOLPM for them */ > - if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) && > - (id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2) > - dev->quirks |= ATA_QUIRK_NOLPM; > - > - if (dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI && > - ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) > - dev->quirks |= ATA_QUIRK_NOLPM; > - > - if (ap->flags & ATA_FLAG_NO_LPM) > - dev->quirks |= ATA_QUIRK_NOLPM; > - > - if (dev->quirks & ATA_QUIRK_NOLPM) { > - ata_dev_warn(dev, "LPM support broken, forcing max_power\n"); > - dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER; > - } > - > /* let ACPI work its magic */ > rc = ata_acpi_on_devcfg(dev); > if (rc) And this now is only dealing with modifying LPM setting, independent on any DIPM setting. Why not make two functions (one for DIPM and one for LPM) so make matters less confusing? Cheers, Hannes
On 7/1/25 3:13 PM, Hannes Reinecke wrote: > On 6/30/25 08:26, Damien Le Moal wrote: >> Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a >> device in ata_dev_configure() to the function ata_dev_config_lpm(). >> This allows having all LPM related settings in one place to facilitate >> maintenance. >> >> No functional changes. >> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> --- >> drivers/ata/libata-core.c | 43 +++++++++++++++++++++++---------------- >> 1 file changed, 26 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index 0d85474f6640..fdce96fd3ffa 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -2798,6 +2798,32 @@ static void ata_dev_config_lpm(struct ata_device *dev) >> struct ata_port *ap = dev->link->ap; >> unsigned int err_mask; >> + if (ap->flags & ATA_FLAG_NO_LPM) { >> + /* >> + * When the port does not support LPM, we cannot support it on >> + * the device either. >> + */ >> + dev->quirks |= ATA_QUIRK_NOLPM; >> + } else { >> + /* >> + * Some WD SATA-1 drives have issues with LPM, turn on NOLPM for >> + * them. >> + */ >> + if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) && >> + (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2) >> + dev->quirks |= ATA_QUIRK_NOLPM; >> + >> + /* ATI specific quirk */ >> + if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) && >> + ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) >> + dev->quirks |= ATA_QUIRK_NOLPM; >> + } >> + >> + if (dev->quirks & ATA_QUIRK_NOLPM) { >> + ata_dev_warn(dev, "LPM support broken, forcing max_power\n"); >> + ap->target_lpm_policy = ATA_LPM_MAX_POWER; >> + } >> + >> /* >> * If the device port does not support Device Initiated Power Management >> * (DIPM), and the device supports this feature, disable it. >> @@ -2881,23 +2907,6 @@ int ata_dev_configure(struct ata_device *dev) >> if (rc) >> return rc; >> - /* some WD SATA-1 drives have issues with LPM, turn on NOLPM for them */ >> - if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) && >> - (id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2) >> - dev->quirks |= ATA_QUIRK_NOLPM; >> - >> - if (dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI && >> - ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) >> - dev->quirks |= ATA_QUIRK_NOLPM; >> - >> - if (ap->flags & ATA_FLAG_NO_LPM) >> - dev->quirks |= ATA_QUIRK_NOLPM; >> - >> - if (dev->quirks & ATA_QUIRK_NOLPM) { >> - ata_dev_warn(dev, "LPM support broken, forcing max_power\n"); >> - dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER; >> - } >> - >> /* let ACPI work its magic */ >> rc = ata_acpi_on_devcfg(dev); >> if (rc) > > And this now is only dealing with modifying LPM setting, independent on > any DIPM setting. Why not make two functions (one for DIPM and one for > LPM) so make matters less confusing? "less confusing" with all LPM things is I think not possible :) The idea is to keep things together as much as possible to facilitate tweaking/maintenance. There is more like this coming to get port capabilities out of ahci.c and into generic libata so that platform AHCI and libsas adapters can be supported too. Right now, it is pretty much LPM == AHCI... > > Cheers, > > Hannes
On 7/1/25 08:43, Damien Le Moal wrote: > On 7/1/25 3:13 PM, Hannes Reinecke wrote: >> On 6/30/25 08:26, Damien Le Moal wrote: >>> Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a >>> device in ata_dev_configure() to the function ata_dev_config_lpm(). >>> This allows having all LPM related settings in one place to facilitate >>> maintenance. >>> >>> No functional changes. >>> >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>> --- >>> drivers/ata/libata-core.c | 43 +++++++++++++++++++++++---------------- >>> 1 file changed, 26 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>> index 0d85474f6640..fdce96fd3ffa 100644 >>> --- a/drivers/ata/libata-core.c >>> +++ b/drivers/ata/libata-core.c >>> @@ -2798,6 +2798,32 @@ static void ata_dev_config_lpm(struct ata_device *dev) >>> struct ata_port *ap = dev->link->ap; >>> unsigned int err_mask; >>> + if (ap->flags & ATA_FLAG_NO_LPM) { >>> + /* >>> + * When the port does not support LPM, we cannot support it on >>> + * the device either. >>> + */ >>> + dev->quirks |= ATA_QUIRK_NOLPM; >>> + } else { >>> + /* >>> + * Some WD SATA-1 drives have issues with LPM, turn on NOLPM for >>> + * them. >>> + */ >>> + if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) && >>> + (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2) >>> + dev->quirks |= ATA_QUIRK_NOLPM; >>> + >>> + /* ATI specific quirk */ >>> + if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) && >>> + ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) >>> + dev->quirks |= ATA_QUIRK_NOLPM; >>> + } >>> + >>> + if (dev->quirks & ATA_QUIRK_NOLPM) { >>> + ata_dev_warn(dev, "LPM support broken, forcing max_power\n"); >>> + ap->target_lpm_policy = ATA_LPM_MAX_POWER; >>> + } >>> + >>> /* >>> * If the device port does not support Device Initiated Power Management >>> * (DIPM), and the device supports this feature, disable it. >>> @@ -2881,23 +2907,6 @@ int ata_dev_configure(struct ata_device *dev) >>> if (rc) >>> return rc; >>> - /* some WD SATA-1 drives have issues with LPM, turn on NOLPM for them */ >>> - if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) && >>> - (id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2) >>> - dev->quirks |= ATA_QUIRK_NOLPM; >>> - >>> - if (dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI && >>> - ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) >>> - dev->quirks |= ATA_QUIRK_NOLPM; >>> - >>> - if (ap->flags & ATA_FLAG_NO_LPM) >>> - dev->quirks |= ATA_QUIRK_NOLPM; >>> - >>> - if (dev->quirks & ATA_QUIRK_NOLPM) { >>> - ata_dev_warn(dev, "LPM support broken, forcing max_power\n"); >>> - dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER; >>> - } >>> - >>> /* let ACPI work its magic */ >>> rc = ata_acpi_on_devcfg(dev); >>> if (rc) >> >> And this now is only dealing with modifying LPM setting, independent on >> any DIPM setting. Why not make two functions (one for DIPM and one for >> LPM) so make matters less confusing? > > "less confusing" with all LPM things is I think not possible :) > > The idea is to keep things together as much as possible to facilitate > tweaking/maintenance. There is more like this coming to get port capabilities > out of ahci.c and into generic libata so that platform AHCI and libsas adapters > can be supported too. Right now, it is pretty much LPM == AHCI... > Ok, makes sense. Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 0d85474f6640..fdce96fd3ffa 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -2798,6 +2798,32 @@ static void ata_dev_config_lpm(struct ata_device *dev) struct ata_port *ap = dev->link->ap; unsigned int err_mask; + if (ap->flags & ATA_FLAG_NO_LPM) { + /* + * When the port does not support LPM, we cannot support it on + * the device either. + */ + dev->quirks |= ATA_QUIRK_NOLPM; + } else { + /* + * Some WD SATA-1 drives have issues with LPM, turn on NOLPM for + * them. + */ + if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) && + (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2) + dev->quirks |= ATA_QUIRK_NOLPM; + + /* ATI specific quirk */ + if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) && + ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) + dev->quirks |= ATA_QUIRK_NOLPM; + } + + if (dev->quirks & ATA_QUIRK_NOLPM) { + ata_dev_warn(dev, "LPM support broken, forcing max_power\n"); + ap->target_lpm_policy = ATA_LPM_MAX_POWER; + } + /* * If the device port does not support Device Initiated Power Management * (DIPM), and the device supports this feature, disable it. @@ -2881,23 +2907,6 @@ int ata_dev_configure(struct ata_device *dev) if (rc) return rc; - /* some WD SATA-1 drives have issues with LPM, turn on NOLPM for them */ - if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) && - (id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2) - dev->quirks |= ATA_QUIRK_NOLPM; - - if (dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI && - ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) - dev->quirks |= ATA_QUIRK_NOLPM; - - if (ap->flags & ATA_FLAG_NO_LPM) - dev->quirks |= ATA_QUIRK_NOLPM; - - if (dev->quirks & ATA_QUIRK_NOLPM) { - ata_dev_warn(dev, "LPM support broken, forcing max_power\n"); - dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER; - } - /* let ACPI work its magic */ rc = ata_acpi_on_devcfg(dev); if (rc)
Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a device in ata_dev_configure() to the function ata_dev_config_lpm(). This allows having all LPM related settings in one place to facilitate maintenance. No functional changes. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-core.c | 43 +++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 17 deletions(-)