RE: RFC: Changes for PCI

RE: RFC: Changes for PCI

[Khachaturov, Vassilii]

On Wed, 27 Jun 2001, Jeff Garzik wrote:
 
> 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.

Is it? With a hotplug device removed and another one added,
isn't there a slight chance that the pci_dev pointer to the new device
will get allocated in place of the old one?

Vassilii

Re: RFC: Changes for PCI

[Jeff Garzik]

"Khachaturov, Vassilii" wrote:
> 
> On Wed, 27 Jun 2001, Jeff Garzik wrote:
> 
> > 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.
> 
> Is it? With a hotplug device removed and another one added,
> isn't there a slight chance that the pci_dev pointer to the new device
> will get allocated in place of the old one?

If you want to get pedantic, yes ;-)  The pci_dev pointer is unique for
the lifetime of the PCI device, which works just as well in the example
used in the thread.

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

Re: RFC: Changes for PCI

[Richard Henderson]

On Wed, Jun 27, 2001 at 02:07:37PM -0500, Tom Gall wrote:
> 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.

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~

Re: RFC: Changes for PCI

[Tom Gall]

Richard Henderson wrote:

> On Wed, Jun 27, 2001 at 02:07:37PM -0500, Tom Gall wrote:
> > 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.
>
> 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.

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.

> 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.

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) tom_gall@vnet.ibm.com         shattering ka-boom!"
(w) 507-253-4558                 -- Marvin Martian
(h) tgall@rochcivictheatre.org
http://www.ibm.com/linux/ltc/projects/ppc

Re: RFC: Changes for PCI

[Gérard Roudier]

On Thu, 28 Jun 2001, Alan Cox wrote:

> > 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
>
> I think it might be better to fix the needed drivers. I suspect ppc64 isnt
> going to need that man drivers handle with initially

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).

> > 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.
>
> 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

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

  Gérard.

Re: RFC: Changes for PCI

[Jeff Garzik]

Tom Gall wrote:
> Gérard Roudier wrote:
> > 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).
> 
> Thanks much for the clarification. Do you still battle buggy double reporting?
> Has this been fixed? Is it a bug on some specific architecture?

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     |

Re: RFC: Changes for PCI

[Jeff Garzik]

Gérard Roudier wrote:
> 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).

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


> 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.

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     |

Re: RFC: Changes for PCI

[Tom Gall]

Gérard Roudier wrote:
> 
> On Wed, 27 Jun 2001, Jeff Garzik wrote:
> 
> > Tom Gall wrote:
> > > 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?
> >
> > 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.
> 
> 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).

Thanks much for the clarification. Do you still battle buggy double reporting?
Has this been fixed? Is it a bug on some specific architecture?
 
> 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).

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)

> 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.

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

>   Gérard.

Regards,

Tom

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

Re: RFC: Changes for PCI

[Gérard Roudier]

On Wed, 27 Jun 2001, Jeff Garzik wrote:

> Tom Gall wrote:
> > 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?
>
> 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.

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.

Re: RFC: Changes for PCI

[Todd Inglett]

Jeff Garzik wrote:
> 
> 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"

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

-- 
-todd

Re: RFC: Changes for PCI

[Alan Cox]

> 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

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

> 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.

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

Re: RFC: Changes for PCI

[Jeff Garzik]

Todd Inglett wrote:
> 
> "David S. Miller" wrote:
> >
> > Tom Gall writes:
> >  >   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?
> >
> > This is totally unreasonable.
> 
> 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...
> 
> "David S. Miller" wrote:
> > There are only two real issues:
> >
> > 1) Extending the type bus numbers use inside the kernel.
> >
> >    Basically how most multi-controller platforms work now
> >    is they allocate bus numbers in the 256 bus space as
> >    controllers are probed. If we change the internal type
> >    used by the kernel to "u32" or whatever, we expand that
> >   available space accordingly.
> >
> >   For the lazy, basically go into include/linux/pci.h
> >   and change the "unsigned char"s in struct pci_bus into
> >   some larger type. This is mindless work.

> 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.

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     |

Re: RFC: Changes for PCI

[Todd Inglett]

"David S. Miller" wrote:
> 
> Tom Gall writes:
>  >   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?
> 
> This is totally unreasonable.

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...

"David S. Miller" wrote:
> There are only two real issues: 
> 
> 1) Extending the type bus numbers use inside the kernel. 
> 
>    Basically how most multi-controller platforms work now 
>    is they allocate bus numbers in the 256 bus space as 
>    controllers are probed. If we change the internal type 
>    used by the kernel to "u32" or whatever, we expand that 
>   available space accordingly. 
> 
>   For the lazy, basically go into include/linux/pci.h 
>   and change the "unsigned char"s in struct pci_bus into 
>   some larger type. This is mindless work. 

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().

> What you want are PCI domains.

Correct.  This is 2.5.

-- 
-todd

Re: RFC: Changes for PCI

[David S. Miller]

Tom Gall writes:
 > "David S. Miller" wrote:
 > 
 > > Looks, ppc64 is really still experimental right?
 > 
 > Heck no.

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.

 > > Which means it is
 > > 2.5.x material, and 2.5.x has been quoted as being a week or two away.
 > 
 > I sure hope that ppc64 is NOT considered 2.5.x material.

No, I'm saying that ppc64 with >=256 physical PCI busses, is 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?

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
>4GB ram on 64-bit PCI systems (sparc64, alpha, etc.).  And it was
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
davem@redhat.com

Re: RFC: Changes for PCI

[David S. Miller]

