[PATCH v2] PCI: Configure MPS on rescan

[PATCH v2] PCI: Configure MPS on rescan

[Jon Derrick]

During pci_rescan_bus(), we may encounter new buses and devices which
don't have MPS set for compatibility. Using this path, newly discovered
buses and devices would then require their MPS to be configured after
driver attachment, which may be too late for drivers which do memory
transactions on probe.

This additionally ensures that any pcie_bus_config kernel settings will
be applied to the buses and devices discovered through this path prior
to driver attachment.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
v1: https://patchwork.kernel.org/patch/10642019/

 drivers/pci/probe.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b1c05b5054a0..126cd426b6f2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3156,6 +3156,34 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
 	return max;
 }
 
+/*
+ * Walks the PCI/PCIe tree to find the first instance of a PCIe device and
+ * hands off the PCIe bus to pcie_bus_configure_settings to walk the rest.
+ */
+static int pcie_rescan_bus_configure_settings(struct pci_dev *dev, void *data)
+{
+	if (pci_is_pcie(dev)) {
+		struct pci_bus *child, *bus = dev->bus;
+
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
+
+		return 1;
+	}
+	return 0;
+}
+
+/**
+ * pci_bus_configure_settings - Configure bus settings
+ * @bus: PCI/PCIE bus to configure
+ *
+ * Currently only configures PCIe bus settings related to MPS and MRRS.
+ */
+static void pci_bus_configure_settings(struct pci_bus *bus)
+{
+	pci_walk_bus(bus, pcie_rescan_bus_configure_settings, NULL);
+}
+
 /**
  * pci_rescan_bus - Scan a PCI bus for devices
  * @bus: PCI bus to scan
@@ -3171,6 +3199,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)
 
 	max = pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
+	pci_bus_configure_settings(bus);
 	pci_bus_add_devices(bus);
 
 	return max;
-- 
2.14.4

Re: [PATCH v2] PCI: Configure MPS on rescan

[Bjorn Helgaas]

On Tue, Nov 06, 2018 at 01:12:42PM -0700, Jon Derrick wrote:
> During pci_rescan_bus(), we may encounter new buses and devices which
> don't have MPS set for compatibility. Using this path, newly discovered
> buses and devices would then require their MPS to be configured after
> driver attachment, which may be too late for drivers which do memory
> transactions on probe.

This definitely looks like something we need to do.  Have you tripped
over an actual problem?  If so, it might be interesting to include a
symptom here, e.g., Unsupported Request error for hot-added device, or
whatever it is.

Can you clarify the "would then require their MPS to be configured"
part?  Is there some path where we *do* configure MPS after driver
attachment?  Or is this just a way of saying that "if we don't
configure MPS *before* driver attachment, we would have to do it
after"?

I'm thinking we could simply say something like:

  During pci_rescan_bus(), we may encounter new devices which haven't
  had MPS configured.  Their MPS must be configured before we make the
  devices available for driver attachment by calling
  pci_bus_add_devices().

> This additionally ensures that any pcie_bus_config kernel settings will
> be applied to the buses and devices discovered through this path prior
> to driver attachment.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
> v1: https://patchwork.kernel.org/patch/10642019/
> 
>  drivers/pci/probe.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b1c05b5054a0..126cd426b6f2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3156,6 +3156,34 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
>  	return max;
>  }
>  
> +/*
> + * Walks the PCI/PCIe tree to find the first instance of a PCIe device and
> + * hands off the PCIe bus to pcie_bus_configure_settings to walk the rest.
> + */
> +static int pcie_rescan_bus_configure_settings(struct pci_dev *dev, void *data)
> +{
> +	if (pci_is_pcie(dev)) {
> +		struct pci_bus *child, *bus = dev->bus;
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);

It looks possible that this could call pcie_bus_configure_settings()
a second time for a device that we've already configured.  For
example, it's legal to call pci_rescan_bus() on an arbitrary bus even
if there has been no hot-add event.

Is there something that prevents us from touching this
already-configured device?  *Probably* we would configure it the same
way the second time, but a driver is likely already attached to it,
and we shouldn't do anything to it.  Even if
pcie_bus_configure_settings() happens to be idempotent, that seems
like it would be hard to verify and keep true indefinitely.

Bjorn

> +
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * pci_bus_configure_settings - Configure bus settings
> + * @bus: PCI/PCIE bus to configure
> + *
> + * Currently only configures PCIe bus settings related to MPS and MRRS.
> + */
> +static void pci_bus_configure_settings(struct pci_bus *bus)
> +{
> +	pci_walk_bus(bus, pcie_rescan_bus_configure_settings, NULL);
> +}
> +
>  /**
>   * pci_rescan_bus - Scan a PCI bus for devices
>   * @bus: PCI bus to scan
> @@ -3171,6 +3199,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)
>  
>  	max = pci_scan_child_bus(bus);
>  	pci_assign_unassigned_bus_resources(bus);
> +	pci_bus_configure_settings(bus);
>  	pci_bus_add_devices(bus);
>  
>  	return max;
> -- 
> 2.14.4
> 

Re: [PATCH v2] PCI: Configure MPS on rescan

[Derrick, Jonathan]

[-- Attachment #1: Type: text/plain, Size: 4387 bytes --]

Hi Bjorn,


On Fri, 2019-02-01 at 16:54 -0600, Bjorn Helgaas wrote:
> On Tue, Nov 06, 2018 at 01:12:42PM -0700, Jon Derrick wrote:
> > During pci_rescan_bus(), we may encounter new buses and devices
> > which
> > don't have MPS set for compatibility. Using this path, newly
> > discovered
> > buses and devices would then require their MPS to be configured
> > after
> > driver attachment, which may be too late for drivers which do
> > memory
> > transactions on probe.
> 
> This definitely looks like something we need to do.  Have you tripped
> over an actual problem?  If so, it might be interesting to include a
> symptom here, e.g., Unsupported Request error for hot-added device,
> or
> whatever it is.
For some history I only ever hit this with vmd. There are a limited
number of callers of pci_rescan_bus() which should probably be fixed
individually if they have the same issue.

Otherwise the normal hotplug path configures it via
pciehp_configure_device()::pcie_bus_configure_settings().
The vmd case was resolved by the open coding patch Lorenzo pulled in
for v5.1.

After looking at the options I became pretty reluctant to do it in
pci_rescan_bus because it's triggered through sysfs and would modify
all MPS settings on the (live) bus. It seems like it would be harmless
because it *shouldn't* change any settings in its current code state. A
hypothetical case where I'd be concerned this wouldn't work: a device
is added that is MPS incompatible and is disabled. Then a new rescan
with this patch might change the MPS settings on the parent to make the
link compatible, but the MPS change makes the parent->parent link
incompatible.

> 
> Can you clarify the "would then require their MPS to be configured"
> part?  Is there some path where we *do* configure MPS after driver
> attachment?  Or is this just a way of saying that "if we don't
> configure MPS *before* driver attachment, we would have to do it
> after"?
Nowhere at this moment. Poor wording on my part.

> 
> I'm thinking we could simply say something like:
> 
>   During pci_rescan_bus(), we may encounter new devices which haven't
>   had MPS configured.  Their MPS must be configured before we make
> the
>   devices available for driver attachment by calling
>   pci_bus_add_devices().
> 
> > This additionally ensures that any pcie_bus_config kernel settings
> > will
> > be applied to the buses and devices discovered through this path
> > prior
> > to driver attachment.
> > 
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> > v1: https://patchwork.kernel.org/patch/10642019/
> > 
> >  drivers/pci/probe.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index b1c05b5054a0..126cd426b6f2 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -3156,6 +3156,34 @@ unsigned int
> > pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
> >  	return max;
> >  }
> >  
> > +/*
> > + * Walks the PCI/PCIe tree to find the first instance of a PCIe
> > device and
> > + * hands off the PCIe bus to pcie_bus_configure_settings to walk
> > the rest.
> > + */
> > +static int pcie_rescan_bus_configure_settings(struct pci_dev *dev,
> > void *data)
> > +{
> > +	if (pci_is_pcie(dev)) {
> > +		struct pci_bus *child, *bus = dev->bus;
> > +
> > +		list_for_each_entry(child, &bus->children, node)
> > +			pcie_bus_configure_settings(child);
> 
> It looks possible that this could call pcie_bus_configure_settings()
> a second time for a device that we've already configured.  For
> example, it's legal to call pci_rescan_bus() on an arbitrary bus even
> if there has been no hot-add event.
> 
> Is there something that prevents us from touching this
> already-configured device?  *Probably* we would configure it the same
> way the second time, but a driver is likely already attached to it,
> and we shouldn't do anything to it.  Even if
> pcie_bus_configure_settings() happens to be idempotent, that seems
> like it would be hard to verify and keep true indefinitely.
> 
I agree with you. There isn't anything preventing the configuration on
the live bus, but as mentioned above it *shouldn't* change the
setting... but I'm not 100% sure.

I'm fine dropping this patch

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]