RFC: Changes for PCI

Hi All,

I'm looking for commentary on the following. As you might recall I had posted
a note a few days back on the lkml about the kinds of systems ppc64 runs on and the reality of having a boatload of PCI buses out there.

I sure appreciate all the comments and feedback.

Today appended below are two patches that address more or less how we've
solved the problem for ppc64 from an architecture independant standpoint. Hopefully you will find these patches reasonable or at a minimum a starting part for more discussion to make this happen. (Our ppc64 work is at linuxppc64.org or in my dir on kernel.org pub/linux/kernel/people/tgall if you're interested)

Part one is the following changes to include/linux/pci.h

The first part changes number, primary, and secondary to unsigned ints from
chars. What we do is encode the PCI "domain" aka PCI Primary Host Bridge, aka pci controller in with the bus number. In our case we do it like this:

pci_controller=dev->bus->number>>8) &0xFF0000
bus_number= dev->bus->number&0x0000FF),

Is this reasonable for everyone?

subordinate probably doesn't need to go to an int really when I come to think
about it as it can fit in a char, but how about in the future? A switch to an unsigned int as well seemed prudent.

The following 3 functions are added. Their purpose is a little different than
to add support for more than 256 buses but they are important. Skip ahead and I'll explain what they are for....

int (*pci_read_bases)(struct pci_dev *, int cnt,int rom); /* These optional hooks provide */
int (*pci_read_irq)(struct pci_dev *); /* the arch dependant
code a way*/
int (*pci_fixup_registers)(struct pci_dev *); /* to manage the
registers. */

@@ -60,6 +60,8 @@
 #define PCI_CACHE_LINE_SIZE    0x0c    /* 8 bits */ 
 #define PCI_LATENCY_TIMER  0x0d    /* 8 bits */ 
 #define PCI_HEADER_TYPE        0x0e    /* 8 bits */ 
+
+#define  PCI_MULTIFUNCTION_CARD 0x80 /* Multi-function bit in header. */
 #define  PCI_HEADER_TYPE_NORMAL    0
 #define  PCI_HEADER_TYPE_BRIDGE 1
 #define  PCI_HEADER_TYPE_CARDBUS 2

@@ -294,6 +296,13 @@
 #define PCIIOC_MMAP_IS_MEM (PCIIOC_BASE | 0x02)    /* Set mmap state to MEM

space. */
#define PCIIOC_WRITE_COMBINE (PCIIOC_BASE | 0x03) /* Enable/disable
write-combining. */

+/* Ioctls for /proc/bus/pci/X/Y nodes. */
+#define PCIIOC_BASE ('P' << 24 | 'C' << 16 | 'I' << 8)
+#define PCIIOC_CONTROLLER (PCIIOC_BASE | 0x00) /* Get controller for PCI
device. */
+#define PCIIOC_MMAP_IS_IO (PCIIOC_BASE | 0x01) /* Set mmap state to I/O
space. */
+#define PCIIOC_MMAP_IS_MEM (PCIIOC_BASE | 0x02) /* Set mmap state to MEM
space. */
+#define PCIIOC_WRITE_COMBINE (PCIIOC_BASE | 0x03) /* Enable/disable
write-combining. */
+
#ifdef __KERNEL__

#include <linux/types.h>
@@ -409,10 +418,10 @@
void *sysdata; /* hook for sys-specific extension */
struct proc_dir_entry *procdir; /* directory entry in /proc/bus/pci */

-   unsigned char   number;     /* bus number */
-   unsigned char   primary;    /* number of primary bridge */
-   unsigned char   secondary;  /* number of secondary bridge */
-   unsigned char   subordinate;    /* max number of subordinate buses */
+   unsigned int  number;       /* pci_controller number + bus number */
+   unsigned int  primary;      /* number of primary bridge */
+   unsigned int  secondary;    /* number of secondary bridge */
+   unsigned int  subordinate;  /* max number of subordinate buses */

    char        name[48];
    unsigned short  vendor; 
@@ -449,6 +458,9 @@
    int (*write_byte)(struct pci_dev *, int where, u8 val);
    int (*write_word)(struct pci_dev *, int where, u16 val);
    int (*write_dword)(struct pci_dev *, int where, u32 val);

