Re: [PATCH v7 20/26] PNP: Don't reserve BARs for PCI when enabled movable BARs

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jan 31, 2020 at 03:48:48PM +0000, Sergei Miroshnichenko wrote:
> Hello Bjorn,
> 
> On Thu, 2020-01-30 at 15:14 -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 29, 2020 at 06:29:31PM +0300, Sergei Miroshnichenko
> > wrote:
> > > When the Movable BARs feature is supported, the PCI subsystem is
> > > able to
> > > distribute existing BARs and allocate the new ones itself, without
> > > need to
> > > reserve gaps by BIOS.
> > > 
> > > CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
> > > ---
> > >  drivers/pnp/system.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/pnp/system.c b/drivers/pnp/system.c
> > > index 6950503741eb..16cd260a609d 100644
> > > --- a/drivers/pnp/system.c
> > > +++ b/drivers/pnp/system.c
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/device.h>
> > >  #include <linux/init.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/pci.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/ioport.h>
> > >  
> > > @@ -58,6 +59,11 @@ static void reserve_resources_of_dev(struct
> > > pnp_dev *dev)
> > >  	struct resource *res;
> > >  	int i;
> > >  
> > > +#ifdef CONFIG_PCI
> > > +	if (pci_can_move_bars)
> > > +		return;
> > > +#endif
> > 
> > I don't understand this.  The reason this function exists is so we
> > keep track of the resources consumed by PNP devices and we can keep
> > from assigning those resources to other things like PCI devices.
> > 
> > Admittedly we currently only do this for PNP0C01 and PNP0C02 devices,
> > but we really should do it for all PNP devices.
> > 
> > Why does Movable BARs mean that we no longer need this
> > information?  The whole point is that this information is needed
> > *during* PCI resource allocation, so I don't understand the idea
> > that "because the PCI subsystem is able to distribute existing
> > BARs and allocate the new ones itself", we don't need to know
> > about PNP resources to avoid.
> 
> Oh. I've made this patch in assumption that non-PCI PNP devices should
> not reside in the PCI address space, and PCI PNP devices behave like
> usual PCI devices - with BARs handled by the common PCI subsystem.

I don't think we can rely on that assumption.  I think all we should
assume is that address space described by _CRS of any PNP device is
unavailable for use by other devices (except for bridge windows, of
course).

> Do I understand correctly after digging a bit into drivers/pnp, that
> some of these resources are some kind of "invisible" BARs, which are
> used by drivers, but the PCI subsystem can't "see" them, so that's why
> the PNP reserves them?

ACPI/PNP _CRS is sort of a generalized BAR idea for devices that don't
have a native configuration protocol.  E.g., PCI has config space that
supports both enumeration and resource configuration (BARs).  There
may be other buses that have similar ideas and don't need PNP devices.

Generally, ACPI describes devices that can't be enumerated and
configured via native means.

> In this case I need just to discard this patch and to modify the
> pci_bus_release_root_bridge_resources() added in patch 06/26 - remove
> the pci_bus_for_each_resource(root_bus, r, i) block there, which
> releases such non-BAR resourses. I've just checked that it works, so
> the next version - v8 - of this patchset will be a bit lighter. Thank
> you for pointing that out!
> 
> Best regards,
> Serge
> 
> 
> > >  	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i));
> > > i++) {
> > >  		if (res->flags & IORESOURCE_DISABLED)
> > >  			continue;
> > > -- 
> > > 2.24.1
> > >