2009-06-22 14:08:22

by Alex Chiang

[permalink] [raw]
Subject: [PATCH] PCI: remove pcibios_scan_all_fns()

This was #define'd as 0 on all platforms, so let's get rid of it.

This change makes pci_scan_slot() slightly easier to read.

Cc: Ivan Kokshaysky <[email protected]>
Cc: Russell King <[email protected]>
Cc: Yoshinori Sato <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: David Howells <[email protected]>
Cc: Kyle McMartin <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mundt <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jeff Dike <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Signed-off-by: Alex Chiang <[email protected]>
---

arch/alpha/include/asm/pci.h | 1 -
arch/arm/include/asm/pci.h | 2 --
arch/h8300/include/asm/pci.h | 1 -
arch/ia64/include/asm/pci.h | 3 ---
arch/mips/include/asm/pci.h | 2 --
arch/mn10300/include/asm/pci.h | 2 --
arch/parisc/include/asm/pci.h | 1 -
arch/powerpc/include/asm/pci.h | 1 -
arch/sh/include/asm/pci.h | 1 -
arch/sparc/include/asm/pci_32.h | 1 -
arch/sparc/include/asm/pci_64.h | 1 -
arch/um/include/asm/pci.h | 1 -
arch/x86/include/asm/pci.h | 1 -
drivers/pci/probe.c | 3 +--
include/asm-generic/pci.h | 2 --
15 files changed, 1 insertions(+), 22 deletions(-)

