* [PATCH 0/2] ahci: Add some controls on actual LPM capability @ 2022-04-21 9:43 Runa Guo-oc 2022-04-21 9:43 ` [PATCH 1/2] ahci: Add PhyRdy Change control " Runa Guo-oc 2022-04-21 9:43 ` [PATCH 2/2] ahci: Add PxSCTL.IPM " Runa Guo-oc 0 siblings, 2 replies; 7+ messages in thread From: Runa Guo-oc @ 2022-04-21 9:43 UTC (permalink / raw) To: damien.lemoal, linux-ide, linux-kernel On some platform, when OS enables LPM by default (eg, min_power), then, PhyRdy Change cannot be detected if ahci supports no LPM; DIPM Slumber request cannot be disallowed if ahci's CAP.PSC is set to '1' and CAP.SSC is cleared to '0', which may cause ahci to be an uncertain state (same for Partial). In ahci spec, PhyRdy Change cannot coexist with LPM; when CAP.PSC/SSC is cleared to '0', the PxSCTL.IPM field must be programmed to disallow device initiated Partial/Slumber request. Adds suports to control these cases on actual LPM capability. Runa Guo-oc (2): ahci: Add PhyRdy Change control on actual LPM capability ahci: Add PxSCTL.IPM control on actual LPM capability drivers/ata/ahci.c | 9 +++++++++ drivers/ata/libata-eh.c | 4 ++++ drivers/ata/libata-sata.c | 12 +++++++++++- include/linux/libata.h | 4 ++++ 4 files changed, 28 insertions(+), 1 deletion(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM capability 2022-04-21 9:43 [PATCH 0/2] ahci: Add some controls on actual LPM capability Runa Guo-oc @ 2022-04-21 9:43 ` Runa Guo-oc 2022-04-21 10:39 ` Damien Le Moal 2022-04-21 9:43 ` [PATCH 2/2] ahci: Add PxSCTL.IPM " Runa Guo-oc 1 sibling, 1 reply; 7+ messages in thread From: Runa Guo-oc @ 2022-04-21 9:43 UTC (permalink / raw) To: damien.lemoal, linux-ide, linux-kernel On some platform, when OS enables LPM by default (eg, min_power), then, PhyRdy Change cannot be detected if ahci supports no LPM. In ahci spec, PhyRdy Change cannot coexist with LPM. Adds support to control this case on actual LPM capability. Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> --- drivers/ata/ahci.c | 9 +++++++++ drivers/ata/libata-eh.c | 4 ++++ include/linux/libata.h | 4 ++++ 3 files changed, 17 insertions(+) diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 397dfd2..03f0cb3 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -1870,6 +1870,15 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) else dev_info(&pdev->dev, "SSS flag set, parallel bus scan disabled\n"); + if (hpriv->cap & HOST_CAP_PART) + host->flags |= ATA_HOST_PART; + + if (hpriv->cap & HOST_CAP_SSC) + host->flags |= ATA_HOST_SSC; + + if (hpriv->cap2 & HOST_CAP2_SDS) + host->flags |= ATA_HOST_DEVSLP; + if (pi.flags & ATA_FLAG_EM) ahci_reset_em(host); diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 3307ed4..05b1043 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -3246,6 +3246,10 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, unsigned int err_mask; int rc; + /* if controller does not support lpm, then sets no LPM flags*/ + if (!(ap->host->flags & (ATA_HOST_PART | ATA_HOST_SSC | ATA_HOST_DEVSLP))) + link->flags |= ATA_LFLAG_NO_LPM; + /* if the link or host doesn't do LPM, noop */ if (!IS_ENABLED(CONFIG_SATA_HOST) || (link->flags & ATA_LFLAG_NO_LPM) || (ap && !ap->ops->set_lpm)) diff --git a/include/linux/libata.h b/include/linux/libata.h index 732de90..7a243f4 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -216,6 +216,10 @@ enum { ATA_HOST_PARALLEL_SCAN = (1 << 2), /* Ports on this host can be scanned in parallel */ ATA_HOST_IGNORE_ATA = (1 << 3), /* Ignore ATA devices on this host. */ + ATA_HOST_PART = (1 << 4), /* Host support partial.*/ + ATA_HOST_SSC = (1 << 5), /* Host support slumber.*/ + ATA_HOST_DEVSLP = (1 << 6), /* Host support devslp.*/ + /* bits 24:31 of host->flags are reserved for LLD specific flags */ /* various lengths of time */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM capability 2022-04-21 9:43 ` [PATCH 1/2] ahci: Add PhyRdy Change control " Runa Guo-oc @ 2022-04-21 10:39 ` Damien Le Moal 2022-04-22 9:57 ` RunaGuo-oc 0 siblings, 1 reply; 7+ messages in thread From: Damien Le Moal @ 2022-04-21 10:39 UTC (permalink / raw) To: Runa Guo-oc, linux-ide, linux-kernel, Mario Limonciello On 4/21/22 18:43, Runa Guo-oc wrote: > On some platform, when OS enables LPM by default (eg, min_power), > then, PhyRdy Change cannot be detected if ahci supports no LPM. Do you mean "...if the ahci adapter does not support LPM." ? If that is what you mean, then min_power should not be set. Mario has patches to fix that. > > In ahci spec, PhyRdy Change cannot coexist with LPM. > > Adds support to control this case on actual LPM capability. > > Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> > --- > drivers/ata/ahci.c | 9 +++++++++ > drivers/ata/libata-eh.c | 4 ++++ > include/linux/libata.h | 4 ++++ > 3 files changed, 17 insertions(+) > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index 397dfd2..03f0cb3 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -1870,6 +1870,15 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > else > dev_info(&pdev->dev, "SSS flag set, parallel bus scan disabled\n"); > > + if (hpriv->cap & HOST_CAP_PART) > + host->flags |= ATA_HOST_PART; > + > + if (hpriv->cap & HOST_CAP_SSC) > + host->flags |= ATA_HOST_SSC; > + > + if (hpriv->cap2 & HOST_CAP2_SDS) > + host->flags |= ATA_HOST_DEVSLP; > + > if (pi.flags & ATA_FLAG_EM) > ahci_reset_em(host); > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > index 3307ed4..05b1043 100644 > --- a/drivers/ata/libata-eh.c > +++ b/drivers/ata/libata-eh.c > @@ -3246,6 +3246,10 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, > unsigned int err_mask; > int rc; > > + /* if controller does not support lpm, then sets no LPM flags*/ > + if (!(ap->host->flags & (ATA_HOST_PART | ATA_HOST_SSC | ATA_HOST_DEVSLP))) > + link->flags |= ATA_LFLAG_NO_LPM; > + > /* if the link or host doesn't do LPM, noop */ > if (!IS_ENABLED(CONFIG_SATA_HOST) || > (link->flags & ATA_LFLAG_NO_LPM) || (ap && !ap->ops->set_lpm)) > diff --git a/include/linux/libata.h b/include/linux/libata.h > index 732de90..7a243f4 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -216,6 +216,10 @@ enum { > ATA_HOST_PARALLEL_SCAN = (1 << 2), /* Ports on this host can be scanned in parallel */ > ATA_HOST_IGNORE_ATA = (1 << 3), /* Ignore ATA devices on this host. */ > > + ATA_HOST_PART = (1 << 4), /* Host support partial.*/ > + ATA_HOST_SSC = (1 << 5), /* Host support slumber.*/ > + ATA_HOST_DEVSLP = (1 << 6), /* Host support devslp.*/ > + > /* bits 24:31 of host->flags are reserved for LLD specific flags */ > > /* various lengths of time */ -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM capability 2022-04-21 10:39 ` Damien Le Moal @ 2022-04-22 9:57 ` RunaGuo-oc 0 siblings, 0 replies; 7+ messages in thread From: RunaGuo-oc @ 2022-04-22 9:57 UTC (permalink / raw) To: Damien Le Moal, linux-ide, linux-kernel, Mario Limonciello, Cobe Chen, Tim Guo, TonyW Wang, Leo Liu On 2022/4/21 18:39, Damien Le Moal wrote: > On 4/21/22 18:43, Runa Guo-oc wrote: >> On some platform, when OS enables LPM by default (eg, min_power), >> then, PhyRdy Change cannot be detected if ahci supports no LPM. > Do you mean "...if the ahci adapter does not support LPM." ? Yes. > If that is what you mean, then min_power should not be set. Yes, I agree with you. But, as we know, link_power_management is a user policy which can be modified, if some users are not familiar with ahci spec, then the above case may happen. > Mario has patches to fix that. Hmm. How to patch this case ? >> In ahci spec, PhyRdy Change cannot coexist with LPM. >> >> Adds support to control this case on actual LPM capability. >> >> Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> >> --- >> drivers/ata/ahci.c | 9 +++++++++ >> drivers/ata/libata-eh.c | 4 ++++ >> include/linux/libata.h | 4 ++++ >> 3 files changed, 17 insertions(+) >> >> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >> index 397dfd2..03f0cb3 100644 >> --- a/drivers/ata/ahci.c >> +++ b/drivers/ata/ahci.c >> @@ -1870,6 +1870,15 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> else >> dev_info(&pdev->dev, "SSS flag set, parallel bus scan disabled\n"); >> >> + if (hpriv->cap & HOST_CAP_PART) >> + host->flags |= ATA_HOST_PART; >> + >> + if (hpriv->cap & HOST_CAP_SSC) >> + host->flags |= ATA_HOST_SSC; >> + >> + if (hpriv->cap2 & HOST_CAP2_SDS) >> + host->flags |= ATA_HOST_DEVSLP; >> + >> if (pi.flags & ATA_FLAG_EM) >> ahci_reset_em(host); >> >> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c >> index 3307ed4..05b1043 100644 >> --- a/drivers/ata/libata-eh.c >> +++ b/drivers/ata/libata-eh.c >> @@ -3246,6 +3246,10 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, >> unsigned int err_mask; >> int rc; >> >> + /* if controller does not support lpm, then sets no LPM flags*/ >> + if (!(ap->host->flags & (ATA_HOST_PART | ATA_HOST_SSC | ATA_HOST_DEVSLP))) >> + link->flags |= ATA_LFLAG_NO_LPM; >> + >> /* if the link or host doesn't do LPM, noop */ >> if (!IS_ENABLED(CONFIG_SATA_HOST) || >> (link->flags & ATA_LFLAG_NO_LPM) || (ap && !ap->ops->set_lpm)) >> diff --git a/include/linux/libata.h b/include/linux/libata.h >> index 732de90..7a243f4 100644 >> --- a/include/linux/libata.h >> +++ b/include/linux/libata.h >> @@ -216,6 +216,10 @@ enum { >> ATA_HOST_PARALLEL_SCAN = (1 << 2), /* Ports on this host can be scanned in parallel */ >> ATA_HOST_IGNORE_ATA = (1 << 3), /* Ignore ATA devices on this host. */ >> >> + ATA_HOST_PART = (1 << 4), /* Host support partial.*/ >> + ATA_HOST_SSC = (1 << 5), /* Host support slumber.*/ >> + ATA_HOST_DEVSLP = (1 << 6), /* Host support devslp.*/ >> + >> /* bits 24:31 of host->flags are reserved for LLD specific flags */ >> >> /* various lengths of time */ > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] ahci: Add PxSCTL.IPM control on actual LPM capability 2022-04-21 9:43 [PATCH 0/2] ahci: Add some controls on actual LPM capability Runa Guo-oc 2022-04-21 9:43 ` [PATCH 1/2] ahci: Add PhyRdy Change control " Runa Guo-oc @ 2022-04-21 9:43 ` Runa Guo-oc 2022-04-21 10:53 ` Damien Le Moal 1 sibling, 1 reply; 7+ messages in thread From: Runa Guo-oc @ 2022-04-21 9:43 UTC (permalink / raw) To: damien.lemoal, linux-ide, linux-kernel On some platform, when OS enables LPM by default (eg, min_power), then, DIPM slumber request cannot be disallowed if ahci's CAP.PSC is set to '1' and CAP.SSC is cleared to '0', which may cause ahci to be an uncertain state (same for Partial). In ahci spec, when CAP.PSC/SSC is cleared to '0', the PxSCTL.IPM field must be programmed to disallow device initiated Partial/ Slumber request. Adds support to control this case on actual LPM capability. Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> --- drivers/ata/libata-sata.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 7a5fe41..e6195cf 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -394,9 +394,19 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy, case ATA_LPM_MED_POWER_WITH_DIPM: case ATA_LPM_MIN_POWER_WITH_PARTIAL: case ATA_LPM_MIN_POWER: - if (ata_link_nr_enabled(link) > 0) + if (ata_link_nr_enabled(link) > 0) { /* no restrictions on LPM transitions */ scontrol &= ~(0x7 << 8); + + /* if controller does not support partial, then disallows it, + * the same for slumber + */ + if (!(link->ap->host->flags & ATA_HOST_PART)) + scontrol |= (0x1 << 8); + + if (!(link->ap->host->flags & ATA_HOST_SSC)) + scontrol |= (0x2 << 8); + } else { /* empty port, power off */ scontrol &= ~0xf; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ahci: Add PxSCTL.IPM control on actual LPM capability 2022-04-21 9:43 ` [PATCH 2/2] ahci: Add PxSCTL.IPM " Runa Guo-oc @ 2022-04-21 10:53 ` Damien Le Moal 2022-04-22 9:58 ` RunaGuo-oc 0 siblings, 1 reply; 7+ messages in thread From: Damien Le Moal @ 2022-04-21 10:53 UTC (permalink / raw) To: Runa Guo-oc, linux-ide, linux-kernel On 4/21/22 18:43, Runa Guo-oc wrote: > On some platform, when OS enables LPM by default (eg, min_power), > then, DIPM slumber request cannot be disallowed if ahci's CAP.PSC > is set to '1' and CAP.SSC is cleared to '0', which may cause ahci > to be an uncertain state (same for Partial). > > In ahci spec, when CAP.PSC/SSC is cleared to '0', the PxSCTL.IPM > field must be programmed to disallow device initiated Partial/ > Slumber request. > > Adds support to control this case on actual LPM capability. s/Adds/Add Overall, I need to reread the specs to confirm all this. > > Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> > --- > drivers/ata/libata-sata.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index 7a5fe41..e6195cf 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -394,9 +394,19 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy, > case ATA_LPM_MED_POWER_WITH_DIPM: > case ATA_LPM_MIN_POWER_WITH_PARTIAL: > case ATA_LPM_MIN_POWER: > - if (ata_link_nr_enabled(link) > 0) > + if (ata_link_nr_enabled(link) > 0) { > /* no restrictions on LPM transitions */> scontrol &= ~(0x7 << 8); Given that the added code below adds restrictions, this comment is strange. Better change it to something like: /* Assume no restrictions on LPM transitions */ > + > + /* if controller does not support partial, then disallows it, > + * the same for slumber > + */ Please correctly format the comment and check the grammar. Some like below is easier to read. /* * If the controller does not support partial or * slumber then disallow these transitions. */ > + if (!(link->ap->host->flags & ATA_HOST_PART)) > + scontrol |= (0x1 << 8); > + > + if (!(link->ap->host->flags & ATA_HOST_SSC)) > + scontrol |= (0x2 << 8); > + } > else { Please do not leave this else here. Put it on the same line as the closing bracket and enclose the below statements in brackets too. > /* empty port, power off */ > scontrol &= ~0xf; } else { /* empty port, power off */ scontrol &= ~0xf; } -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ahci: Add PxSCTL.IPM control on actual LPM capability 2022-04-21 10:53 ` Damien Le Moal @ 2022-04-22 9:58 ` RunaGuo-oc 0 siblings, 0 replies; 7+ messages in thread From: RunaGuo-oc @ 2022-04-22 9:58 UTC (permalink / raw) To: Damien Le Moal, linux-ide, linux-kernel, Cobe Chen, Tim Guo, TonyW Wang, Leo Liu On 2022/4/21 18:53, Damien Le Moal wrote: > On 4/21/22 18:43, Runa Guo-oc wrote: >> On some platform, when OS enables LPM by default (eg, min_power), >> then, DIPM slumber request cannot be disallowed if ahci's CAP.PSC >> is set to '1' and CAP.SSC is cleared to '0', which may cause ahci >> to be an uncertain state (same for Partial). >> >> In ahci spec, when CAP.PSC/SSC is cleared to '0', the PxSCTL.IPM >> field must be programmed to disallow device initiated Partial/ >> Slumber request. >> >> Adds support to control this case on actual LPM capability. > s/Adds/Add Sorry, here should use 'Add' instead of 'Adds'. > Overall, I need to reread the specs to confirm all this. Ok. >> Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com> >> --- >> drivers/ata/libata-sata.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c >> index 7a5fe41..e6195cf 100644 >> --- a/drivers/ata/libata-sata.c >> +++ b/drivers/ata/libata-sata.c >> @@ -394,9 +394,19 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy, >> case ATA_LPM_MED_POWER_WITH_DIPM: >> case ATA_LPM_MIN_POWER_WITH_PARTIAL: >> case ATA_LPM_MIN_POWER: >> - if (ata_link_nr_enabled(link) > 0) >> + if (ata_link_nr_enabled(link) > 0) { >> /* no restrictions on LPM transitions */> scontrol &= ~(0x7 << 8); > Given that the added code below adds restrictions, this comment is > strange. Better change it to something like: > > /* Assume no restrictions on LPM transitions */ > >> + >> + /* if controller does not support partial, then disallows it, >> + * the same for slumber >> + */ > Please correctly format the comment and check the grammar. Some like below > is easier to read. > > /* > * If the controller does not support partial or > * slumber then disallow these transitions. > */ > >> + if (!(link->ap->host->flags & ATA_HOST_PART)) >> + scontrol |= (0x1 << 8); >> + >> + if (!(link->ap->host->flags & ATA_HOST_SSC)) >> + scontrol |= (0x2 << 8); >> + } >> else { > Please do not leave this else here. Put it on the same line as the closing > bracket and enclose the below statements in brackets too. > >> /* empty port, power off */ >> scontrol &= ~0xf; > } else { > /* empty port, power off */ > scontrol &= ~0xf; > } I'll change it like below, + if (ata_link_nr_enabled(link) > 0) { - /* no restrictions on LPM transitions */ + /* Assume no restrictions on LPM transitions */ scontrol &= ~(0x7 << 8); + /* + * If the controller does not support partial or + * slumber then disallow these transitions. + */ + if (!(link->ap->host->flags & ATA_HOST_PART)) + scontrol |= (0x1 << 8); + + if (!(link->ap->host->flags & ATA_HOST_SSC)) + scontrol |= (0x2 << 8); } else { /* empty port, power off */ scontrol &= ~0xf; } ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-04-22 9:58 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-04-21 9:43 [PATCH 0/2] ahci: Add some controls on actual LPM capability Runa Guo-oc 2022-04-21 9:43 ` [PATCH 1/2] ahci: Add PhyRdy Change control " Runa Guo-oc 2022-04-21 10:39 ` Damien Le Moal 2022-04-22 9:57 ` RunaGuo-oc 2022-04-21 9:43 ` [PATCH 2/2] ahci: Add PxSCTL.IPM " Runa Guo-oc 2022-04-21 10:53 ` Damien Le Moal 2022-04-22 9:58 ` RunaGuo-oc
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox