2005-01-12 00:03:34

by Domen Puncer

[permalink] [raw]
Subject: [patch 03/11] arch/i386/pci/i386.c: Use new for_each_pci_dev macro




As requested by Christoph Hellwig I've created a new macro called
for_each_pci_dev. It is a wrapper for this common use of pci_get/find_device:

(while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL))

This macro will return the pci_dev *for all pci devices. Here is the first patch I
used to test this macro with. Compiled and booted on my T23. There will be
53 more patches using this new macro.

Hanna Linder
IBM Linux Technology Center

Signed-off-by: Hanna Linder <[email protected]>
Signed-off-by: Maximilian Attems <[email protected]>

---

Signed-off-by: Domen Puncer <[email protected]>
---


kj-domen/arch/i386/pci/i386.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff -puN arch/i386/pci/i386.c~for-each-pci-dev-arch_i386_pci_i386 arch/i386/pci/i386.c
--- kj/arch/i386/pci/i386.c~for-each-pci-dev-arch_i386_pci_i386 2005-01-10 17:59:57.000000000 +0100
+++ kj-domen/arch/i386/pci/i386.c 2005-01-10 17:59:57.000000000 +0100
@@ -124,7 +124,7 @@ static void __init pcibios_allocate_reso
u16 command;
struct resource *r, *pr;

- while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
+ for_each_pci_dev(dev) {
pci_read_config_word(dev, PCI_COMMAND, &command);
for(idx = 0; idx < 6; idx++) {
r = &dev->resource[idx];
@@ -168,7 +168,7 @@ static int __init pcibios_assign_resourc
int idx;
struct resource *r;

- while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
+ for_each_pci_dev(dev) {
int class = dev->class >> 8;

/* Don't touch classless devices and host bridges */
_


2005-01-12 00:54:08

by Dave Jones

[permalink] [raw]
Subject: Re: [patch 03/11] arch/i386/pci/i386.c: Use new for_each_pci_dev macro

On Wed, Jan 12, 2005 at 12:34:58AM +0100, [email protected] wrote:

> As requested by Christoph Hellwig I've created a new macro called
> for_each_pci_dev. It is a wrapper for this common use of pci_get/find_device:
>
> (while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL))
>
> This macro will return the pci_dev *for all pci devices. Here is the first patch I
> used to test this macro with. Compiled and booted on my T23. There will be
> 53 more patches using this new macro.

Which looks just like the pci_for_each_dev we used to have.
That function got removed due some shortcoming or other that I never
fully understood, but ISTR it had something to do with locking.
(why it couldnt be hidden inside for_each_pci_dev is a mystery to me)

We've had lots of code in the kernel go from this..

pci_for_each_dev(loop_dev) {

to the disgustingly unreadable..

while ((loop_dev = pci_find_device(PCI_ANY_ID, PCI_ANY_ID, loop_dev)) != NULL) {

and now its going to ..

for_each_pci_dev(loop_dev) {

So,.. what has all this churn bought us, and where does it end ?
With four words in the function name, we've enough possibilities
for quite a few more iterations yet.

Dave

2005-01-12 01:16:24

by Greg KH

[permalink] [raw]
Subject: Re: [patch 03/11] arch/i386/pci/i386.c: Use new for_each_pci_dev macro

On Tue, Jan 11, 2005 at 07:46:19PM -0500, Dave Jones wrote:
> On Wed, Jan 12, 2005 at 12:34:58AM +0100, [email protected] wrote:
>
> > As requested by Christoph Hellwig I've created a new macro called
> > for_each_pci_dev. It is a wrapper for this common use of pci_get/find_device:
> >
> > (while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL))
> >
> > This macro will return the pci_dev *for all pci devices. Here is the first patch I
> > used to test this macro with. Compiled and booted on my T23. There will be
> > 53 more patches using this new macro.
>
> Which looks just like the pci_for_each_dev we used to have.
> That function got removed due some shortcoming or other that I never
> fully understood, but ISTR it had something to do with locking.
> (why it couldnt be hidden inside for_each_pci_dev is a mystery to me)
>
> We've had lots of code in the kernel go from this..
>
> pci_for_each_dev(loop_dev) {

Which did not have any locks at all, and was broken for hotplug systems.

> to the disgustingly unreadable..
>
> while ((loop_dev = pci_find_device(PCI_ANY_ID, PCI_ANY_ID, loop_dev)) != NULL) {

Yeah, blame me for that, but it was race proof for hotplug boxes.

> and now its going to ..
>
> for_each_pci_dev(loop_dev) {

Which is because I didn't think of that in the first place, sorry.

> So,.. what has all this churn bought us, and where does it end ?
> With four words in the function name, we've enough possibilities
> for quite a few more iterations yet.

We now have a simple macro to iterate over all pci devices, in a
reference count and lock safe manner.

The next change will be to just delete the thing alltogether, as most
all drivers shouldn't be walking the list of pci devices in the first
place.

Yeah yeah yeah, I know I can't really do that, as there are lots of
places where it is valid to do so, but I can dream...

thanks,

greg k-h