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@xxxxxxxxx> > > --- > > 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:
smime.p7s
Description: S/MIME cryptographic signature
