* [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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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 2022-04-22 22:37 ` Damien Le Moal 0 siblings, 1 reply; 13+ 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] 13+ messages in thread
* Re: [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM capability 2022-04-22 9:57 ` RunaGuo-oc @ 2022-04-22 22:37 ` Damien Le Moal 2022-04-27 10:18 ` Runa Guo-oc 0 siblings, 1 reply; 13+ messages in thread From: Damien Le Moal @ 2022-04-22 22:37 UTC (permalink / raw) To: RunaGuo-oc, linux-ide, Mario Limonciello, Cobe Chen, Tim Guo, TonyW Wang, Leo Liu On 4/22/22 18:57, RunaGuo-oc wrote: > 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. Users should *never* need to be aware of the HW specs and what can or cannot be done with a particular adapter/drive. User actions trying to enable an unsupported feature should be failed, always. In your case though, quickly checking the AHCI specs, the scontrol register bits you change seem to be for preventing *device* initiated power mode transitions, not user/host initiated operations. Your commit message should clearly mention that. But I still need to spend more time re-reading the specs to confirm. Will do that next week. Since you added the CAP flags, these flags should be used to detect low power policies that can be allowed for user actions. Mario, Please rebase and repost your patches ! I rebased the for-5.19 branch on rc3 to have the LPM config name revert. We need to fix this LPM mess :) > >> Mario has patches to fix that. > > > Hmm. How to patch this case ? Mario's patches modify the beginning of the sata_link_scr_lpm() function to prevent setting an LPM policy that the adapter/drive does not support. This together with the correct bits set/reset in the scr register will only allow LPM transitions that are supported. It may also be good to revisit ata_scsi_lpm_store() to prevent the user from setting an unsupported policy. Currently, that uselessly triggers an EH sequence. > >>> 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] 13+ messages in thread
* Re: [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM capability 2022-04-22 22:37 ` Damien Le Moal @ 2022-04-27 10:18 ` Runa Guo-oc 2022-05-02 13:05 ` Damien Le Moal 0 siblings, 1 reply; 13+ messages in thread From: Runa Guo-oc @ 2022-04-27 10:18 UTC (permalink / raw) To: Damien Le Moal, linux-ide, Mario Limonciello, Cobe Chen, Tim Guo, TonyW Wang, Leo Liu On 2022/4/23 6:37, Damien Le Moal wrote: > On 4/22/22 18:57, RunaGuo-oc wrote: >> 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. > Users should *never* need to be aware of the HW specs and what can or > cannot be done with a particular adapter/drive. User actions trying to > enable an unsupported feature should be failed, always. > > In your case though, quickly checking the AHCI specs, the scontrol > register bits you change seem to be for preventing *device* initiated > power mode transitions, not user/host initiated operations. Your commit > message should clearly mention that. But I still need to spend more time > re-reading the specs to confirm. Will do that next week. > > Since you added the CAP flags, these flags should be used to detect low > power policies that can be allowed for user actions. > > Mario, > > Please rebase and repost your patches ! I rebased the for-5.19 branch on > rc3 to have the LPM config name revert. We need to fix this LPM mess :) > >>> Mario has patches to fix that. >> >> Hmm. How to patch this case ? > Mario's patches modify the beginning of the sata_link_scr_lpm() function > to prevent setting an LPM policy that the adapter/drive does not support. > This together with the correct bits set/reset in the scr register will > only allow LPM transitions that are supported. > > It may also be good to revisit ata_scsi_lpm_store() to prevent the user > from setting an unsupported policy. Currently, that uselessly triggers an > EH sequence. To avoid some confusion in this patch set, I want to explain it here. The patch set involves two LPM related issues, one for the ahci adapter does not support LPM (no partial & slumber & devslp), the other for ahci adapter supports part of LPM(eg, only partial, no slumber & devslp). The former case is what I metioned in this mail thread. Namely, when LPM is enabled, actions trying to enable this unsupported feature will be failed, but will disable PORT_IRQ_PHYRDY bit at the beginning of the ahci_set_lpm() function, which would make PhyRdy Changed cannot be detected. So I add flags in the ata_eh_set_lpm() function which will not go to the disable operation. The latter case is what I metioned in "PATCH[2/2]". Namely, if the ahci adapter only supports partial (no slumber & devslp), then when LPM is enabled (eg, min_power), *device* initiated power mode transitions will be enabled with the scontrol register bits setting to indicate no restrictions on LPM transitions. After that, if SSD/HDD sends a DIPM slumber request, it cannot be disallowed by ahci adpter for driver not setting scontrol register bits properly. So I add flags to control it. Therefore, Mario's patches in the sata_link_scr_lpm() function may fix the issue in "PATCH[2/2]". Revisit ata_scsi_lpm_store() to prevent the user from setting an unsupported policy may be a way to fix the issue in "PATCH[1/2]", but there may be another case where some operating system manufacturers setting LPM default enable in driver, like the following code in the ahci_init_one() function, also need to control. if (ap->flags & ATA_FLAG_EM) ap->em_message_type = hpriv->em_msg_type; + ap->target_lpm_policy = ATA_LPM_MIN_POWER; ahci_update_initial_lpm_policy(ap, hpriv); According to my current understanding, the currently submitted patches can solve the above problems, and definitely not the best. I haven't figured out good way to use CAP flags to patch. Hope you can give me some advice, thanks. >>>> 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] 13+ messages in thread
* Re: [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM capability 2022-04-27 10:18 ` Runa Guo-oc @ 2022-05-02 13:05 ` Damien Le Moal 2022-05-07 7:36 ` Runa Guo-oc 0 siblings, 1 reply; 13+ messages in thread From: Damien Le Moal @ 2022-05-02 13:05 UTC (permalink / raw) To: Runa Guo-oc, linux-ide, Mario Limonciello, Cobe Chen, Tim Guo, TonyW Wang, Leo Liu On 2022/04/27 19:18, Runa Guo-oc wrote: > > On 2022/4/23 6:37, Damien Le Moal wrote: >> On 4/22/22 18:57, RunaGuo-oc wrote: >>> 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. >> Users should *never* need to be aware of the HW specs and what can or >> cannot be done with a particular adapter/drive. User actions trying to >> enable an unsupported feature should be failed, always. >> >> In your case though, quickly checking the AHCI specs, the scontrol >> register bits you change seem to be for preventing *device* initiated >> power mode transitions, not user/host initiated operations. Your commit >> message should clearly mention that. But I still need to spend more time >> re-reading the specs to confirm. Will do that next week. >> >> Since you added the CAP flags, these flags should be used to detect low >> power policies that can be allowed for user actions. >> >> Mario, >> >> Please rebase and repost your patches ! I rebased the for-5.19 branch on >> rc3 to have the LPM config name revert. We need to fix this LPM mess :) >> >>>> Mario has patches to fix that. >>> >>> Hmm. How to patch this case ? >> Mario's patches modify the beginning of the sata_link_scr_lpm() function >> to prevent setting an LPM policy that the adapter/drive does not support. >> This together with the correct bits set/reset in the scr register will >> only allow LPM transitions that are supported. >> >> It may also be good to revisit ata_scsi_lpm_store() to prevent the user >> from setting an unsupported policy. Currently, that uselessly triggers an >> EH sequence. > > To avoid some confusion in this patch set, I want to explain it here. > The patch set involves two LPM related issues, one for the ahci adapter > does not support LPM (no partial & slumber & devslp), the other for > ahci adapter supports part of LPM(eg, only partial, no slumber & devslp). > > The former case is what I metioned in this mail thread. Namely, when LPM is > enabled, actions trying to enable this unsupported feature will be failed, > but will disable PORT_IRQ_PHYRDY bit at the beginning of the ahci_set_lpm() > function, which would make PhyRdy Changed cannot be detected. So I add flags > in the ata_eh_set_lpm() function which will not go to the disable operation. > > The latter case is what I metioned in "PATCH[2/2]". Namely, if the ahci > adapter only supports partial (no slumber & devslp), then when LPM is enabled > (eg, min_power), *device* initiated power mode transitions will be enabled > with the scontrol register bits setting to indicate no restrictions on LPM > transitions. After that, if SSD/HDD sends a DIPM slumber request, it cannot > be disallowed by ahci adpter for driver not setting scontrol register bits > properly. So I add flags to control it. > > Therefore, Mario's patches in the sata_link_scr_lpm() function may fix the > issue in "PATCH[2/2]". > > Revisit ata_scsi_lpm_store() to prevent the user from setting an unsupported > policy may be a way to fix the issue in "PATCH[1/2]", but there may be another > case where some operating system manufacturers setting LPM default enable in > driver, like the following code in the ahci_init_one() function, also need to > control. > > if (ap->flags & ATA_FLAG_EM) > ap->em_message_type = hpriv->em_msg_type; > > + ap->target_lpm_policy = ATA_LPM_MIN_POWER; This one looks wrong. This is set inside ahci_update_initial_lpm_policy() according to the default kernel config (CONFIG_SATA_MOBILE_LPM_POLICY) and module param + what the drive can do according to ACPI. The problem though is that the adapter capabilities are not checked in that function, so the initial target lpm policy may be wrong. Since your patch 1/2 adds the hpriv flags indicating the capabilities, you need to use these in ahci_update_initial_lpm_policy() to validate whatever initial policy is asked for by the user. > ahci_update_initial_lpm_policy(ap, hpriv); > > According to my current understanding, the currently submitted patches can > solve the above problems, and definitely not the best. I haven't figured out > good way to use CAP flags to patch. Hope you can give me some advice, thanks. -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM capability 2022-05-02 13:05 ` Damien Le Moal @ 2022-05-07 7:36 ` Runa Guo-oc 2022-05-11 8:14 ` Damien Le Moal 0 siblings, 1 reply; 13+ messages in thread From: Runa Guo-oc @ 2022-05-07 7:36 UTC (permalink / raw) To: Damien Le Moal, linux-ide, Mario Limonciello, Cobe Chen, Tim Guo, TonyW Wang, Leo Liu On 2022/5/2 21:05, Damien Le Moal wrote: > On 2022/04/27 19:18, Runa Guo-oc wrote: >> On 2022/4/23 6:37, Damien Le Moal wrote: >>> On 4/22/22 18:57, RunaGuo-oc wrote: >>>> 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. >>> Users should *never* need to be aware of the HW specs and what can or >>> cannot be done with a particular adapter/drive. User actions trying to >>> enable an unsupported feature should be failed, always. >>> >>> In your case though, quickly checking the AHCI specs, the scontrol >>> register bits you change seem to be for preventing *device* initiated >>> power mode transitions, not user/host initiated operations. Your commit >>> message should clearly mention that. But I still need to spend more time >>> re-reading the specs to confirm. Will do that next week. >>> >>> Since you added the CAP flags, these flags should be used to detect low >>> power policies that can be allowed for user actions. >>> >>> Mario, >>> >>> Please rebase and repost your patches ! I rebased the for-5.19 branch on >>> rc3 to have the LPM config name revert. We need to fix this LPM mess :) >>> >>>>> Mario has patches to fix that. >>>> >>>> Hmm. How to patch this case ? >>> Mario's patches modify the beginning of the sata_link_scr_lpm() function >>> to prevent setting an LPM policy that the adapter/drive does not support. >>> This together with the correct bits set/reset in the scr register will >>> only allow LPM transitions that are supported. >>> >>> It may also be good to revisit ata_scsi_lpm_store() to prevent the user >>> from setting an unsupported policy. Currently, that uselessly triggers an >>> EH sequence. >> To avoid some confusion in this patch set, I want to explain it here. >> The patch set involves two LPM related issues, one for the ahci adapter >> does not support LPM (no partial & slumber & devslp), the other for >> ahci adapter supports part of LPM(eg, only partial, no slumber & devslp). >> >> The former case is what I metioned in this mail thread. Namely, when LPM is >> enabled, actions trying to enable this unsupported feature will be failed, >> but will disable PORT_IRQ_PHYRDY bit at the beginning of the ahci_set_lpm() >> function, which would make PhyRdy Changed cannot be detected. So I add flags >> in the ata_eh_set_lpm() function which will not go to the disable operation. >> >> The latter case is what I metioned in "PATCH[2/2]". Namely, if the ahci >> adapter only supports partial (no slumber & devslp), then when LPM is enabled >> (eg, min_power), *device* initiated power mode transitions will be enabled >> with the scontrol register bits setting to indicate no restrictions on LPM >> transitions. After that, if SSD/HDD sends a DIPM slumber request, it cannot >> be disallowed by ahci adpter for driver not setting scontrol register bits >> properly. So I add flags to control it. >> >> Therefore, Mario's patches in the sata_link_scr_lpm() function may fix the >> issue in "PATCH[2/2]". >> >> Revisit ata_scsi_lpm_store() to prevent the user from setting an unsupported >> policy may be a way to fix the issue in "PATCH[1/2]", but there may be another >> case where some operating system manufacturers setting LPM default enable in >> driver, like the following code in the ahci_init_one() function, also need to >> control. >> >> if (ap->flags & ATA_FLAG_EM) >> ap->em_message_type = hpriv->em_msg_type; >> >> + ap->target_lpm_policy = ATA_LPM_MIN_POWER; > This one looks wrong. This is set inside ahci_update_initial_lpm_policy() > according to the default kernel config (CONFIG_SATA_MOBILE_LPM_POLICY) and > module param + what the drive can do according to ACPI. The problem though is > that the adapter capabilities are not checked in that function, so the initial > target lpm policy may be wrong. > > Since your patch 1/2 adds the hpriv flags indicating the capabilities, you need > to use these in ahci_update_initial_lpm_policy() to validate whatever initial > policy is asked for by the user. Yes, the above code is not rigorous, existing methods provided by kernel as you said should be used in this case. In order to use CAP flags to validate user policy, I review the latest kernel LPM policies, here is my understanding: ATA_LPM_UNKNOWN: default policy, no LPM ATA_LPM_MAX_POWER: disable LPM (hipm & dipm) ATA_LPM_MED_POWER: enable hipm partial ATA_LPM_MED_POWER_WITH_DIPM: enable hipm partial &dipm partial &slumber ATA_LPM_MIN_POWER_WITH_PARTIAL: enable hipm partial &dipm partial&slumber&devslp ATA_LPM_MIN_POWER: enable hipm slumber &dipm partial &slumber &devslp hipm: adpter initiated power mode transitions; dipm:*device* initiated power mode transitions; From my comprehension, user policy in [ATA_LPM_MED_POWER, ATA_LPM_MIN_POWER] need to be validated on adapter's capabilities (partial(y/n), slumber(y/n), devslp(y/n)), so, there exits the following cases: 1, (n, n, n), validate it to ATA_LPM_UNKNOWN; 2, (n, n, y), validate it to ATA_LPM_MIN_POWER_WITH_PARTIAL/ATA_LPM_MIN_POWER? 3, (n, y, n), validate it to ATA_LPM_MIN_POWER; 4, (n, y, y), validate it to ATA_LPM_MIN_POWER; 5, (y, n, n), validate it to ATA_LPM_MED_POWER; 6, (y, n, y), validate it to ATA_LPM_MIN_POWER_WITH_PARTIAL; 7, (y, y, n), default user policy; 8, (y, y, y), default user policy; ('y' for support, 'n' for not support) for case 2, I'm not quiet sure, for which may enable hipm partial/slumber. The way I provided above is quiet complicated and may be incomplete. It may not be realistic to take all into account, but I think case 1 should be taken seriously for which may cause the above PORT_IRQ_PHYRDY issue. Perhaps, I could refer to Mario's patches later (I have not found yet on kernel/git ^_^). >> ahci_update_initial_lpm_policy(ap, hpriv); >> >> According to my current understanding, the currently submitted patches can >> solve the above problems, and definitely not the best. I haven't figured out >> good way to use CAP flags to patch. Hope you can give me some advice, thanks. > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM capability 2022-05-07 7:36 ` Runa Guo-oc @ 2022-05-11 8:14 ` Damien Le Moal 2022-05-18 21:31 ` Limonciello, Mario 0 siblings, 1 reply; 13+ messages in thread From: Damien Le Moal @ 2022-05-11 8:14 UTC (permalink / raw) To: Runa Guo-oc, linux-ide, Mario Limonciello, Cobe Chen, Tim Guo, TonyW Wang, Leo Liu On 2022/05/07 9:36, Runa Guo-oc wrote: > On 2022/5/2 21:05, Damien Le Moal wrote: >> On 2022/04/27 19:18, Runa Guo-oc wrote: >>> On 2022/4/23 6:37, Damien Le Moal wrote: >>>> On 4/22/22 18:57, RunaGuo-oc wrote: >>>>> 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. >>>> Users should *never* need to be aware of the HW specs and what can or >>>> cannot be done with a particular adapter/drive. User actions trying to >>>> enable an unsupported feature should be failed, always. >>>> >>>> In your case though, quickly checking the AHCI specs, the scontrol >>>> register bits you change seem to be for preventing *device* initiated >>>> power mode transitions, not user/host initiated operations. Your commit >>>> message should clearly mention that. But I still need to spend more time >>>> re-reading the specs to confirm. Will do that next week. >>>> >>>> Since you added the CAP flags, these flags should be used to detect low >>>> power policies that can be allowed for user actions. >>>> >>>> Mario, >>>> >>>> Please rebase and repost your patches ! I rebased the for-5.19 branch on >>>> rc3 to have the LPM config name revert. We need to fix this LPM mess :) >>>> >>>>>> Mario has patches to fix that. >>>>> >>>>> Hmm. How to patch this case ? >>>> Mario's patches modify the beginning of the sata_link_scr_lpm() function >>>> to prevent setting an LPM policy that the adapter/drive does not support. >>>> This together with the correct bits set/reset in the scr register will >>>> only allow LPM transitions that are supported. >>>> >>>> It may also be good to revisit ata_scsi_lpm_store() to prevent the user >>>> from setting an unsupported policy. Currently, that uselessly triggers an >>>> EH sequence. >>> To avoid some confusion in this patch set, I want to explain it here. >>> The patch set involves two LPM related issues, one for the ahci adapter >>> does not support LPM (no partial & slumber & devslp), the other for >>> ahci adapter supports part of LPM(eg, only partial, no slumber & devslp). >>> >>> The former case is what I metioned in this mail thread. Namely, when LPM is >>> enabled, actions trying to enable this unsupported feature will be failed, >>> but will disable PORT_IRQ_PHYRDY bit at the beginning of the ahci_set_lpm() >>> function, which would make PhyRdy Changed cannot be detected. So I add flags >>> in the ata_eh_set_lpm() function which will not go to the disable operation. >>> >>> The latter case is what I metioned in "PATCH[2/2]". Namely, if the ahci >>> adapter only supports partial (no slumber & devslp), then when LPM is enabled >>> (eg, min_power), *device* initiated power mode transitions will be enabled >>> with the scontrol register bits setting to indicate no restrictions on LPM >>> transitions. After that, if SSD/HDD sends a DIPM slumber request, it cannot >>> be disallowed by ahci adpter for driver not setting scontrol register bits >>> properly. So I add flags to control it. >>> >>> Therefore, Mario's patches in the sata_link_scr_lpm() function may fix the >>> issue in "PATCH[2/2]". >>> >>> Revisit ata_scsi_lpm_store() to prevent the user from setting an unsupported >>> policy may be a way to fix the issue in "PATCH[1/2]", but there may be another >>> case where some operating system manufacturers setting LPM default enable in >>> driver, like the following code in the ahci_init_one() function, also need to >>> control. >>> >>> if (ap->flags & ATA_FLAG_EM) >>> ap->em_message_type = hpriv->em_msg_type; >>> >>> + ap->target_lpm_policy = ATA_LPM_MIN_POWER; >> This one looks wrong. This is set inside ahci_update_initial_lpm_policy() >> according to the default kernel config (CONFIG_SATA_MOBILE_LPM_POLICY) and >> module param + what the drive can do according to ACPI. The problem though is >> that the adapter capabilities are not checked in that function, so the initial >> target lpm policy may be wrong. >> >> Since your patch 1/2 adds the hpriv flags indicating the capabilities, you need >> to use these in ahci_update_initial_lpm_policy() to validate whatever initial >> policy is asked for by the user. > > Yes, the above code is not rigorous, existing methods provided by kernel as you > said should be used in this case. > > In order to use CAP flags to validate user policy, I review the latest kernel > LPM policies, here is my understanding: > ATA_LPM_UNKNOWN: default policy, no LPM > ATA_LPM_MAX_POWER: disable LPM (hipm & dipm) > ATA_LPM_MED_POWER: enable hipm partial > ATA_LPM_MED_POWER_WITH_DIPM: enable hipm partial &dipm partial &slumber > ATA_LPM_MIN_POWER_WITH_PARTIAL: enable hipm partial &dipm partial&slumber&devslp > ATA_LPM_MIN_POWER: enable hipm slumber &dipm partial &slumber &devslp > hipm: adpter initiated power mode transitions; > dipm:*device* initiated power mode transitions; > > From my comprehension, user policy in [ATA_LPM_MED_POWER, ATA_LPM_MIN_POWER] > need to be validated on adapter's capabilities (partial(y/n), slumber(y/n), > devslp(y/n)), so, there exits the following cases: Note that devslp is a device side feature too. See ata_dev_config_devslp() in libata-core.c. So even if the adapter supports devslp, if the drive does not, devslp should not be enabled on the port. > 1, (n, n, n), validate it to ATA_LPM_UNKNOWN; > 2, (n, n, y), validate it to ATA_LPM_MIN_POWER_WITH_PARTIAL/ATA_LPM_MIN_POWER? > 3, (n, y, n), validate it to ATA_LPM_MIN_POWER; > 4, (n, y, y), validate it to ATA_LPM_MIN_POWER; > 5, (y, n, n), validate it to ATA_LPM_MED_POWER; > 6, (y, n, y), validate it to ATA_LPM_MIN_POWER_WITH_PARTIAL; > 7, (y, y, n), default user policy; > 8, (y, y, y), default user policy; > ('y' for support, 'n' for not support) > > for case 2, I'm not quiet sure, for which may enable hipm partial/slumber. For all above cases, the default should be to use the default configured policy defined by SATA_MOBILE_LPM_POLICY or the ahci module parameter but corrected to match what the adapter & device support, including the eventual NO LPM horkage flag. Mario's patch started addressing that, but that patch can be improved using yours. > The way I provided above is quiet complicated and may be incomplete. > It may not be realistic to take all into account, but I think case 1 should > be taken seriously for which may cause the above PORT_IRQ_PHYRDY issue. yes. > Perhaps, I could refer to Mario's patches later (I have not found yet on > kernel/git ^_^). Mario's patch is here: https://lore.kernel.org/all/20220404194510.9206-2-mario.limonciello@amd.com/ Can you add that patch into your series with eventual modifications to better check the adapter's CAP bits ? Mario ? Are you OK with that ? -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM capability 2022-05-11 8:14 ` Damien Le Moal @ 2022-05-18 21:31 ` Limonciello, Mario 0 siblings, 0 replies; 13+ messages in thread From: Limonciello, Mario @ 2022-05-18 21:31 UTC (permalink / raw) To: Damien Le Moal, Runa Guo-oc, linux-ide, Cobe Chen, Tim Guo, TonyW Wang, Leo Liu [Public] > -----Original Message----- > From: Damien Le Moal <damien.lemoal@opensource.wdc.com> > Sent: Wednesday, May 11, 2022 03:15 > To: Runa Guo-oc <RunaGuo-oc@zhaoxin.com>; linux-ide@vger.kernel.org; > Limonciello, Mario <Mario.Limonciello@amd.com>; Cobe Chen > <CobeChen@zhaoxin.com>; Tim Guo <TimGuo@zhaoxin.com>; TonyW > Wang <TonyWWang@zhaoxin.com>; Leo Liu <LeoLiu@zhaoxin.com> > Subject: Re: [PATCH 1/2] ahci: Add PhyRdy Change control on actual LPM > capability > > On 2022/05/07 9:36, Runa Guo-oc wrote: > > On 2022/5/2 21:05, Damien Le Moal wrote: > >> On 2022/04/27 19:18, Runa Guo-oc wrote: > >>> On 2022/4/23 6:37, Damien Le Moal wrote: > >>>> On 4/22/22 18:57, RunaGuo-oc wrote: > >>>>> 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. > >>>> Users should *never* need to be aware of the HW specs and what can > or > >>>> cannot be done with a particular adapter/drive. User actions trying to > >>>> enable an unsupported feature should be failed, always. > >>>> > >>>> In your case though, quickly checking the AHCI specs, the scontrol > >>>> register bits you change seem to be for preventing *device* initiated > >>>> power mode transitions, not user/host initiated operations. Your > commit > >>>> message should clearly mention that. But I still need to spend more > time > >>>> re-reading the specs to confirm. Will do that next week. > >>>> > >>>> Since you added the CAP flags, these flags should be used to detect > low > >>>> power policies that can be allowed for user actions. > >>>> > >>>> Mario, > >>>> > >>>> Please rebase and repost your patches ! I rebased the for-5.19 branch > on > >>>> rc3 to have the LPM config name revert. We need to fix this LPM mess > :) > >>>> > >>>>>> Mario has patches to fix that. > >>>>> > >>>>> Hmm. How to patch this case ? > >>>> Mario's patches modify the beginning of the sata_link_scr_lpm() > function > >>>> to prevent setting an LPM policy that the adapter/drive does not > support. > >>>> This together with the correct bits set/reset in the scr register will > >>>> only allow LPM transitions that are supported. > >>>> > >>>> It may also be good to revisit ata_scsi_lpm_store() to prevent the user > >>>> from setting an unsupported policy. Currently, that uselessly triggers an > >>>> EH sequence. > >>> To avoid some confusion in this patch set, I want to explain it here. > >>> The patch set involves two LPM related issues, one for the ahci adapter > >>> does not support LPM (no partial & slumber & devslp), the other for > >>> ahci adapter supports part of LPM(eg, only partial, no slumber & devslp). > >>> > >>> The former case is what I metioned in this mail thread. Namely, when > LPM is > >>> enabled, actions trying to enable this unsupported feature will be failed, > >>> but will disable PORT_IRQ_PHYRDY bit at the beginning of the > ahci_set_lpm() > >>> function, which would make PhyRdy Changed cannot be detected. So I > add flags > >>> in the ata_eh_set_lpm() function which will not go to the disable > operation. > >>> > >>> The latter case is what I metioned in "PATCH[2/2]". Namely, if the ahci > >>> adapter only supports partial (no slumber & devslp), then when LPM is > enabled > >>> (eg, min_power), *device* initiated power mode transitions will be > enabled > >>> with the scontrol register bits setting to indicate no restrictions on LPM > >>> transitions. After that, if SSD/HDD sends a DIPM slumber request, it > cannot > >>> be disallowed by ahci adpter for driver not setting scontrol register bits > >>> properly. So I add flags to control it. > >>> > >>> Therefore, Mario's patches in the sata_link_scr_lpm() function may fix > the > >>> issue in "PATCH[2/2]". > >>> > >>> Revisit ata_scsi_lpm_store() to prevent the user from setting an > unsupported > >>> policy may be a way to fix the issue in "PATCH[1/2]", but there may be > another > >>> case where some operating system manufacturers setting LPM default > enable in > >>> driver, like the following code in the ahci_init_one() function, also need > to > >>> control. > >>> > >>> if (ap->flags & ATA_FLAG_EM) > >>> ap->em_message_type = hpriv->em_msg_type; > >>> > >>> + ap->target_lpm_policy = ATA_LPM_MIN_POWER; > >> This one looks wrong. This is set inside ahci_update_initial_lpm_policy() > >> according to the default kernel config > (CONFIG_SATA_MOBILE_LPM_POLICY) and > >> module param + what the drive can do according to ACPI. The problem > though is > >> that the adapter capabilities are not checked in that function, so the initial > >> target lpm policy may be wrong. > >> > >> Since your patch 1/2 adds the hpriv flags indicating the capabilities, you > need > >> to use these in ahci_update_initial_lpm_policy() to validate whatever > initial > >> policy is asked for by the user. > > > > Yes, the above code is not rigorous, existing methods provided by kernel as > you > > said should be used in this case. > > > > In order to use CAP flags to validate user policy, I review the latest kernel > > LPM policies, here is my understanding: > > ATA_LPM_UNKNOWN: default policy, no LPM > > ATA_LPM_MAX_POWER: disable LPM (hipm & dipm) > > ATA_LPM_MED_POWER: enable hipm partial > > ATA_LPM_MED_POWER_WITH_DIPM: enable hipm partial &dipm partial > &slumber > > ATA_LPM_MIN_POWER_WITH_PARTIAL: enable hipm partial &dipm > partial&slumber&devslp > > ATA_LPM_MIN_POWER: enable hipm slumber &dipm partial &slumber > &devslp > > hipm: adpter initiated power mode transitions; > > dipm:*device* initiated power mode transitions; > > > > From my comprehension, user policy in [ATA_LPM_MED_POWER, > ATA_LPM_MIN_POWER] > > need to be validated on adapter's capabilities (partial(y/n), slumber(y/n), > > devslp(y/n)), so, there exits the following cases: > > Note that devslp is a device side feature too. See ata_dev_config_devslp() in > libata-core.c. So even if the adapter supports devslp, if the drive does not, > devslp should not be enabled on the port. > > > 1, (n, n, n), validate it to ATA_LPM_UNKNOWN; > > 2, (n, n, y), validate it > to ATA_LPM_MIN_POWER_WITH_PARTIAL/ATA_LPM_MIN_POWER? > > 3, (n, y, n), validate it to ATA_LPM_MIN_POWER; > > 4, (n, y, y), validate it to ATA_LPM_MIN_POWER; > > 5, (y, n, n), validate it to ATA_LPM_MED_POWER; > > 6, (y, n, y), validate it to ATA_LPM_MIN_POWER_WITH_PARTIAL; > > 7, (y, y, n), default user policy; > > 8, (y, y, y), default user policy; > > ('y' for support, 'n' for not support) > > > > for case 2, I'm not quiet sure, for which may enable hipm partial/slumber. > > For all above cases, the default should be to use the default configured policy > defined by SATA_MOBILE_LPM_POLICY or the ahci module parameter but > corrected to > match what the adapter & device support, including the eventual NO LPM > horkage > flag. Mario's patch started addressing that, but that patch can be improved > using yours. > > > The way I provided above is quiet complicated and may be incomplete. > > It may not be realistic to take all into account, but I think case 1 should > > be taken seriously for which may cause the above PORT_IRQ_PHYRDY > issue. > > yes. > > > Perhaps, I could refer to Mario's patches later (I have not found yet on > > kernel/git ^_^). > > Mario's patch is here: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore. > kernel.org%2Fall%2F20220404194510.9206-2- > mario.limonciello%40amd.com%2F&data=05%7C01%7Cmario.limonciell > o%40amd.com%7C2ff34a9405444559699208da33264e52%7C3dd8961fe4884e > 608e11a82d994e183d%7C0%7C0%7C637878536864920264%7CUnknown%7CT > WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC > JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=izsUMCw%2BOzV7myxDH > JBqmWnzInVJ2CJz3wfsdgVmDdY%3D&reserved=0 > > Can you add that patch into your series with eventual modifications to better > check the adapter's CAP bits ? > > Mario ? Are you OK with that ? I think that's fine. I've obviously gotten side tracked and didn't get a chance to rebase and send them up. but I do want to bring to your attention that I just found out there was a regression from the original patch that set the policy to mobile. I was hoping to figure out whether it's just a stable artifact or if it's a real problem with mainline kernel before giving to much thought to setting ALL boards to this new policy. It might be a single outlier, or it might be a crystal ball - TBD. This regression happened because it got brought back to 5.4-stable, and as it turns out the exact same FCH controller ID from the client silicon is used in another product. It's an ASUS server system with AMD Epyc processor. The regression is specifically along hotplug, that hotplug no longer works with the more aggressive policies. The regression (including the bisect) is reported here: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1971576 There is active open tests to figure out whether the issue happens in mainline kernel as well or if it's just from 5.4 backport. My suggestion is that you pull my patches into your series and lets keep iterating Them, but let's hold off pulling into 5.19 until that's figured out, especially if we have to revert 380cd49e207ba4 to solve that problem then we shouldn't push this policy across all boards (unfortunately). > > > -- > Damien Le Moal > Western Digital Research ^ permalink raw reply [flat|nested] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ messages in thread
end of thread, other threads:[~2022-05-18 21:31 UTC | newest] Thread overview: 13+ 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-22 22:37 ` Damien Le Moal 2022-04-27 10:18 ` Runa Guo-oc 2022-05-02 13:05 ` Damien Le Moal 2022-05-07 7:36 ` Runa Guo-oc 2022-05-11 8:14 ` Damien Le Moal 2022-05-18 21:31 ` Limonciello, Mario 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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.