+ int (*pci_read_bases)(struct pci_dev *, int cnt,int rom); /* These optional
hooks provide */
+ int (*pci_read_irq)(struct pci_dev *); /* the arch
dependant code a way*/
+ int (*)(struct pci_dev *); /* to manage the registers. */
};

struct pbus_set_ranges_data

The other file we changed is drivers/pci/pci.c

The 3 additional functions are hooks so that an architecture has a chance to
make sure things are in order beforehand. pci_read_bases is for the management and fixup of the BARs. pci_read_irq is the same but for IRQs. pci_fixup_registers again same idea but for bridge resources.

The essense of the design here is to allow you to get in and make sure
everything is ok with the card, bridge etc, beforehand so as to avoid multiple bus walks.

diff -u linux-2.4.5-ac18/drivers/pci/pci.c linuxppc64_2_4/drivers/pci/pci.c
@@ -1,5 +1,5 @@
 /*
- * $Id: pci.c,v 1.91 1999/01/21 13:34:01 davem Exp $
+ *
  *
  * PCI Bus Services, see include/linux/pci.h for further explanation.
  *

@@ -745,6 +745,14 @@
    u32 l, sz;
    struct resource *res;
 
+   /************************************************************
+    * Check for architecture dependant call to read the BARs.
+    ************************************************************/
+   if(  dev->bus->ops->pci_read_bases != NULL) {
+       dev->bus->ops->pci_read_bases(dev, howmany, rom);
+       return;
+   }
+
    for(pos=0; pos<howmany; pos = next) {
        next = pos+1;
        res = &dev->resource[pos];

@@ -1026,6 +1034,14 @@
 static void pci_read_irq(struct pci_dev *dev)
 {
    unsigned char irq;
+
+   /************************************************************
+    * Check for architecture dependant call to read and set irg
+    ************************************************************/
+   if(dev->bus->ops->pci_read_irq != NULL) {
+       dev->bus->ops->pci_read_irq(dev);
+       return;
+   }
 
    pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
    if (irq)

@@ -1047,7 +1063,17 @@
 {
    u32 class;
 
-   sprintf(dev->slot_name, "%02x:%02x.%d", dev->bus->number,

PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
+ /* sprintf(dev->slot_name, "%02x:%02x.%d", dev->bus->number,
PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn)); */
+
+
+ /********************************************************************
+ * Prefix the bus number with the phb number for large(>256 bus)
systems.
+ ********************************************************************/
+ sprintf(dev->slot_name, "%4x%02x:%02x.%1x",

+               ( (dev->bus->number>>8) &0xFF0000),
+               (  dev->bus->number&0x0000FF),
+               PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
+
    sprintf(dev->name, "PCI device %04x:%04x", dev->vendor, dev->device);

    pci_read_config_dword(dev, PCI_CLASS_REVISION, &class);
@@ -1091,6 +1117,14 @@
        printk(KERN_ERR "PCI: %s: class %x doesn't match header type %02x.
Ignoring class.\n",
               dev->slot_name, class, dev->hdr_type);
        dev->class = PCI_CLASS_NOT_DEFINED;

+ }
+
+
+ /*********************************************************************
+ * Give the architure code a chance to fix up/tweak the devices.
+ *********************************************************************/
+ if(dev->bus->ops->pci_fixup_registers != NULL) {

+       dev->bus->ops->pci_fixup_registers(dev);
    }

    /* We found a fine healthy device, go go go... */

So as Joel from MST3K used to say, "What do you think sirs?"

Regards,

Tom

Tom Gall - PPC64 Maintainer      "Where's the ka-boom? There was
Linux Technology Center           supposed to be an earth
(w) [email protected]         shattering ka-boom!"
(w) 507-253-4558                 -- Marvin Martian
(h) [email protected]
http://www.ibm.com/linux/ltc/projects/ppc

Why not use sysdata like the other arches?

Changing the meaning of dev->bus->number globally seems pointless. If you are going to do that, just do it the right way and introduce another struct member, pci_domain or somesuch.

Jeff

Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Right.

Actually, the concept of "system domains" is what we had finally talked about.

Later,
David S. Miller
[email protected]

Hi Jeff,

Well you have device drivers like the symbios scsi driver for instance that tries to determine if it's seen a card before. It does this by looking at the bus,dev etc numbers... It's quite reasonable for two different scsi cards to be on the same bus number, same dev number etc yet they are in different PCI domains.

Is this a device driver bug or feature?

Right, one could do that and then all the large machine architectures would have their own implementation for the same problem. That's not necessarily a bad thing, but some commonality I think would be a good thing.

Regards,

Tom

Tom Gall - PPC64 Maintainer      "Where's the ka-boom? There was
Linux Technology Center           supposed to be an earth
(w) [email protected]         shattering ka-boom!"
(w) 507-253-4558                 -- Marvin Martian
(h) [email protected]
http://www.ibm.com/linux/ltc/projects/ppc

As Peter Zaitcev and others have pointed out, they can use simple pointer comparisons here on the pci_dev struct.

Well, if the claim is that your suggested changes provide this "commonality", I totally disagree. Your changes do no more than provide hooks where no hooks are needed. The hooks are there, and are why we have "sysdata" and all the drivers/pci/setup-*.c buisness. If ppc64 cannot fit itself into the drivers/pci/setup-*.c infrastructure, either fix the infrastructure or concede that "our stuff is too weird to solve with generic code" and do what sparc64 does with driving it's own PCI probing layer.

Later,
David S. Miller
[email protected]

Hi,

I do not think these functions are required at all.

Please look at how other architectures solve this problem. For example on ppc32 we already fix up the BARs if required. There are also hooks to fix the irq, you seem to be duplicating all of this.

Anton

Thats 2.5 material. For 2.4 we should do as davem suggested and make the bus number unique. I do this by just adding 256 to each overlapping host bridge.

Anton

Looks, ppc64 is really still experimental right? Which means it is 2.5.x material, and 2.5.x has been quoted as being a week or two away.

So we can solve this problem for real, with system bus domains, and get ppc64 working all within the framework of 2.5.x which is just around the corner.

For now, I am rather sure your systems for testing have < 256 physical PCI busses and you can for 2.4.x use the remapping scheme sparc64 uses.

Later,
David S. Miller
[email protected]

I hesitate to call it a device driver bug, because that was likely the best decision Gerard could make at the time.

However, I think the driver (only going by your description) would be more correct to use a pointer to struct pci_dev. We have a token in the kernel that is guaranteed 100% unique to any given PCI device: the pointer to its struct pci_dev.

Sorry, not pci_domain, just system bus number, for any bus, like we talked about in the previous discussion.

Jeff

Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

The driver checks against PCI bus+dev+func in 2 situations:

1) To apply the boot order that user can set up in the controller NVRAMs.
2) To detect buggy double reporting of the same device by the kernel PCI code (this made lot of troubles at some time).

The great bug is to invent useless abstractions that don't match reality. Such brain masturbation leads to confusion (hence subtle bugs) and useless software bloatage (thus _real_ resource wastage).

If we want to handle _real_ PCI bus domains, we just have to add a domain number to identify a _real_ PCI device. Anything that wants to hide such reality in some opaque data looks like brain masturbation to me.

Gérard.

Cool. The premise of the thread was that you merely were checking for uniqueness, not order. That changes our answer...

I think all of us agree on this: in 2.5, our solution is to have a system domain number, which increases from zero each time you add sbus, pci bus, isa bus, etc.

For 2.4, non-x86 arches first had to deal with PCI domains, so the solution was to stick "arch-specific data" into pci_dev->sysdata, which in some cases was the PCI domain number.

So, you have an ugly solution in drivers for 2.4 if you need to know PCI domain for what reason, and a clean solution in 2.5.

Jeff

Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Thanks much for the clarification. Do you still battle buggy double reporting? Has this been fixed? Is it a bug on some specific architecture?

Agreed. (A couple of my posts last night didn't make it through... appears that us.ibm.com isn't set up entirely right for ENC)

Again also agreed. Now I'm REALLY anxious for 2.5 8-)

Regards,

Tom

Tom Gall - PPC64 Maintainer      "Where's the ka-boom? There was
Linux Technology Center           supposed to be an earth
(w) [email protected]         shattering ka-boom!"
(w) 507-253-4558                 -- Marvin Martian
(h) [email protected]
http://www.ibm.com/linux/ltc/projects/ppc

I've seen it occur in 2.2.x in buggy drivers, but in 2.4 the driver should -not- have to check for this. The PCI core takes care of it.

Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

You've mixed in here changes which already exist in include/linux/pci.h, namely the PCIIOC_* ioctl values. Please get your patches straight.

This is totally unreasonable.

Bus numbers are dictacted by the PCI standard, they cannot exist larger than 8-bits, so they are char. This is the end of the story.

What you want are PCI domains.

This seems totally unnecessary to me. Also your mailreader has fux0red the patch with linebreaks making most of this unreadable.

We have an entire infrastructure for this already. And if it isn't sufficient either fix it or control the whole PCI bus probe from arch specific layers, see arch/sparc64/kernel/pci_common.c for an example.

It's crap.

If the problem is to be fixed, it should be fixed correctly.

First lets look at your bus number expansion. How do your patches handle the user interface aspects of this? The bus number nodes of /proc/bus/pci/${BUS} are 2 digit hex values. They are still 2-digit hex values after your patch, so bus numbers >255 simply won't work with your changes.

This is only the beginning of the list of problems your changes do not address.

Fact is, we need PCI, actually "system", domains. Any other attempt is an outright kludge. And such outright kludges can be totally hidden in arch specific code _today_.

It is startling to me that the ppc64 folks looked at this problem and saw fit to make it some new issue. It's old as day, and it is solved for all reasonable cases already by sparc64, alpha, etc. As long as you have < 256 _PHYSICAL_ busses allocated on the machine, you can hide the controller issue by using unique PCI bus numbers throughout the system.

Sure, this does not handle >=256 busses, but when you hit that point you must fix it correctly. And this requires a system bus abstraction, not some hokey extending of the PCI bus number space.

All of your changes do not provide any new functionality. Several other architectures deal with whatever issues your new hooks solve. They do this all by themselves and without changes to generic code. Why can't ppc64 do the same?

Later,
David S. Miller
[email protected]

I don't doubt it. I wouldn't doubt I've been guilty of simliar things in my past...

Sound good.

Yes agreed. However do you think it's possible for the additional of such a value now in 2.4.x series? Alan? Linus?

Regards,

Tom
--
Tom Gall - PPC64 Maintainer "Where's the ka-boom? There was
Linux Technology Center supposed to be an earth
(w) [email protected] shattering ka-boom!"
(w) 507-253-4558 -- Marvin Martian
(h) [email protected]
http://www.ibm.com/linux/ltc/projects/ppc

Heck no.

I sure hope that ppc64 is NOT considered 2.5.x material.

A real solution would be nice. And if the real solution can ONLY be in 2.5, then is it such a bad idea moving the bus number type to unsigned int for 2.4.x?

Wellll, remember that post about more than 256 PCI buses? I meant it.

Regards,

Tom
--
Tom Gall - PPC64 Maintainer "Where's the ka-boom? There was
Linux Technology Center supposed to be an earth
(w) [email protected] shattering ka-boom!"
(w) 507-253-4558 -- Marvin Martian
(h) [email protected]
http://www.ibm.com/linux/ltc/projects/ppc

So it is so stable that it isn't even merged into the mainline 2.4.x sources? :-)

We're talking about a port which doesn't even exist in the mainline sources yet.

No, I'm saying that ppc64 with >=256 physical PCI busses, is 2.5.x material.

Yes, no kludges for 2.4.x

Look, I do not even feel for you.

I waited patiently for a sane PCI dma architecture so I could support

worth the wait, most of the important PCI drivers fully use this interface, and it was all done properly.

Similarly you can wait for 2.5.x for >=256 physical PCI bus support. Ok?

Later,
David S. Miller
[email protected]

Well let me be more clear and I should have been earlier. I think encoding the system domain into the bus value is a good approach IF and ONLY IF we're trying to avoid the addition of a new value to pci_dev. Alternatively a new value for pci_dev is to me the better approach.

Consider also in drivers/pci/pci.c:

The function pci_bus_exists checks based on bus numbers. This function is of course used by pci_alloc_primary_bus, which is in turn used by pci_scan_bus. As is today, this code can break me the second I'm onto my second PCI system domain.

Additionally there is pci_find_slot which is done by bus number and devfn number. If I don't take into account the PCI system domain I've got problems.

To get past that either things need to be expanded out to ints from chars or a new value for PCI system domain needs to be added. I'm happy with either, and personally I like the later better, but the former seems more compatible.

Sure one could do some fancy bus mapping and encoding, but then I'm going to be limited to some number of PCI system domains + buses <= 256. Not fun.

I need to look at this some more, perferably under the influence of more sugar and caffeine. Fitting in is one of my primary goals. I sure as heck do NOT want to do any changes to any directory outside of arch/ppc64 or include/asm-ppc64 than what is absolutely needed and for good technical reason.

Regards,

Tom

--
Tom Gall - PPC64 Maintainer "Where's the ka-boom? There was
Linux Technology Center supposed to be an earth
(w) [email protected] shattering ka-boom!"
(w) 507-253-4558 -- Marvin Martian
(h) [email protected]
http://www.ibm.com/linux/ltc/projects/ppc

Heh...

Just about there...finger crossed, Maintainers willing, etc etc.

Well, if that's what we gotta live with, then that's what we gotta live with. Viva la 2.5 then!

Understood and agreed.

Yeah and I understand and appreciate that just for the matter of the device driver owners making sure they are inline with the new direction.

Rather not, but if that's the decision, I'm happy to live by it. That why I posted this as an RFC, and I appreciate everyone's time, patience and feedback.

Regards,

Tom

--
Tom Gall - PPC64 Maintainer "Where's the ka-boom? There was
Linux Technology Center supposed to be an earth
(w) [email protected] shattering ka-boom!"
(w) 507-253-4558 -- Marvin Martian
(h) [email protected]
http://www.ibm.com/linux/ltc/projects/ppc

Well, back in the "Going beyond 256 PCI buses" thread two weeks ago when you argued that Linux not supporting >256 busses was a fallacy...

Yes it is mindless work and is in that patch. Should we attempt to go beyond 256 physical busses in 2.4? Maybe not. But it is a simple change and it does work and it works around the existing drivers which compare busid+devfn for uniqueness when they really should compare pci_dev pointers. Should it be redone the correct way (domains) in 2.5? Absolutely.

The patch does not handle the user mode case. This leaves the X server broken. We could probably weed out busses beyond 256 under /proc/bus/pci as a workaround -- meaning the video adapter (if any -- rare in these boxes) must be in one of the first I/O drawers.

The added pci_op functions are probably not needed if the code probes ahead in pcibios_fixup_bus().

Correct. This is 2.5.

-todd

I think it might be better to fix the needed drivers. I suspect ppc64 isnt going to need that man drivers handle with initially

Or scan the busses for video cards and number those busses 0,1,2... then number the rest. Ugly but probably best for 2.4

As far as the Symbios driver is in concern, there is nothing to fix.

1) The bogus double reporting of PCI devices used (uses) 2 different pci_dev structures.

2) The boot order has nothing to do with the kernel and must use the only relevant way to identify PCI devices in a PCI BUS hierarchy (bus + devfn).

