| Message ID | 20220902145835.344302-2-helgaas@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PCI/PM: Always disable PTM for all devices during | expand |
On 9/2/22 7:58 AM, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > When disabling PTM, there's no need to clear the Root Select bit. We > disable PTM during suspend, and we want to re-enable it during resume. > Clearing Root Select here makes re-enabling more complicated. Currently, it looks like we disable PCI_PTM_CTRL_ROOT in pci_disable_ptm(), but not enable it in pci_enable_ptm(). Do you know this did not trigger an issue? Also, you mentioned that it is complicated to enable it, can you add some details? > > Per PCIe r6.0, sec 7.9.15.3, "When set, if the PTM Enable bit is also Set, > this Time Source is the PTM Root," so if PTM Enable is cleared, the value > of Root Select should be irrelevant. > > Preserve Root Select to simplify re-enabling PTM. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: David E. Box <david.e.box@linux.intel.com> > --- > drivers/pci/pcie/ptm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c > index 368a254e3124..b6a417247ce3 100644 > --- a/drivers/pci/pcie/ptm.c > +++ b/drivers/pci/pcie/ptm.c > @@ -42,7 +42,7 @@ void pci_disable_ptm(struct pci_dev *dev) > return; > > pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl); > - ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT); > + ctrl &= ~PCI_PTM_CTRL_ENABLE; > pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl); > } >
On Fri, Sep 02, 2022 at 10:24:05AM -0700, Sathyanarayanan Kuppuswamy wrote: > On 9/2/22 7:58 AM, Bjorn Helgaas wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > When disabling PTM, there's no need to clear the Root Select bit. We > > disable PTM during suspend, and we want to re-enable it during resume. > > Clearing Root Select here makes re-enabling more complicated. > > Currently, it looks like we disable PCI_PTM_CTRL_ROOT in pci_disable_ptm(), > but not enable it in pci_enable_ptm(). Do you know this did not trigger an > issue? For Root Ports and Switches, we enable PTM (and set Root Select when appropriate) during enumeration in pci_ptm_init(). This is based on the assumption that enabling PTM in Root Ports and Switches is a no-op unless there's an Endpoint that generates PTM Requests. (It turns out that's not quite true, because Kai-Heng's bug report [1] shows the 08:00.0 Switch sending PTM Requests even though no Endpoint even has a PTM Capability.) If we didn't enable PTM in Root Ports and Switches during enumeration, we'd have to walk the whole path and enable them when enabling PTM for an Endpoint. pci_enable_ptm() currently only works for Endpoints, which cannot be PTM Roots, so it never has to set PCI_PTM_CTRL_ROOT. If we clear PCI_PTM_CTRL_ROOT in pci_disable_ptm(), it will never get set again unless we re-enumerate the Root Port. Thanks for asking this, because it reminds me why I didn't add pci_enable_ptm() calls in the resume paths! That would make them parallel with the suspend paths, which would definitely be nice. But we would have to rework pci_enable_ptm() to work for Root Ports and Switch Ports as well. I think we *could* do that. What do you think? Regardless of that question, I think it's unnecessary to clear PCI_PTM_CTRL_ROOT in pci_disable_ptm(), so we should leave it alone. [1] https://bugzilla.kernel.org/show_bug.cgi?id=215453 > Also, you mentioned that it is complicated to enable it, can you add some > details? > > > Per PCIe r6.0, sec 7.9.15.3, "When set, if the PTM Enable bit is also Set, > > this Time Source is the PTM Root," so if PTM Enable is cleared, the value > > of Root Select should be irrelevant. > > > > Preserve Root Select to simplify re-enabling PTM. > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > Cc: David E. Box <david.e.box@linux.intel.com> > > --- > > drivers/pci/pcie/ptm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c > > index 368a254e3124..b6a417247ce3 100644 > > --- a/drivers/pci/pcie/ptm.c > > +++ b/drivers/pci/pcie/ptm.c > > @@ -42,7 +42,7 @@ void pci_disable_ptm(struct pci_dev *dev) > > return; > > > > pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl); > > - ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT); > > + ctrl &= ~PCI_PTM_CTRL_ENABLE; > > pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl); > > } > > > > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer
Hi Bjorn, On 9/2/22 1:38 PM, Bjorn Helgaas wrote: > On Fri, Sep 02, 2022 at 10:24:05AM -0700, Sathyanarayanan Kuppuswamy wrote: >> On 9/2/22 7:58 AM, Bjorn Helgaas wrote: >>> From: Bjorn Helgaas <bhelgaas@google.com> >>> >>> When disabling PTM, there's no need to clear the Root Select bit. We >>> disable PTM during suspend, and we want to re-enable it during resume. >>> Clearing Root Select here makes re-enabling more complicated. >> >> Currently, it looks like we disable PCI_PTM_CTRL_ROOT in pci_disable_ptm(), >> but not enable it in pci_enable_ptm(). Do you know this did not trigger an >> issue? > > For Root Ports and Switches, we enable PTM (and set Root Select when > appropriate) during enumeration in pci_ptm_init(). This is based on > the assumption that enabling PTM in Root Ports and Switches is a no-op > unless there's an Endpoint that generates PTM Requests. (It turns out > that's not quite true, because Kai-Heng's bug report [1] shows the > 08:00.0 Switch sending PTM Requests even though no Endpoint even has a > PTM Capability.) > > If we didn't enable PTM in Root Ports and Switches during enumeration, > we'd have to walk the whole path and enable them when enabling PTM for > an Endpoint. > > pci_enable_ptm() currently only works for Endpoints, which cannot be > PTM Roots, so it never has to set PCI_PTM_CTRL_ROOT. > > If we clear PCI_PTM_CTRL_ROOT in pci_disable_ptm(), it will never get > set again unless we re-enumerate the Root Port. Thanks for clarifying. > > Thanks for asking this, because it reminds me why I didn't add > pci_enable_ptm() calls in the resume paths! That would make them > parallel with the suspend paths, which would definitely be nice. But > we would have to rework pci_enable_ptm() to work for Root Ports and > Switch Ports as well. I think we *could* do that. What do you think? IMO, the code will look better if we keep the suspend and resume paths in sync. Since we are calling pci_disable_ptm() in suspend path, it makes sense to call pci_enable_ptm() in resume path. Making the pci_enable_ptm() handle root and upstream ports should not be very complicated, right? > > Regardless of that question, I think it's unnecessary to clear > PCI_PTM_CTRL_ROOT in pci_disable_ptm(), so we should leave it alone. I agree with you. We should not touch PCI_PTM_CTRL_ROOT in pci_disable_ptm(). > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=215453 > >> Also, you mentioned that it is complicated to enable it, can you add some >> details? >> >>> Per PCIe r6.0, sec 7.9.15.3, "When set, if the PTM Enable bit is also Set, >>> this Time Source is the PTM Root," so if PTM Enable is cleared, the value >>> of Root Select should be irrelevant. >>> >>> Preserve Root Select to simplify re-enabling PTM. >>> >>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >>> Cc: David E. Box <david.e.box@linux.intel.com> >>> --- >>> drivers/pci/pcie/ptm.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c >>> index 368a254e3124..b6a417247ce3 100644 >>> --- a/drivers/pci/pcie/ptm.c >>> +++ b/drivers/pci/pcie/ptm.c >>> @@ -42,7 +42,7 @@ void pci_disable_ptm(struct pci_dev *dev) >>> return; >>> >>> pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl); >>> - ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT); >>> + ctrl &= ~PCI_PTM_CTRL_ENABLE; >>> pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl); >>> } >>> >> >> -- >> Sathyanarayanan Kuppuswamy >> Linux Kernel Developer
On Fri, Sep 02, 2022 at 02:11:12PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 9/2/22 1:38 PM, Bjorn Helgaas wrote: > > On Fri, Sep 02, 2022 at 10:24:05AM -0700, Sathyanarayanan Kuppuswamy wrote: > >> On 9/2/22 7:58 AM, Bjorn Helgaas wrote: > >>> From: Bjorn Helgaas <bhelgaas@google.com> > >>> > >>> When disabling PTM, there's no need to clear the Root Select bit. We > >>> disable PTM during suspend, and we want to re-enable it during resume. > >>> Clearing Root Select here makes re-enabling more complicated. > >> > >> Currently, it looks like we disable PCI_PTM_CTRL_ROOT in pci_disable_ptm(), > >> but not enable it in pci_enable_ptm(). Do you know this did not trigger an > >> issue? > ... > > Thanks for asking this, because it reminds me why I didn't add > > pci_enable_ptm() calls in the resume paths! That would make them > > parallel with the suspend paths, which would definitely be nice. But > > we would have to rework pci_enable_ptm() to work for Root Ports and > > Switch Ports as well. I think we *could* do that. What do you think? > > IMO, the code will look better if we keep the suspend and resume paths in > sync. Since we are calling pci_disable_ptm() in suspend path, it makes > sense to call pci_enable_ptm() in resume path. > > Making the pci_enable_ptm() handle root and upstream ports should not > be very complicated, right? I took a stab at it. pci_enable_ptm() is getting kind of ugly, but maybe it's better overall. I'll post it and you can see what you think.
diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c index 368a254e3124..b6a417247ce3 100644 --- a/drivers/pci/pcie/ptm.c +++ b/drivers/pci/pcie/ptm.c @@ -42,7 +42,7 @@ void pci_disable_ptm(struct pci_dev *dev) return; pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl); - ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT); + ctrl &= ~PCI_PTM_CTRL_ENABLE; pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl); }