anton@samba.org writes:
 > > 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.
 > 
 > 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.

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
davem@redhat.com

Re: RFC: Changes for PCI

[David S. Miller]

Tom Gall writes:
 > 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?

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

 > 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.

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
davem@redhat.com

Re: RFC: Changes for PCI

[David S. Miller]

Jeff Garzik writes:
 > >   Is this reasonable for everyone?
 > 
 > Why not use sysdata like the other arches?

Right.

 > 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.

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

Later,
David S. Miller
davem@redhat.com

Re: RFC: Changes for PCI

[David S. Miller]

Tom Gall writes:
 >   Part one is the following changes to include/linux/pci.h

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

 >   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?

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.

 >   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.     */

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

 >   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.

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.

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

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
davem@redhat.com

Re: RFC: Changes for PCI

[David S. Miller]

Pete Zaitcev writes:
 > > 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...
 > 
 > Can it be done by comparing struct pci_dev pointers for equal?

Exactly.

Later,
David S. Miller
davem@redhat.com

Re: RFC: Changes for PCI

[Pete Zaitcev]

> 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...

Can it be done by comparing struct pci_dev pointers for equal?

-- Pete

Re: RFC: Changes for PCI

[Jeff Garzik]

Tom Gall wrote:
> 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?

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.


> > 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.
> 
> 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.

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     |

Re: RFC: Changes for PCI

[anton]

 
> 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.

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

Re: RFC: Changes for PCI

[anton]

Hi,

> 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.     */

I do not think these functions are required at all.

> 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. 

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

Re: RFC: Changes for PCI

[Tom Gall]

Jeff Garzik wrote:
> 
> Tom Gall wrote:
> >   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?
> 
> Why not use sysdata like the other arches?

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?

> 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.

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.
 
>         Jeff

Regards,

Tom

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

Re: RFC: Changes for PCI

[Jeff Garzik]

Tom Gall wrote:
>   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?

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     |

RFC: Changes for PCI

[Tom Gall]

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.     */


--- linux-2.4.5-ac18/include/linux/pci.h    Tue Jun 26 21:59:23 2001
+++ linuxppc64_2_4/include/linux/pci.h  Wed Jun 27 13:36:26 2001
@@ -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
--- linux-2.4.5-ac18/drivers/pci/pci.c  Tue Jun 26 21:55:54 2001
+++ linuxppc64_2_4/drivers/pci/pci.c    Tue Jun 26 08:09:31 2001
@@ -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) tom_gall@vnet.ibm.com         shattering ka-boom!"
(w) 507-253-4558                 -- Marvin Martian
(h) tgall@rochcivictheatre.org
http://www.ibm.com/linux/ltc/projects/ppc

Re: RFC: Changes for PCI

[Tom Gall]

"David S. Miller" wrote:

> Tom Gall writes:
>  > "David S. Miller" wrote:
>  >
>  > > Looks, ppc64 is really still experimental right?
>  >
>  > Heck no.
>
> So it is so stable that it isn't even merged into the mainline 2.4.x
> sources? :-)

Heh...

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

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

>  > > Which means it is
>  > > 2.5.x material, and 2.5.x has been quoted as being a week or two away.
>  >
>  > I sure hope that ppc64 is NOT considered 2.5.x material.
>
> No, I'm saying that ppc64 with >=256 physical PCI busses, is 2.5.x
> material.

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

>  > 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?
>
> Yes, no kludges for 2.4.x

Understood and agreed.

> Look, I do not even feel for you.
>
> I waited patiently for a sane PCI dma architecture so I could support
> >4GB ram on 64-bit PCI systems (sparc64, alpha, etc.).  And it was
> worth the wait, most of the important PCI drivers fully use this
> interface, and it was all done properly.

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.

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

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) tom_gall@vnet.ibm.com         shattering ka-boom!"
(w) 507-253-4558                 -- Marvin Martian
(h) tgall@rochcivictheatre.org
http://www.ibm.com/linux/ltc/projects/ppc

Re: RFC: Changes for PCI

[Tom Gall]

"David S. Miller" wrote:

> Tom Gall writes:
>  > 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.
>
> Well, if the claim is that your suggested changes provide this
> "commonality", I totally disagree.

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.

> 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.

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) tom_gall@vnet.ibm.com         shattering ka-boom!"
(w) 507-253-4558                 -- Marvin Martian
(h) tgall@rochcivictheatre.org
http://www.ibm.com/linux/ltc/projects/ppc

Re: RFC: Changes for PCI

[Tom Gall]

"David S. Miller" wrote:

> Looks, ppc64 is really still experimental right?

Heck no.

> Which means it is
> 2.5.x material, and 2.5.x has been quoted as being a week or two away.

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

> 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.

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?

> 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.

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) tom_gall@vnet.ibm.com         shattering ka-boom!"
(w) 507-253-4558                 -- Marvin Martian
(h) tgall@rochcivictheatre.org
http://www.ibm.com/linux/ltc/projects/ppc

Re: RFC: Changes for PCI

[Tom Gall]

Jeff Garzik wrote:

> Tom Gall wrote:
> > 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?
>
> I hesitate to call it a device driver bug, because that was likely the
> best decision Gerard could make at the time.

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


> 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.

Sound good.

> > > 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.
>
> Sorry, not pci_domain, just system bus number, for any bus, like we
> talked about in the previous discussion.

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) tom_gall@vnet.ibm.com         shattering ka-boom!"
(w) 507-253-4558                 -- Marvin Martian
(h) tgall@rochcivictheatre.org
http://www.ibm.com/linux/ltc/projects/ppc