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

["Derrick, Jonathan" <jonathan.derrick@intel.com>]

[-- 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 --]