diff mbox series

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

Message ID 20200129152937.311162-21-s.miroshnichenko@yadro.com (mailing list archive)
State Superseded, archived
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Allow BAR movement during boot and hotplug | expand

Commit Message

Sergei Miroshnichenko Jan. 29, 2020, 3:29 p.m. UTC 
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(+)

Comments

Rafael J. Wysocki Jan. 30, 2020, 2:39 p.m. UTC | #1 
On Wednesday, January 29, 2020 4:29:31 PM CET 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

Would it be a problem to define pci_can_move_bars() as a static inline
returning 'false' when CONFIG_PCI is unset?

The #ifdef wouldn't be needed here then.

> +
>  	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); i++) {
>  		if (res->flags & IORESOURCE_DISABLED)
>  			continue;
>
Bjorn Helgaas Jan. 30, 2020, 9:14 p.m. UTC | #2 
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.

>  	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); i++) {
>  		if (res->flags & IORESOURCE_DISABLED)
>  			continue;
> -- 
> 2.24.1
>
Sergei Miroshnichenko Jan. 31, 2020, 3:48 p.m. UTC | #3 
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.

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?

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
> >
Bjorn Helgaas Jan. 31, 2020, 7:39 p.m. UTC | #4 
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
> > >
diff mbox series

Patch

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
+
 	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); i++) {
 		if (res->flags & IORESOURCE_DISABLED)
 			continue;