diff --git a/arch/alpha/include/asm/pci.h b/arch/alpha/include/asm/pci.h
index d22ace9..dd8dcab 100644
--- a/arch/alpha/include/asm/pci.h
+++ b/arch/alpha/include/asm/pci.h
@@ -52,7 +52,6 @@ struct pci_controller {
bus numbers. */

#define pcibios_assign_all_busses() 1
-#define pcibios_scan_all_fns(a, b) 0

#define PCIBIOS_MIN_IO alpha_mv.min_io_address
#define PCIBIOS_MIN_MEM alpha_mv.min_mem_address
diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
index 0abf386..226cddd 100644
--- a/arch/arm/include/asm/pci.h
+++ b/arch/arm/include/asm/pci.h
@@ -6,8 +6,6 @@

#include <mach/hardware.h> /* for PCIBIOS_MIN_* */

-#define pcibios_scan_all_fns(a, b) 0
-
#ifdef CONFIG_PCI_HOST_ITE8152
/* ITE bridge requires setting latency timer to avoid early bus access
termination by PIC bus mater devices
diff --git a/arch/h8300/include/asm/pci.h b/arch/h8300/include/asm/pci.h
index 97389b3..cc97620 100644
--- a/arch/h8300/include/asm/pci.h
+++ b/arch/h8300/include/asm/pci.h
@@ -8,7 +8,6 @@
*/

#define pcibios_assign_all_busses() 0
-#define pcibios_scan_all_fns(a, b) 0

static inline void pcibios_set_master(struct pci_dev *dev)
{
diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h
index fcfca56..ceb1db1 100644
--- a/arch/ia64/include/asm/pci.h
+++ b/arch/ia64/include/asm/pci.h
@@ -17,7 +17,6 @@
* loader.
*/
#define pcibios_assign_all_busses() 0
-#define pcibios_scan_all_fns(a, b) 0

#define PCIBIOS_MIN_IO 0x1000
#define PCIBIOS_MIN_MEM 0x10000000
@@ -135,8 +134,6 @@ extern void pcibios_resource_to_bus(struct pci_dev *dev,
extern void pcibios_bus_to_resource(struct pci_dev *dev,
struct resource *res, struct pci_bus_region *region);

-#define pcibios_scan_all_fns(a, b) 0
-
#define HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
{
diff --git a/arch/mips/include/asm/pci.h b/arch/mips/include/asm/pci.h
index a68d111..5ebf825 100644
--- a/arch/mips/include/asm/pci.h
+++ b/arch/mips/include/asm/pci.h
@@ -65,8 +65,6 @@ extern int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin);

extern unsigned int pcibios_assign_all_busses(void);

-#define pcibios_scan_all_fns(a, b) 0
-
extern unsigned long PCIBIOS_MIN_IO;
extern unsigned long PCIBIOS_MIN_MEM;

diff --git a/arch/mn10300/include/asm/pci.h b/arch/mn10300/include/asm/pci.h
index e58b9a4..c001149 100644
--- a/arch/mn10300/include/asm/pci.h
+++ b/arch/mn10300/include/asm/pci.h
@@ -106,8 +106,6 @@ extern void pcibios_bus_to_resource(struct pci_dev *dev,
struct resource *res,
struct pci_bus_region *region);

-#define pcibios_scan_all_fns(a, b) 0
-
static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
{
return channel ? 15 : 14;
diff --git a/arch/parisc/include/asm/pci.h b/arch/parisc/include/asm/pci.h
index 7d842d6..64c7aa5 100644
--- a/arch/parisc/include/asm/pci.h
+++ b/arch/parisc/include/asm/pci.h
@@ -233,7 +233,6 @@ static inline void pcibios_register_hba(struct pci_hba_data *x)
* rp7420/8420 boxes and then revisit this issue.
*/
#define pcibios_assign_all_busses() (1)
-#define pcibios_scan_all_fns(a, b) (0)

#define PCIBIOS_MIN_IO 0x10
#define PCIBIOS_MIN_MEM 0x1000 /* NBPG - but pci/setup-res.c dies */
diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index d9483c5..36057c8 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -40,7 +40,6 @@ struct pci_dev;
*/
#define pcibios_assign_all_busses() \
(ppc_pci_has_flag(PPC_PCI_REASSIGN_ALL_BUS))
-#define pcibios_scan_all_fns(a, b) 0

static inline void pcibios_set_master(struct pci_dev *dev)
{
diff --git a/arch/sh/include/asm/pci.h b/arch/sh/include/asm/pci.h
index d3633f5..4163950 100644
--- a/arch/sh/include/asm/pci.h
+++ b/arch/sh/include/asm/pci.h
@@ -10,7 +10,6 @@
or architectures with incomplete PCI setup by the loader */

#define pcibios_assign_all_busses() 1
-#define pcibios_scan_all_fns(a, b) 0

/*
* A board can define one or more PCI channels that represent built-in (or
diff --git a/arch/sparc/include/asm/pci_32.h b/arch/sparc/include/asm/pci_32.h
index b41c4c1..810d924 100644
--- a/arch/sparc/include/asm/pci_32.h
+++ b/arch/sparc/include/asm/pci_32.h
@@ -10,7 +10,6 @@
* or architectures with incomplete PCI setup by the loader.
*/
#define pcibios_assign_all_busses() 0
-#define pcibios_scan_all_fns(a, b) 0

#define PCIBIOS_MIN_IO 0UL
#define PCIBIOS_MIN_MEM 0UL
diff --git a/arch/sparc/include/asm/pci_64.h b/arch/sparc/include/asm/pci_64.h
index 7a1e356..a329708 100644
--- a/arch/sparc/include/asm/pci_64.h
+++ b/arch/sparc/include/asm/pci_64.h
@@ -10,7 +10,6 @@
* or architectures with incomplete PCI setup by the loader.
*/
#define pcibios_assign_all_busses() 0
-#define pcibios_scan_all_fns(a, b) 0

#define PCIBIOS_MIN_IO 0UL
#define PCIBIOS_MIN_MEM 0UL
diff --git a/arch/um/include/asm/pci.h b/arch/um/include/asm/pci.h
index 5992319..b44cf59 100644
--- a/arch/um/include/asm/pci.h
+++ b/arch/um/include/asm/pci.h
@@ -2,6 +2,5 @@
#define __UM_PCI_H

#define PCI_DMA_BUS_IS_PHYS (1)
-#define pcibios_scan_all_fns(a, b) 0

#endif
diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index b51a1e8..73cd31c 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -48,7 +48,6 @@ extern unsigned int pcibios_assign_all_busses(void);
#else
#define pcibios_assign_all_busses() 0
#endif
-#define pcibios_scan_all_fns(a, b) 0

extern unsigned long pci_mem_start;
#define PCIBIOS_MIN_IO 0x1000
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index f1ae247..b613cad 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1056,8 +1056,7 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
if (dev && !dev->is_added) /* new device? */
nr++;

- if ((dev && dev->multifunction) ||
- (!dev && pcibios_scan_all_fns(bus, devfn))) {
+ if (dev && dev->multifunction) {
for (fn = 1; fn < 8; fn++) {
dev = pci_scan_single_device(bus, devfn + fn);
if (dev) {
diff --git a/include/asm-generic/pci.h b/include/asm-generic/pci.h
index b4326b5..1a1496f 100644
--- a/include/asm-generic/pci.h
+++ b/include/asm-generic/pci.h
@@ -30,8 +30,6 @@ pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
res->end = region->end;
}

-#define pcibios_scan_all_fns(a, b) 0
-
#ifndef HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
{


2009-06-22 14:21:24

by Kyle McMartin

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, Jun 22, 2009 at 08:08:07AM -0600, Alex Chiang wrote:
> Cc: Kyle McMartin <[email protected]>

Acked-by: Kyle McMartin <[email protected]>

2009-06-22 14:30:29

by Ralf Baechle

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, Jun 22, 2009 at 08:08:07AM -0600, Alex Chiang wrote:

> This was #define'd as 0 on all platforms, so let's get rid of it.
>
> This change makes pci_scan_slot() slightly easier to read.

I don't see why one would want to have a non-zero pcibios_scan_all_fns()
anyway.

Acked-by: Ralf Baechle <[email protected]>

Ralf

2009-06-22 14:34:41

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, Jun 22, 2009 at 08:08:07AM -0600, Alex Chiang wrote:
> This was #define'd as 0 on all platforms, so let's get rid of it.
>
> This change makes pci_scan_slot() slightly easier to read.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f1ae247..b613cad 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1056,8 +1056,7 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
> if (dev && !dev->is_added) /* new device? */
> nr++;
>
> - if ((dev && dev->multifunction) ||
> - (!dev && pcibios_scan_all_fns(bus, devfn))) {
> + if (dev && dev->multifunction) {
> for (fn = 1; fn < 8; fn++) {
> dev = pci_scan_single_device(bus, devfn + fn);
> if (dev) {

What a good idea. I was just looking at making this more complicated
(due to the ARI capability).

I'd like to know what the KVM / Xen / ... people think about this.
I don't know if they rely on function 5 being able to show up out of
the blue.

Anyway,
Reviewed-by: Matthew Wilcox <[email protected]>

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2009-06-22 14:36:23

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, Jun 22, 2009 at 03:26:02PM +0100, Ralf Baechle wrote:
> On Mon, Jun 22, 2009 at 08:08:07AM -0600, Alex Chiang wrote:
> > This was #define'd as 0 on all platforms, so let's get rid of it.
> >
> > This change makes pci_scan_slot() slightly easier to read.
>
> I don't see why one would want to have a non-zero pcibios_scan_all_fns()
> anyway.

If you have real hardware, absolutely. If you have a hypervisor that
gives you function 5 of a device (instead of mapping your function 0 to
function 5 of the real device), you'll want something along these lines.

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2009-06-22 15:31:18

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, Jun 22, 2009 at 08:08:07AM -0600, Alex Chiang wrote:
> This was #define'd as 0 on all platforms, so let's get rid of it.
>
> This change makes pci_scan_slot() slightly easier to read.
>
> Cc: Ivan Kokshaysky <[email protected]>
> Cc: Russell King <[email protected]>

Acked-by: Russell King <[email protected]>

For:

> diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
> index 0abf386..226cddd 100644
> --- a/arch/arm/include/asm/pci.h
> +++ b/arch/arm/include/asm/pci.h
> @@ -6,8 +6,6 @@
>
> #include <mach/hardware.h> /* for PCIBIOS_MIN_* */
>
> -#define pcibios_scan_all_fns(a, b) 0
> -
> #ifdef CONFIG_PCI_HOST_ITE8152
> /* ITE bridge requires setting latency timer to avoid early bus access
> termination by PIC bus mater devices

Thanks.

2009-06-22 16:54:56

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Monday 22 June 2009, Alex Chiang wrote:
> --- a/include/asm-generic/pci.h
> +++ b/include/asm-generic/pci.h
> @@ -30,8 +30,6 @@ pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> res->end = region->end;
> }
>
> -#define pcibios_scan_all_fns(a, b) 0
> -
> #ifndef HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
> static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> {
>

Acked-by: Arnd Bergmann <[email protected]>

2009-06-22 18:20:36

by Jeremy Fitzhardinge

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On 06/22/09 07:34, Matthew Wilcox wrote:
> On Mon, Jun 22, 2009 at 08:08:07AM -0600, Alex Chiang wrote:
>
>> This was #define'd as 0 on all platforms, so let's get rid of it.
>>
>> This change makes pci_scan_slot() slightly easier to read.
>>
>
>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index f1ae247..b613cad 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1056,8 +1056,7 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
>> if (dev && !dev->is_added) /* new device? */
>> nr++;
>>
>> - if ((dev && dev->multifunction) ||
>> - (!dev && pcibios_scan_all_fns(bus, devfn))) {
>> + if (dev && dev->multifunction) {
>> for (fn = 1; fn < 8; fn++) {
>> dev = pci_scan_single_device(bus, devfn + fn);
>> if (dev) {
>>
>
> What a good idea. I was just looking at making this more complicated
> (due to the ARI capability).
>
> I'd like to know what the KVM / Xen / ... people think about this.
> I don't know if they rely on function 5 being able to show up out of
> the blue.
>

We want to be able to export specific functions to a particular domain,
so it might see a PCI device with only function 5.

It looks like we lose that ability with this patch, is that right?

J

2009-06-22 18:31:10

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, Jun 22, 2009 at 11:20:24AM -0700, Jeremy Fitzhardinge wrote:
> > I'd like to know what the KVM / Xen / ... people think about this.
> > I don't know if they rely on function 5 being able to show up out of
> > the blue.
>
> We want to be able to export specific functions to a particular domain,
> so it might see a PCI device with only function 5.
>
> It looks like we lose that ability with this patch, is that right?

That would be correct. I'm guessing your out-of-tree code sets
pcibios_scan_all_fns()?

Now, there are various options. One is that you could remap config
space accesses -- domain:bus:dev.fn in the guest don't have to match
domain:bus:dev.fn in the host. That's a certain amount of overhead in
every config space access, but it doesn't have to be a large one.

Another would be that you could create dummy devices in the guest at
function 0, and then the guest would scan all the functions. A little
ugly, perhaps.

A third would be for guests to not do this scanning at all. You could
present the devices through something like the openfirmware tree, and
create them insteaqd of scanning for them. If you care about startup
time, this is probably the way to go.

There's probably other ways I haven't thought of ...

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2009-06-22 23:42:26

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, 2009-06-22 at 12:30 -0600, Matthew Wilcox wrote:
>
> That would be correct. I'm guessing your out-of-tree code sets
> pcibios_scan_all_fns()?
>
> Now, there are various options. One is that you could remap config
> space accesses -- domain:bus:dev.fn in the guest don't have to match
> domain:bus:dev.fn in the host. That's a certain amount of overhead in
> every config space access, but it doesn't have to be a large one.
>
That's tricky. Some devices have internal registers that -do- depend on
what function they are on. In fact, I remember seeing that in
multifunction devices that are meant to be virtualized but still need to
have some registers be accessed differently depending on the function
(ugh) though don't ask me who that was ...

> Another would be that you could create dummy devices in the guest at
> function 0, and then the guest would scan all the functions. A little
> ugly, perhaps.

But less ugly than the above.

> A third would be for guests to not do this scanning at all. You could
> present the devices through something like the openfirmware tree, and
> create them insteaqd of scanning for them. If you care about startup
> time, this is probably the way to go.

Which is what we do on powerpc nowadays. In fact, this code is currently
inside arch/powerpc and arch/sparc (2 copies slightly diverged) but I
had plans to make it common at move it over to either drivers/of or
drivers/pci (most probably the later).

> There's probably other ways I haven't thought of ...

Well, making up the devices without actual config space probing is nice
and fast but I don't think we want to see too many occurences of such
code in the kernel. We already had breakage once in powerpc land iirc
due to changes in drivers/pci/probe.c that we didn't reflect properly. I
think the normal and OF methods should be enough.

At this stage, it does look to me like a trivial tweak like
pcibios_scan_all_fns() but maybe done a bit nicely, would still be the
simplest solution in term of amount of code involved etc...

Maybe something like

pcibios_get_slot_fn_mask() which returns a bitmask of functions to be
scanned, whose default implementation (weak) would basically check
the header type for function 0 ? As I said, I don't -need- that right
now on powerpc "server" platforms but heh...

Cheers,
Ben.

2009-06-22 23:55:52

by Jeremy Fitzhardinge

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On 06/22/09 11:30, Matthew Wilcox wrote:
> On Mon, Jun 22, 2009 at 11:20:24AM -0700, Jeremy Fitzhardinge wrote:
>
>>> I'd like to know what the KVM / Xen / ... people think about this.
>>> I don't know if they rely on function 5 being able to show up out of
>>> the blue.
>>>
>> We want to be able to export specific functions to a particular domain,
>> so it might see a PCI device with only function 5.
>>
>> It looks like we lose that ability with this patch, is that right?
>>
>
> That would be correct. I'm guessing your out-of-tree code sets
> pcibios_scan_all_fns()?
>

Yes.

> Now, there are various options. One is that you could remap config
> space accesses -- domain:bus:dev.fn in the guest don't have to match
> domain:bus:dev.fn in the host. That's a certain amount of overhead in
> every config space access, but it doesn't have to be a large one.
>

I think the problem with that is that certain devices have fixed
functions at fixed function numbers, so the drivers just expect the
function to be there and nowhere else (AFAIK there's no way to do
discovery on functions). Now perhaps you'd never export just some
functions to such devices, but that was the answer when I proposed doing
this.

And if we did want to do this, would it be acceptible to put config
space remapping in? I presume that's something that would have to be in
the generic pci library?

> Another would be that you could create dummy devices in the guest at
> function 0, and then the guest would scan all the functions. A little
> ugly, perhaps.
>

I wonder if that would upset drivers. Assuming they can cope with just
being given some functions, how could one generically make a function
look present-but-dummy enough to not upset the driver?

> A third would be for guests to not do this scanning at all. You could
> present the devices through something like the openfirmware tree, and
> create them insteaqd of scanning for them. If you care about startup
> time, this is probably the way to go.
>

I don't think startup time is an overriding concern.

J

2009-06-23 01:29:56

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, 2009-06-22 at 16:55 -0700, Jeremy Fitzhardinge wrote:
>
> I wonder if that would upset drivers. Assuming they can cope with just
> being given some functions, how could one generically make a function
> look present-but-dummy enough to not upset the driver?

Well, the dummy function would have a totally different vid/did, I think
that would work. In general, while I've seen drivers that -do- need to
know their real function number to get to the right registers, I haven't
seen much that also need to see function 0 "right" (or if they do, then
function 0 just needs to be plugged in the same partition and the whole
problem is somewhat moot).

Cheers,
Ben.

2009-06-23 01:49:21

by Chris Wright

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

* Matthew Wilcox ([email protected]) wrote:
> I'd like to know what the KVM / Xen / ... people think about this.
> I don't know if they rely on function 5 being able to show up out of
> the blue.

KVM does not rely on this. Typically a multi-function device's BDF gets
remapped between host and guest (usually becoming a single function,
function 0).

thanks,
-chris

2009-06-23 02:23:45

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, 2009-06-22 at 18:43 -0700, Chris Wright wrote:
> > I'd like to know what the KVM / Xen / ... people think about this.
> > I don't know if they rely on function 5 being able to show up out of
> > the blue.
>
> KVM does not rely on this. Typically a multi-function device's BDF
> gets
> remapped between host and guest (usually becoming a single function,
> function 0).

I'm surprised you don't hit problems with some devices/drivers here,
I have distinct memories of seeing such things as drivers who need
to know their own function number reliably.

Cheers,
Ben.

2009-06-23 16:44:32

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, Jun 22, 2009 at 08:08:07AM -0600, Alex Chiang wrote:
> This was #define'd as 0 on all platforms, so let's get rid of it.
>
> This change makes pci_scan_slot() slightly easier to read.
>
> Cc: Ivan Kokshaysky <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Yoshinori Sato <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Kyle McMartin <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mundt <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jeff Dike <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Signed-off-by: Alex Chiang <[email protected]>

Acked-by: Paul Mundt <[email protected]>

2009-06-23 18:31:30

by Avi Kivity

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On 06/22/2009 05:34 PM, Matthew Wilcox wrote:
>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index f1ae247..b613cad 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1056,8 +1056,7 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
>> if (dev&& !dev->is_added) /* new device? */
>> nr++;
>>
>> - if ((dev&& dev->multifunction) ||
>> - (!dev&& pcibios_scan_all_fns(bus, devfn))) {
>> + if (dev&& dev->multifunction) {
>> for (fn = 1; fn< 8; fn++) {
>> dev = pci_scan_single_device(bus, devfn + fn);
>> if (dev) {
>>
>
> What a good idea. I was just looking at making this more complicated
> (due to the ARI capability).
>
> I'd like to know what the KVM / Xen / ... people think about this.
> I don't know if they rely on function 5 being able to show up out of
> the blue.
>
>

You mean have a function 5 without a function 0? This doesn't impact
kvm at all.

Moreover, I see pcibios_scan_all_fns() uniformly defined as 0, so this
patch doesn't change much, does it?

--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

2009-06-23 19:08:36

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Tue, Jun 23, 2009 at 09:40:08AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2009-06-22 at 12:30 -0600, Matthew Wilcox wrote:
> >
> > That would be correct. I'm guessing your out-of-tree code sets
> > pcibios_scan_all_fns()?
> >
> > Now, there are various options. One is that you could remap config
> > space accesses -- domain:bus:dev.fn in the guest don't have to match
> > domain:bus:dev.fn in the host. That's a certain amount of overhead in
> > every config space access, but it doesn't have to be a large one.
> >
> That's tricky. Some devices have internal registers that -do- depend on
> what function they are on. In fact, I remember seeing that in
> multifunction devices that are meant to be virtualized but still need to
> have some registers be accessed differently depending on the function
> (ugh) though don't ask me who that was ...

That's pretty horrific. PA-RISC has a NS87415 superio chip that is
full of that kind of bogosity, but nobody's ever suggested it should
support v12n.

> > Another would be that you could create dummy devices in the guest at
> > function 0, and then the guest would scan all the functions. A little
> > ugly, perhaps.
>
> But less ugly than the above.

There's some nastiness when you want to later migrate function 0 into
the VM, and it looks a little ugly to have a fake func 0 in the VM with
nothing attached to it.

> > A third would be for guests to not do this scanning at all. You could
> > present the devices through something like the openfirmware tree, and
> > create them insteaqd of scanning for them. If you care about startup
> > time, this is probably the way to go.
>
> Which is what we do on powerpc nowadays. In fact, this code is currently
> inside arch/powerpc and arch/sparc (2 copies slightly diverged) but I
> had plans to make it common at move it over to either drivers/of or
> drivers/pci (most probably the later).

I'd support a drivers/pci/of.c. Definitely better than having two copies
of it under arch/, and you'd be well within your rights to complain if
we changed something and didn't fix it up.

> > There's probably other ways I haven't thought of ...
>
> Well, making up the devices without actual config space probing is nice
> and fast but I don't think we want to see too many occurences of such
> code in the kernel. We already had breakage once in powerpc land iirc
> due to changes in drivers/pci/probe.c that we didn't reflect properly. I
> think the normal and OF methods should be enough.

Particularly since we have these people creating fake OF trees for
embedded platforms so we don't have to probe them. The v12n people
should definitely take advantage of this work.

> At this stage, it does look to me like a trivial tweak like
> pcibios_scan_all_fns() but maybe done a bit nicely, would still be the
> simplest solution in term of amount of code involved etc...
>
> Maybe something like
>
> pcibios_get_slot_fn_mask() which returns a bitmask of functions to be
> scanned, whose default implementation (weak) would basically check
> the header type for function 0 ? As I said, I don't -need- that right
> now on powerpc "server" platforms but heh...

So we need to tweak this code anyway for Alternate RoutingID Interpretation,
and what I've ended up doing is creating a bunch of different functions that
can be called to determine what the next function to probe should be, given
the current device and function.

Take a look: http://marc.info/?l=linux-pci&m=124578246906927&w=2

It wouldn't be hard to continue supporting pcibios_scan_all_fns() with
this scheme; it's an extra two lines:

+ else if (pcibios_scan_all_fns())
+ next_fn = next_trad_fn;

I think simply materialising them, either the way the OF code does,
or the way the IOV code does is the best route forwards.

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2009-06-23 20:34:55

by Jeremy Fitzhardinge

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On 06/23/09 12:08, Matthew Wilcox wrote:
> I think simply materialising them, either the way the OF code does,
> or the way the IOV code does is the best route forwards.
>

On reflection, I think this will work. We have a Xen pci passthrough
driver which gets told about the passed-through devices via xenbus, and
does the appropriate setup. At first glance, there doesn't seem to be
any problem with that code just explicitly instantiate the devices at
the PCI level in the same way pci_scan_device does (ie,
alloc_pci_device, initalize the dev struct, pci_setup_device).

Is that what you mean?

IanC has looked at that code more closely, so perhaps he can confirm
that this will work on our side.

J

2009-06-23 20:57:07

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Tue, Jun 23, 2009 at 01:34:46PM -0700, Jeremy Fitzhardinge wrote:
> On 06/23/09 12:08, Matthew Wilcox wrote:
> > I think simply materialising them, either the way the OF code does,
> > or the way the IOV code does is the best route forwards.
> >
>
> On reflection, I think this will work. We have a Xen pci passthrough
> driver which gets told about the passed-through devices via xenbus, and
> does the appropriate setup. At first glance, there doesn't seem to be
> any problem with that code just explicitly instantiate the devices at
> the PCI level in the same way pci_scan_device does (ie,
> alloc_pci_device, initalize the dev struct, pci_setup_device).

Even easier actually, pci_scan_single_device() should do everything you want.

> Is that what you mean?
>
> IanC has looked at that code more closely, so perhaps he can confirm
> that this will work on our side.
>
> J
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arch" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2009-06-23 21:49:26

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Tue, 2009-06-23 at 13:08 -0600, Matthew Wilcox wrote:
>
> I think simply materialising them, either the way the OF code does,
> or the way the IOV code does is the best route forwards.

Agreed. I'm fine with removing pcibios_scan_all_fns() for now anyway,
and leave whatever arch do care with the responsibility of finding a
solution though :-)

Regarding the OF stuff, I'll see what I can do vs. moving it to
drivers/pci some time next week after I'm done fixing the regressions
from this merge window :-)

Cheers,
Ben.

2009-06-23 22:02:14

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Tue, 2009-06-23 at 13:34 -0700, Jeremy Fitzhardinge wrote:
> On reflection, I think this will work. We have a Xen pci passthrough
> driver which gets told about the passed-through devices via xenbus,
> and
> does the appropriate setup. At first glance, there doesn't seem to be
> any problem with that code just explicitly instantiate the devices at
> the PCI level in the same way pci_scan_device does (ie,
> alloc_pci_device, initalize the dev struct, pci_setup_device).
>
> Is that what you mean?
>
> IanC has looked at that code more closely, so perhaps he can confirm
> that this will work on our side.
>
Yes, we do that in arch/powerpc/kernel/pci_64.c (and somewhere in
arch/sparc too). I'll merge those 2 implementations one of these days
and move them to drivers/of.

I think Willy was actually suggesting that you start using our
OF/device-tree stuff though which is a slightly more invasive deal but
might end up useful for other things in the long run :-)

Cheers,
Ben.

2009-06-23 22:24:56

by Jeremy Fitzhardinge

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On 06/23/09 14:49, Benjamin Herrenschmidt wrote:
> I think Willy was actually suggesting that you start using our
> OF/device-tree stuff though which is a slightly more invasive deal but
> might end up useful for other things in the long run :-)
>

I guess we could fake up OF-like datastructures or something, but I
don't see what wide application that would have. It might help in this
case, but its a lot more complex than just calling
pci_scan_single_device() at the appropriate place.

Do you have any other cases in mind where it would be helpful?

J

2009-06-23 22:43:31

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Tue, 2009-06-23 at 15:24 -0700, Jeremy Fitzhardinge wrote:
> I guess we could fake up OF-like datastructures or something, but I
> don't see what wide application that would have. It might help in this
> case, but its a lot more complex than just calling
> pci_scan_single_device() at the appropriate place.

It would be indeed more complex and probably not something to do
right now to solve that specific problem

> Do you have any other cases in mind where it would be helpful?

Well, it might be for virtual device discovery etc... but don't bother
now. We might talk about it at KS for those interested. It's more
something we see as useful for embedded archs at the moment but in the
long run it might make sense for hypervisors as well.

Cheers,
Ben.

2009-06-23 22:53:27

by Jeremy Fitzhardinge

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On 06/23/09 15:41, Benjamin Herrenschmidt wrote:
>> Do you have any other cases in mind where it would be helpful?
>>
>
> Well, it might be for virtual device discovery etc... but don't bother
> now. We might talk about it at KS for those interested. It's more
> something we see as useful for embedded archs at the moment but in the
> long run it might make sense for hypervisors as well.
>

Perhaps. We have Xenbus - which is a little bit like OF in that it has
data in a hierarchical namespace - and I guess it might be possible to
find a mapping onto some generic OF-like interface. However, Xenbus is
an active communication channel between virtual machines rather than a
static representation of a machine configuration (for example, you can
put a watch on a particular path to get a notification of when someone
else changes it).

But, yes, this is a good KS hallway track subject.

J

2009-06-24 00:04:24

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Tue, 2009-06-23 at 15:53 -0700, Jeremy Fitzhardinge wrote:
> On 06/23/09 15:41, Benjamin Herrenschmidt wrote:
> >> Do you have any other cases in mind where it would be helpful?
> >>
> >
> > Well, it might be for virtual device discovery etc... but don't bother
> > now. We might talk about it at KS for those interested. It's more
> > something we see as useful for embedded archs at the moment but in the
> > long run it might make sense for hypervisors as well.
> >
>
> Perhaps. We have Xenbus - which is a little bit like OF in that it has
> data in a hierarchical namespace - and I guess it might be possible to
> find a mapping onto some generic OF-like interface.

Which is sort-of what we did. IE. We disconnected the device-tree itself
from the underlying firmware, using the device-tree and OF-style
bindings (in some case simplified) as a basis for representing devices
but without the need for an actual open firmware underneath.

> However, Xenbus is an active communication channel between virtual machines
> rather than a
> static representation of a machine configuration (for example, you can
> put a watch on a particular path to get a notification of when someone
> else changes it).

On ppc64 too, the HV can feed us with new tree nodes or remove some, it
doesn't have to be static. Though we mostly use it as a static tree on
embedded.

> But, yes, this is a good KS hallway track subject.

Cheers,
Ben.

2009-06-24 10:31:18

by Ian Campbell

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Tue, 2009-06-23 at 13:34 -0700, Jeremy Fitzhardinge wrote:
> On 06/23/09 12:08, Matthew Wilcox wrote:
> > I think simply materialising them, either the way the OF code does,
> > or the way the IOV code does is the best route forwards.
> >
>
> On reflection, I think this will work. We have a Xen pci passthrough
> driver which gets told about the passed-through devices via xenbus, and
> does the appropriate setup. At first glance, there doesn't seem to be
> any problem with that code just explicitly instantiate the devices at
> the PCI level in the same way pci_scan_device does (ie,
> alloc_pci_device, initalize the dev struct, pci_setup_device).
>
> Is that what you mean?
>
> IanC has looked at that code more closely, so perhaps he can confirm
> that this will work on our side.

I'm not 100% familiar with this stuff but pcifront_rescan_root()
currently iterates over all devfns for a bus and calls
pci_scan_single_device(). I don't see where we call pci_scan_slot()
which I think would be the only way pcibios_scan_all_fns() would matter
to us so I'm not sure why we have that patch. It's possible all this has
changed since AlexN originally did the work? It was quite a long time
ago now.

Ian.

2009-06-29 20:34:00

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH] PCI: remove pcibios_scan_all_fns()

On Mon, 22 Jun 2009 08:08:07 -0600
Alex Chiang <[email protected]> wrote:

> This was #define'd as 0 on all platforms, so let's get rid of it.
>
> This change makes pci_scan_slot() slightly easier to read.
>
> Cc: Ivan Kokshaysky <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Yoshinori Sato <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Kyle McMartin <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mundt <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jeff Dike <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Signed-off-by: Alex Chiang <[email protected]>
> ---

Applied this to my linux-next tree, thanks.

--
Jesse Barnes, Intel Open Source Technology Center