| Message ID | 20190311133122.11417-10-s.miroshnichenko@yadro.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Allow BAR movement during hotplug | expand |
On Mon, Mar 11, 2019 at 04:31:10PM +0300, Sergey Miroshnichenko wrote: > If a PCIe device driver doesn't yet have support for movable BARs, > mark device's BARs with IORESOURCE_PCI_FIXED. I'm hesitant about using IORESOURCE_PCI_FIXED for this purpose. That was originally added to describe resources that can not be changed because they're hardwired in the device, e.g., legacy resources and Enhanced Allocation resources. In general, I think the bits in res->flags should tell us things about the hardware. This particular use would be something about the *driver*, and I think we should figure that out by looking at dev->driver. > Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> > --- > drivers/pci/probe.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index dc935f82a595..1cf6ec960236 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -3262,6 +3262,21 @@ static void pci_bus_rescan_prepare(struct pci_bus *bus) > } else if (dev->driver && > dev->driver->rescan_prepare) { > dev->driver->rescan_prepare(dev); > + } else if (dev->driver || ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA)) { > + int i; > + > + for (i = 0; i < PCI_NUM_RESOURCES; i++) { > + struct resource *r = &dev->resource[i]; > + > + if (!r->flags || !r->parent || > + (r->flags & IORESOURCE_UNSET) || > + (r->flags & IORESOURCE_PCI_FIXED)) > + continue; > + > + r->flags |= IORESOURCE_PCI_FIXED; > + pci_warn(dev, "%s: no support for movable BARs, mark BAR %d (%pR) as fixed\n", > + __func__, i, r); > + } > } > } > } > -- > 2.20.1 >
From: Bjorn Helgaas > Sent: 26 March 2019 20:29 > > On Mon, Mar 11, 2019 at 04:31:10PM +0300, Sergey Miroshnichenko wrote: > > If a PCIe device driver doesn't yet have support for movable BARs, > > mark device's BARs with IORESOURCE_PCI_FIXED. > > I'm hesitant about using IORESOURCE_PCI_FIXED for this purpose. That > was originally added to describe resources that can not be changed > because they're hardwired in the device, e.g., legacy resources and > Enhanced Allocation resources. > > In general, I think the bits in res->flags should tell us things about > the hardware. This particular use would be something about the > *driver*, and I think we should figure that out by looking at > dev->driver. There will also be drivers that don't support BARs being moved, but may be in a state (ie not actually open) where they can go through a remove-rescan sequence to allow the BAR be moved. This might even be true if the open count is non-zero. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 3/27/19 8:03 PM, David Laight wrote: > From: Bjorn Helgaas >> Sent: 26 March 2019 20:29 >> >> On Mon, Mar 11, 2019 at 04:31:10PM +0300, Sergey Miroshnichenko wrote: >>> If a PCIe device driver doesn't yet have support for movable BARs, >>> mark device's BARs with IORESOURCE_PCI_FIXED. >> >> I'm hesitant about using IORESOURCE_PCI_FIXED for this purpose. That >> was originally added to describe resources that can not be changed >> because they're hardwired in the device, e.g., legacy resources and >> Enhanced Allocation resources. >> >> In general, I think the bits in res->flags should tell us things about >> the hardware. This particular use would be something about the >> *driver*, and I think we should figure that out by looking at >> dev->driver. > > There will also be drivers that don't support BARs being moved, > but may be in a state (ie not actually open) where they can go > through a remove-rescan sequence to allow the BAR be moved. > > This might even be true if the open count is non-zero. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > This approach with IORESOURCE_PCI_FIXED was used because struct resource doesn't have a pointer to its device (and so to its driver). But now, after you have mentioned that, I can see that in every place I use the FIXED flag to mark the immovable resources - also has the according struct pci_dev *dev nearby. So, replacing every if (r->flags & IORESOURCE_PCI_FIXED) with if (!dev->driver->rescan_prepare) or something like if (pci_dev_movable_bars_capable(dev)) will reduce this huge patchset a little, and also makes irrelevant the case I've completely forgotten about - IORESOURCE_PCI_FIXED must be unset on removing (rmmod) the "immovable" driver. Thanks a lot! I'll rework the changes in this way and resend it as v5. Serge
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index dc935f82a595..1cf6ec960236 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -3262,6 +3262,21 @@ static void pci_bus_rescan_prepare(struct pci_bus *bus) } else if (dev->driver && dev->driver->rescan_prepare) { dev->driver->rescan_prepare(dev); + } else if (dev->driver || ((dev->class >> 8) == PCI_CLASS_DISPLAY_VGA)) { + int i; + + for (i = 0; i < PCI_NUM_RESOURCES; i++) { + struct resource *r = &dev->resource[i]; + + if (!r->flags || !r->parent || + (r->flags & IORESOURCE_UNSET) || + (r->flags & IORESOURCE_PCI_FIXED)) + continue; + + r->flags |= IORESOURCE_PCI_FIXED; + pci_warn(dev, "%s: no support for movable BARs, mark BAR %d (%pR) as fixed\n", + __func__, i, r); + } } } }
If a PCIe device driver doesn't yet have support for movable BARs, mark device's BARs with IORESOURCE_PCI_FIXED. Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> --- drivers/pci/probe.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)