| Message ID | 8a76b4a524d3915d42e894da7f4dbdec6c104512.1595649348.git.sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Simplify PCIe native ownership detection logic | expand |
On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > If CONFIG_PCIEPORTBUS is not enabled in kernel then initialing > struct pci_host_bridge PCIe specific native_* members to "1" is > incorrect. So protect the PCIe specific member initialization > with CONFIG_PCIEPORTBUS. s/initialing/initializing/ > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > drivers/pci/probe.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 2f66988cea25..a94b97564ceb 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -588,12 +588,14 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge) > * may implement its own AER handling and use _OSC to prevent the > * OS from interfering. > */ > +#ifdef CONFIG_PCIEPORTBUS > bridge->native_aer = 1; > bridge->native_pcie_hotplug = 1; > - bridge->native_shpc_hotplug = 1; > bridge->native_pme = 1; > bridge->native_ltr = 1; native_ltr isn't dependent on PCIEPORTBUS either, is it? It's only used for ASPM. > bridge->native_dpc = 1; > +#endif > + bridge->native_shpc_hotplug = 1; > > device_initialize(&bridge->dev); > } > -- > 2.17.1 >
On 9/10/20 12:49 PM, Bjorn Helgaas wrote: > On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> >> If CONFIG_PCIEPORTBUS is not enabled in kernel then initialing >> struct pci_host_bridge PCIe specific native_* members to "1" is >> incorrect. So protect the PCIe specific member initialization >> with CONFIG_PCIEPORTBUS. > > s/initialing/initializing/ will fix it in next version. > >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> drivers/pci/probe.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 2f66988cea25..a94b97564ceb 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -588,12 +588,14 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge) >> * may implement its own AER handling and use _OSC to prevent the >> * OS from interfering. >> */ >> +#ifdef CONFIG_PCIEPORTBUS >> bridge->native_aer = 1; >> bridge->native_pcie_hotplug = 1; >> - bridge->native_shpc_hotplug = 1; >> bridge->native_pme = 1; >> bridge->native_ltr = 1; > > native_ltr isn't dependent on PCIEPORTBUS either, is it? It's only > used for ASPM. Agreed. I was confused due to a comment in include/linux/pci.h unsigned int native_ltr:1; /* OS may use PCIe LTR */ > >> bridge->native_dpc = 1; >> +#endif >> + bridge->native_shpc_hotplug = 1; >> >> device_initialize(&bridge->dev); >> } >> -- >> 2.17.1 >>
On 9/10/20 2:00 PM, Kuppuswamy, Sathyanarayanan wrote: > > > On 9/10/20 12:49 PM, Bjorn Helgaas wrote: >> On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: >>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >>> >>> If CONFIG_PCIEPORTBUS is not enabled in kernel then initialing >>> struct pci_host_bridge PCIe specific native_* members to "1" is >>> incorrect. So protect the PCIe specific member initialization >>> with CONFIG_PCIEPORTBUS. >> >> s/initialing/initializing/ > will fix it in next version. >> >>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >>> --- >>> drivers/pci/probe.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>> index 2f66988cea25..a94b97564ceb 100644 >>> --- a/drivers/pci/probe.c >>> +++ b/drivers/pci/probe.c >>> @@ -588,12 +588,14 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge) >>> * may implement its own AER handling and use _OSC to prevent the >>> * OS from interfering. >>> */ >>> +#ifdef CONFIG_PCIEPORTBUS >>> bridge->native_aer = 1; >>> bridge->native_pcie_hotplug = 1; >>> - bridge->native_shpc_hotplug = 1; >>> bridge->native_pme = 1; >>> bridge->native_ltr = 1; >> >> native_ltr isn't dependent on PCIEPORTBUS either, is it? It's only >> used for ASPM. > Agreed. I was confused due to a comment in include/linux/pci.h > > unsigned int native_ltr:1; /* OS may use PCIe LTR */ Currently there is no code dependency between LTR and CONFIG_PCIEPORTBUS. But I am wondering whether its correct to move LTR code under CONFIG_PCIEPORTBUS?. As per PCIe spec v5.0 sec 7.8.2, LTR is a optional PCIe extended capability. So why is not moved under drivers/pci/pcie/*. What is the criteria for code to be placed under drivers/pci/pcie/* > >> >>> bridge->native_dpc = 1; >>> +#endif >>> + bridge->native_shpc_hotplug = 1; >>> device_initialize(&bridge->dev); >>> } >>> -- >>> 2.17.1 >>> >
On Sun, Sep 13, 2020 at 01:49:26PM -0700, Kuppuswamy, Sathyanarayanan wrote: > On 9/10/20 2:00 PM, Kuppuswamy, Sathyanarayanan wrote: > > On 9/10/20 12:49 PM, Bjorn Helgaas wrote: > > > On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > > > > If CONFIG_PCIEPORTBUS is not enabled in kernel then initialing > > > > struct pci_host_bridge PCIe specific native_* members to "1" is > > > > incorrect. So protect the PCIe specific member initialization > > > > with CONFIG_PCIEPORTBUS. > > > > > > s/initialing/initializing/ > > will fix it in next version. > > > > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > --- > > > > drivers/pci/probe.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > > index 2f66988cea25..a94b97564ceb 100644 > > > > --- a/drivers/pci/probe.c > > > > +++ b/drivers/pci/probe.c > > > > @@ -588,12 +588,14 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge) > > > > * may implement its own AER handling and use _OSC to prevent the > > > > * OS from interfering. > > > > */ > > > > +#ifdef CONFIG_PCIEPORTBUS > > > > bridge->native_aer = 1; > > > > bridge->native_pcie_hotplug = 1; > > > > - bridge->native_shpc_hotplug = 1; > > > > bridge->native_pme = 1; > > > > bridge->native_ltr = 1; > > > > > > native_ltr isn't dependent on PCIEPORTBUS either, is it? It's only > > > used for ASPM. > > Agreed. I was confused due to a comment in include/linux/pci.h > > > > unsigned int native_ltr:1; /* OS may use PCIe LTR */ > Currently there is no code dependency between LTR and CONFIG_PCIEPORTBUS. > > But I am wondering whether its correct to move LTR code under > CONFIG_PCIEPORTBUS?. As per PCIe spec v5.0 sec 7.8.2, LTR is a > optional PCIe extended capability. So why is not moved under > drivers/pci/pcie/*. What is the criteria for code to be placed under > drivers/pci/pcie/* Some folks think drivers/pci/pcie/ should not exist, and I tend to agree, but it's a fair bit of churn to remove it. We do have quite a bit of PCIe extended capability support in drivers/pci -- ats.c, iov.c, vc.c. There's no need to move LTR under CONFIG_PCIEPORTBUS because CONFIG_PCIEPORTBUS enables portdrv, and AFAIK there's nothing LTR-related that relies on portdrv. The stuff currently in drivers/pci/pcie is mostly just portdrv and services that depend on it. aspm.c and ptm.c are exceptions and really should be in drivers/pci. > > > > bridge->native_dpc = 1; > > > > +#endif > > > > + bridge->native_shpc_hotplug = 1; > > > > device_initialize(&bridge->dev); > > > > } > > > > -- > > > > 2.17.1 > > > > > > > > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer
On 9/15/20 3:17 PM, Bjorn Helgaas wrote: > On Sun, Sep 13, 2020 at 01:49:26PM -0700, Kuppuswamy, Sathyanarayanan wrote: >> On 9/10/20 2:00 PM, Kuppuswamy, Sathyanarayanan wrote: >>> On 9/10/20 12:49 PM, Bjorn Helgaas wrote: >>>> On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: >>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >>>>> >> >> But I am wondering whether its correct to move LTR code under >> CONFIG_PCIEPORTBUS?. As per PCIe spec v5.0 sec 7.8.2, LTR is a >> optional PCIe extended capability. So why is not moved under >> drivers/pci/pcie/*. What is the criteria for code to be placed under >> drivers/pci/pcie/* > > Some folks think drivers/pci/pcie/ should not exist, and I tend to > agree, but it's a fair bit of churn to remove it. We do have quite a > bit of PCIe extended capability support in drivers/pci -- ats.c, > iov.c, vc.c. > > There's no need to move LTR under CONFIG_PCIEPORTBUS because > CONFIG_PCIEPORTBUS enables portdrv, and AFAIK there's nothing > LTR-related that relies on portdrv. > > The stuff currently in drivers/pci/pcie is mostly just portdrv and > services that depend on it. aspm.c and ptm.c are exceptions and > really should be in drivers/pci. Thanks for the clarification. I will remove the CONFIG_PCIEPORTBUS dependency. > >>>>> -- >>>>> 2.17.1 >>>>> >>> >> >> -- >> Sathyanarayanan Kuppuswamy >> Linux Kernel Developer
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 2f66988cea25..a94b97564ceb 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -588,12 +588,14 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge) * may implement its own AER handling and use _OSC to prevent the * OS from interfering. */ +#ifdef CONFIG_PCIEPORTBUS bridge->native_aer = 1; bridge->native_pcie_hotplug = 1; - bridge->native_shpc_hotplug = 1; bridge->native_pme = 1; bridge->native_ltr = 1; bridge->native_dpc = 1; +#endif + bridge->native_shpc_hotplug = 1; device_initialize(&bridge->dev); }