Btw, the suggested PCI bus numbering change looks like utter hackery to me... Seems some guys somewhere are abusing tequilla too much. :-)

Gérard.

2.5 is right around the corner, and sysdata should handle PCI domains/segments just fine in 2.4.

Why do we need to patch 2.4 at all right now? Since 2.5 is close I
don't think it's a big deal saying "use 2.5+ for >256 physical buses"

Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

I agree...and we can always maintain a 2.4 patch on the side to handle the bus limit.

-todd

You'll find that the existing ports that support multiple pci domains do not number the busses on the secondary domains from zero. If domain 0 has 3 busses, then domain 1's root bus will be bus number 3, and so on.

This approach works quite well in practice, even on machines with large numbers of pci domains, since there tends to be no pci-pci busses on domains other than the one containing legacy i/o widgetry.

r~

Yes I've looked at this in depth, and it does work well to compact things down and conserve on the precious and limited bus numbers.

We have pci-pci bridges in every PCI system domain. Additionally we have cards that have pci-pci bridges on them and certainly they can be plugged in anywhere on the system.

Unfortunately the majority of our problem is centered on the fact that in the end we have more than 256 buses. I look forward to this limit disappearing in 2.5.

Regards,

Tom

--
Tom Gall - PPC64 Maintainer "Where's the ka-boom? There was
Linux Technology Center supposed to be an earth
(w) [email protected] shattering ka-boom!"
(w) 507-253-4558 -- Marvin Martian
(h) [email protected]
http://www.ibm.com/linux/ltc/projects/ppc