Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jeff Garzik]

Jiri Slaby wrote:
> bcm43xx avoid pci_find_device
> 
> Change pci_find_device to safer pci_get_device with support for more
> devices.
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> 
> ---
> commit 1d3b6caf027fe53351c645523587aeac40bc3e47
> tree ae37c86b633442cdf8a7a19ac287542724081c90
> parent ab3443d79c94d0ae6a9e020daefa4d29eccff50d
> author Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006 01:49:12 +0159
> committer Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006 01:49:12 +0159
> 
>  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   20 ++++++++++++++++----
>  1 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> index b488f77..56d2fc6 100644
> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> @@ -2131,6 +2131,13 @@ out:
>  	return err;
>  }
>  
> +#ifdef CONFIG_BCM947XX
> +static struct pci_device_id bcm43xx_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
> +	{ 0 }
> +};
> +#endif
> +
>  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
>  {
>  	int res;
> @@ -2141,10 +2148,15 @@ static int bcm43xx_initialize_irq(struct
>  #ifdef CONFIG_BCM947XX
>  	if (bcm->pci_dev->bus->number == 0) {
>  		struct pci_dev *d = NULL;
> -		/* FIXME: we will probably need more device IDs here... */
> -		d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
> -		if (d != NULL) {
> -			bcm->irq = d->irq;
> +		struct pci_device_id *id = bcm43xx_ids;
> +		while (id->vendor) {
> +			d = pci_get_device(id->vendor, id->device, NULL);
> +			if (d != NULL) {
> +				bcm->irq = d->irq;
> +				pci_dev_put(d);
> +				break;

You'll want to use pci_match_device() or pci_match_one_device()
[I forget which one]

	Jeff

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jiri Slaby]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeff Garzik napsal(a):
> Jiri Slaby wrote:
>> bcm43xx avoid pci_find_device
>>
>> Change pci_find_device to safer pci_get_device with support for more
>> devices.
>>
>> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>>
>> ---
>> commit 1d3b6caf027fe53351c645523587aeac40bc3e47
>> tree ae37c86b633442cdf8a7a19ac287542724081c90
>> parent ab3443d79c94d0ae6a9e020daefa4d29eccff50d
>> author Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006 01:49:12
>> +0159
>> committer Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006
>> 01:49:12 +0159
>>
>>  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   20 ++++++++++++++++----
>>  1 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> index b488f77..56d2fc6 100644
>> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> @@ -2131,6 +2131,13 @@ out:
>>      return err;
>>  }
>>  
>> +#ifdef CONFIG_BCM947XX
>> +static struct pci_device_id bcm43xx_ids[] = {
>> +    { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
>> +    { 0 }
>> +};
>> +#endif
>> +
>>  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
>>  {
>>      int res;
>> @@ -2141,10 +2148,15 @@ static int bcm43xx_initialize_irq(struct
>>  #ifdef CONFIG_BCM947XX
>>      if (bcm->pci_dev->bus->number == 0) {
>>          struct pci_dev *d = NULL;
>> -        /* FIXME: we will probably need more device IDs here... */
>> -        d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
>> -        if (d != NULL) {
>> -            bcm->irq = d->irq;
>> +        struct pci_device_id *id = bcm43xx_ids;
>> +        while (id->vendor) {
>> +            d = pci_get_device(id->vendor, id->device, NULL);
>> +            if (d != NULL) {
>> +                bcm->irq = d->irq;
>> +                pci_dev_put(d);
>> +                break;
> 
> You'll want to use pci_match_device() or pci_match_one_device()
> [I forget which one]
Why? Matching is done by pci_get_device() or pci_get_subsys(), respectively.
[pci_match_device() is for matching dev <-> drv, you meant pci_match_one_device()]

thanks,
- --
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFEdtY+MsxVwznUen4RAkmFAJ9FFm6nCgnGCdZAcPqv2H99rBNMzwCeK3DA
nPBv8s+ldDrSOpin+mGdDdg=
=MBag
-----END PGP SIGNATURE-----

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jeff Garzik]

Jiri Slaby wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Jeff Garzik napsal(a):
>> Jiri Slaby wrote:
>>> bcm43xx avoid pci_find_device
>>>
>>> Change pci_find_device to safer pci_get_device with support for more
>>> devices.
>>>
>>> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>>>
>>> ---
>>> commit 1d3b6caf027fe53351c645523587aeac40bc3e47
>>> tree ae37c86b633442cdf8a7a19ac287542724081c90
>>> parent ab3443d79c94d0ae6a9e020daefa4d29eccff50d
>>> author Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006 01:49:12
>>> +0159
>>> committer Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006
>>> 01:49:12 +0159
>>>
>>>  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   20 ++++++++++++++++----
>>>  1 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>> b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>> index b488f77..56d2fc6 100644
>>> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>> @@ -2131,6 +2131,13 @@ out:
>>>      return err;
>>>  }
>>>  
>>> +#ifdef CONFIG_BCM947XX
>>> +static struct pci_device_id bcm43xx_ids[] = {
>>> +    { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
>>> +    { 0 }
>>> +};
>>> +#endif
>>> +
>>>  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
>>>  {
>>>      int res;
>>> @@ -2141,10 +2148,15 @@ static int bcm43xx_initialize_irq(struct
>>>  #ifdef CONFIG_BCM947XX
>>>      if (bcm->pci_dev->bus->number == 0) {
>>>          struct pci_dev *d = NULL;
>>> -        /* FIXME: we will probably need more device IDs here... */
>>> -        d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
>>> -        if (d != NULL) {
>>> -            bcm->irq = d->irq;
>>> +        struct pci_device_id *id = bcm43xx_ids;
>>> +        while (id->vendor) {
>>> +            d = pci_get_device(id->vendor, id->device, NULL);
>>> +            if (d != NULL) {
>>> +                bcm->irq = d->irq;
>>> +                pci_dev_put(d);
>>> +                break;
>> You'll want to use pci_match_device() or pci_match_one_device()
>> [I forget which one]
> Why? Matching is done by pci_get_device() or pci_get_subsys(), respectively.
> [pci_match_device() is for matching dev <-> drv, you meant pci_match_one_device()]

The FIXME says "we will probably need more device IDs here."

Thus, if you are touching this area, it would make sense to add the 
capability to easily add a second (and third, fourth...) PCI ID.  And 
that means pci_match_one_device() and a pci_device_id table.

	Jeff

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jiri Slaby]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeff Garzik napsal(a):
> Jiri Slaby wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Jeff Garzik napsal(a):
>>> Jiri Slaby wrote:
>>>> bcm43xx avoid pci_find_device
>>>>
>>>> Change pci_find_device to safer pci_get_device with support for more
>>>> devices.
>>>>
>>>> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>>>>
>>>> ---
>>>> commit 1d3b6caf027fe53351c645523587aeac40bc3e47
>>>> tree ae37c86b633442cdf8a7a19ac287542724081c90
>>>> parent ab3443d79c94d0ae6a9e020daefa4d29eccff50d
>>>> author Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006 01:49:12
>>>> +0159
>>>> committer Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006
>>>> 01:49:12 +0159
>>>>
>>>>  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   20
>>>> ++++++++++++++++----
>>>>  1 files changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>> b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>> index b488f77..56d2fc6 100644
>>>> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>> @@ -2131,6 +2131,13 @@ out:
>>>>      return err;
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_BCM947XX
>>>> +static struct pci_device_id bcm43xx_ids[] = {
>>>> +    { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
>>>> +    { 0 }
>>>> +};
Table is here ^^^. You just add an entry, and that's it.
>>>> +#endif
>>>> +
>>>>  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
>>>>  {
>>>>      int res;
>>>> @@ -2141,10 +2148,15 @@ static int bcm43xx_initialize_irq(struct
>>>>  #ifdef CONFIG_BCM947XX
>>>>      if (bcm->pci_dev->bus->number == 0) {
>>>>          struct pci_dev *d = NULL;
>>>> -        /* FIXME: we will probably need more device IDs here... */
>>>> -        d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
>>>> -        if (d != NULL) {
>>>> -            bcm->irq = d->irq;
>>>> +        struct pci_device_id *id = bcm43xx_ids;
>>>> +        while (id->vendor) {
>>>> +            d = pci_get_device(id->vendor, id->device, NULL);
>>>> +            if (d != NULL) {
>>>> +                bcm->irq = d->irq;
>>>> +                pci_dev_put(d);
>>>> +                break;
>>> You'll want to use pci_match_device() or pci_match_one_device()
>>> [I forget which one]
>> Why? Matching is done by pci_get_device() or pci_get_subsys(),
>> respectively.
>> [pci_match_device() is for matching dev <-> drv, you meant
>> pci_match_one_device()]
> 
> The FIXME says "we will probably need more device IDs here."
Yup.
> 
> Thus, if you are touching this area, it would make sense to add the
> capability to easily add a second (and third, fourth...) PCI ID.  And
> that means pci_match_one_device() and a pci_device_id table.
But the while loop do the work: unless id->vendor != NULL, do the matching with
the current raw (id) of the table, then jump to the next raw (id++).

pci_get_device returns NULL if the device with id->vendor, id->device wasn't
found, then we try next raw, otherwise, we break the loop.

Implementations before and now do the same strangeness -- assume there is only
one device (?shouldn't matter?, since it is embedded).

cheers,
- --
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFEdtlbMsxVwznUen4RAo/OAJsHy6sED+a9QYmbcaGTMUjwSYm4vACgwfQL
GhmfbtwskPB3Dnvw8HfJzpE=
=+kxv
-----END PGP SIGNATURE-----

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jeff Garzik]

Jiri Slaby wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Jeff Garzik napsal(a):
>> Jiri Slaby wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> Jeff Garzik napsal(a):
>>>> Jiri Slaby wrote:
>>>>> bcm43xx avoid pci_find_device
>>>>>
>>>>> Change pci_find_device to safer pci_get_device with support for more
>>>>> devices.
>>>>>
>>>>> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>>>>>
>>>>> ---
>>>>> commit 1d3b6caf027fe53351c645523587aeac40bc3e47
>>>>> tree ae37c86b633442cdf8a7a19ac287542724081c90
>>>>> parent ab3443d79c94d0ae6a9e020daefa4d29eccff50d
>>>>> author Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006 01:49:12
>>>>> +0159
>>>>> committer Jiri Slaby <ku@bellona.localdomain> Fri, 26 May 2006
>>>>> 01:49:12 +0159
>>>>>
>>>>>  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   20
>>>>> ++++++++++++++++----
>>>>>  1 files changed, 16 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>>> b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>>> index b488f77..56d2fc6 100644
>>>>> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>>> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>>> @@ -2131,6 +2131,13 @@ out:
>>>>>      return err;
>>>>>  }
>>>>>  
>>>>> +#ifdef CONFIG_BCM947XX
>>>>> +static struct pci_device_id bcm43xx_ids[] = {
>>>>> +    { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
>>>>> +    { 0 }
>>>>> +};
> Table is here ^^^. You just add an entry, and that's it.
>>>>> +#endif
>>>>> +
>>>>>  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
>>>>>  {
>>>>>      int res;
>>>>> @@ -2141,10 +2148,15 @@ static int bcm43xx_initialize_irq(struct
>>>>>  #ifdef CONFIG_BCM947XX
>>>>>      if (bcm->pci_dev->bus->number == 0) {
>>>>>          struct pci_dev *d = NULL;
>>>>> -        /* FIXME: we will probably need more device IDs here... */
>>>>> -        d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
>>>>> -        if (d != NULL) {
>>>>> -            bcm->irq = d->irq;
>>>>> +        struct pci_device_id *id = bcm43xx_ids;
>>>>> +        while (id->vendor) {
>>>>> +            d = pci_get_device(id->vendor, id->device, NULL);
>>>>> +            if (d != NULL) {
>>>>> +                bcm->irq = d->irq;
>>>>> +                pci_dev_put(d);
>>>>> +                break;
>>>> You'll want to use pci_match_device() or pci_match_one_device()
>>>> [I forget which one]
>>> Why? Matching is done by pci_get_device() or pci_get_subsys(),
>>> respectively.
>>> [pci_match_device() is for matching dev <-> drv, you meant
>>> pci_match_one_device()]
>> The FIXME says "we will probably need more device IDs here."
> Yup.
>> Thus, if you are touching this area, it would make sense to add the
>> capability to easily add a second (and third, fourth...) PCI ID.  And
>> that means pci_match_one_device() and a pci_device_id table.
> But the while loop do the work: unless id->vendor != NULL, do the matching with
> the current raw (id) of the table, then jump to the next raw (id++).
> 
> pci_get_device returns NULL if the device with id->vendor, id->device wasn't
> found, then we try next raw, otherwise, we break the loop.
> 
> Implementations before and now do the same strangeness -- assume there is only
> one device (?shouldn't matter?, since it is embedded).

The point is that you don't need to loop over the table, 
pci_match_one_device() does that for you.

And this code, like the gt96100_eth code, is testing the existence of 
certain platform devices, to be certain that it can proceed with certain 
platform-specific duties.

Thus we don't care about matching multiple devices -- an unlikely case 
-- but we do care about making the code as small as possible by calling 
a standard PCI match function which searches through a list of PCI IDs.

	Jeff

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jiri Slaby]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeff Garzik napsal(a):
> The point is that you don't need to loop over the table,
> pci_match_one_device() does that for you.
The problem is, that there is no such function, I think.
If you take a look at pci_dev_present:
http://sosdg.org/~coywolf/lxr/source/drivers/pci/search.c#L270
They traverse the table in similar way as I do.

pci_match_one_device() just checks (one to one) values without any looping.

regards,
- --
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFEdt5HMsxVwznUen4RAqt8AJ9pzaDey2zn399lrahelv17w8IiDgCguUwa
4xOX7pUX2Au/WBsbJbnNwBE=
=P1cu
-----END PGP SIGNATURE-----

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jeff Garzik]

Jiri Slaby wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Jeff Garzik napsal(a):
>> The point is that you don't need to loop over the table,
>> pci_match_one_device() does that for you.
> The problem is, that there is no such function, I think.
> If you take a look at pci_dev_present:

The function you want is pci_dev_present().

	Jeff

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jiri Slaby]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeff Garzik napsal(a):
> Jiri Slaby wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Jeff Garzik napsal(a):
>>> The point is that you don't need to loop over the table,
>>> pci_match_one_device() does that for you.
>> The problem is, that there is no such function, I think.
>> If you take a look at pci_dev_present:
> 
> The function you want is pci_dev_present().
Nope, it returns only 0/1.

regards,
- --
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFEduafMsxVwznUen4RAjfzAKCaxRAK1nN5qx+akiA59E5Mq/ZPcgCffRwa
vwAz0SPClr6sCYy+DOjtilE=
=DHzA
-----END PGP SIGNATURE-----

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[John W. Linville]

On Fri, May 26, 2006 at 01:29:12PM +0159, Jiri Slaby wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Jeff Garzik napsal(a):
> > Jiri Slaby wrote:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> Jeff Garzik napsal(a):
> >>> The point is that you don't need to loop over the table,
> >>> pci_match_one_device() does that for you.
> >> The problem is, that there is no such function, I think.
> >> If you take a look at pci_dev_present:
> > 
> > The function you want is pci_dev_present().
> Nope, it returns only 0/1.

Did we get a resolution on this?  I don't think Jeff is going to pull
this patch from me until you satisfy him that it is correct... :-)

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jiri Slaby]

John W. Linville napsal(a):
> On Fri, May 26, 2006 at 01:29:12PM +0159, Jiri Slaby wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Jeff Garzik napsal(a):
>>> Jiri Slaby wrote:
>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>> Hash: SHA1
>>>>
>>>> Jeff Garzik napsal(a):
>>>>> The point is that you don't need to loop over the table,
>>>>> pci_match_one_device() does that for you.
>>>> The problem is, that there is no such function, I think.
>>>> If you take a look at pci_dev_present:
>>> The function you want is pci_dev_present().
>> Nope, it returns only 0/1.
> 
> Did we get a resolution on this?  I don't think Jeff is going to pull
> this patch from me until you satisfy him that it is correct... :-)
Yup: to use pci_get_device in some nice loop.
But unfortunately here is no time now, that's the problem, I'll post corrected
ones next month, sorry.

regards,
-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Michael Buesch]

On Friday 26 May 2006 12:33, you wrote:
> >>>> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> >>>> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> >>>> @@ -2131,6 +2131,13 @@ out:
> >>>>      return err;
> >>>>  }
> >>>>
> >>>> +#ifdef CONFIG_BCM947XX
> >>>> +static struct pci_device_id bcm43xx_ids[] = {

Call it
static struct pci_device_id bcm43xx_47xx_ids[] = {
please.

And; _important_; if you submit this change, _also_
do a patch against the devicescape version of the driver in
John Linville's wireless-dev tree
drivers/net/wireless/d80211/bcm43xx
in the tree at git://kernel.org/pub/scm/linux/kernel/git/linville/wireless-dev.git

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jiri Slaby]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Michael Buesch napsal(a):
> On Friday 26 May 2006 12:33, you wrote:
>>>>>> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>>>> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>>>>>> @@ -2131,6 +2131,13 @@ out:
>>>>>>      return err;
>>>>>>  }
>>>>>>
>>>>>> +#ifdef CONFIG_BCM947XX
>>>>>> +static struct pci_device_id bcm43xx_ids[] = {
> 
> Call it
> static struct pci_device_id bcm43xx_47xx_ids[] = {
> please.
> 
> And; _important_; if you submit this change, _also_
> do a patch against the devicescape version of the driver in
> John Linville's wireless-dev tree
> drivers/net/wireless/d80211/bcm43xx
> in the tree at git://kernel.org/pub/scm/linux/kernel/git/linville/wireless-dev.git
Ok, thanks.

- --
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFEduvXMsxVwznUen4RAqQcAJ9j870AGMn5jXW68tEQHZXltAenmQCfX9Ik
oyRfuNnKxGHu8HGVvcDVJHM=
=/NUD
-----END PGP SIGNATURE-----

[PATCH 0/3 #3] avoid pci_find_device

[Jiri Slaby]

Hello,

there are some patches to avoid pci_find_device in drivers.
I will make a bcm43xx patch against wireless git too.

Take #3.

It's against 2.6.17-rc5-mm3 tree.

01-i2c-scx200-acb-avoid-pci-find-device.patch
02-bcm43xx-avoid-pci-find-device.patch
03-gt96100eth-avoid-pci-find-device.patch

 i2c/busses/scx200_acb.c             |    9 ++++++---
 net/gt96100eth.c                    |   23 ++++++++++++++++++-----
 net/wireless/bcm43xx/bcm43xx_main.c |   21 ++++++++++++++++-----
 3 files changed, 40 insertions(+), 13 deletions(-)

Thanks.

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Greg KH]

On Mon, Jun 05, 2006 at 10:20:07PM +0200, Jiri Slaby wrote:
> bcm43xx avoid pci_find_device
> 
> Change pci_find_device to safer pci_get_device with support for more
> devices.
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> 
> ---
> commit 4b73c16f5411d97360d5f26f292ffddeb670ff75
> tree 6e43c8bd02498eb1ceec6bdc64277fa8408da9e2
> parent d59f9ea8489749f59cd0c7333a4784cab964daa8
> author Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
> committer Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
> 
>  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   21 ++++++++++++++++-----
>  1 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> index 22b8fa6..d1a9975 100644
> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> @@ -2133,6 +2133,13 @@ out:
>  	return err;
>  }
>  
> +#ifdef CONFIG_BCM947XX
> +static struct pci_device_id bcm43xx_47xx_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
> +	{ 0 }
> +};
> +#endif
> +
>  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
>  {
>  	int res;
> @@ -2142,11 +2149,15 @@ static int bcm43xx_initialize_irq(struct
>  	bcm->irq = bcm->pci_dev->irq;
>  #ifdef CONFIG_BCM947XX
>  	if (bcm->pci_dev->bus->number == 0) {
> -		struct pci_dev *d = NULL;
> -		/* FIXME: we will probably need more device IDs here... */
> -		d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
> -		if (d != NULL) {
> -			bcm->irq = d->irq;
> +		struct pci_dev *d;
> +		struct pci_device_id *id;
> +		for (id = bcm43xx_47xx_ids; id->vendor; id++) {
> +			d = pci_get_device(id->vendor, id->device, NULL);
> +			if (d != NULL) {
> +				bcm->irq = d->irq;
> +				pci_dev_put(d);
> +				break;
> +			}

This will not work if you have more than one of the same devices in the
system.

Well, the original code will not either :(

Why not just use the proper pci interface?  Why poke around in another
pci device to steal an irq, when that irq might not even be valid?
(irqs are not valid until pci_enable_device() is called on them...)

thanks,

greg k-h

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jiri Slaby]

Greg KH napsal(a):
> On Mon, Jun 05, 2006 at 10:20:07PM +0200, Jiri Slaby wrote:
>> bcm43xx avoid pci_find_device
>>
>> Change pci_find_device to safer pci_get_device with support for more
>> devices.
>>
>> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>>
>> ---
>> commit 4b73c16f5411d97360d5f26f292ffddeb670ff75
>> tree 6e43c8bd02498eb1ceec6bdc64277fa8408da9e2
>> parent d59f9ea8489749f59cd0c7333a4784cab964daa8
>> author Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
>> committer Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
>>
>>  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   21 ++++++++++++++++-----
>>  1 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> index 22b8fa6..d1a9975 100644
>> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> @@ -2133,6 +2133,13 @@ out:
>>  	return err;
>>  }
>>  
>> +#ifdef CONFIG_BCM947XX
>> +static struct pci_device_id bcm43xx_47xx_ids[] = {
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
>> +	{ 0 }
>> +};
>> +#endif
>> +
>>  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
>>  {
>>  	int res;
>> @@ -2142,11 +2149,15 @@ static int bcm43xx_initialize_irq(struct
>>  	bcm->irq = bcm->pci_dev->irq;
>>  #ifdef CONFIG_BCM947XX
>>  	if (bcm->pci_dev->bus->number == 0) {
>> -		struct pci_dev *d = NULL;
>> -		/* FIXME: we will probably need more device IDs here... */
>> -		d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
>> -		if (d != NULL) {
>> -			bcm->irq = d->irq;
>> +		struct pci_dev *d;
>> +		struct pci_device_id *id;
>> +		for (id = bcm43xx_47xx_ids; id->vendor; id++) {
>> +			d = pci_get_device(id->vendor, id->device, NULL);
>> +			if (d != NULL) {
>> +				bcm->irq = d->irq;
>> +				pci_dev_put(d);
>> +				break;
>> +			}
> 
> This will not work if you have more than one of the same devices in the
> system.
> 
> Well, the original code will not either :(
> 
> Why not just use the proper pci interface?  Why poke around in another
> pci device to steal an irq, when that irq might not even be valid?
> (irqs are not valid until pci_enable_device() is called on them...)
Ok, this is some bus or something, not "real" device, so no pci probing or
anything else could be done. There is only one in the system, if any, since
CONFIG_BCM947XX is set for some sort of embedded systems, but this would be
uttered by somebody more responsible.

Should I call pci_enable_device anyway, I am confused now :/? And then, should I
hold the reference all time until ifdown for this "some kind of parent"?

thanks,
-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jeff Garzik]

On Mon, Jun 05, 2006 at 01:53:09PM -0700, Greg KH wrote:
> Why not just use the proper pci interface?  Why poke around in another
> pci device to steal an irq, when that irq might not even be valid?
> (irqs are not valid until pci_enable_device() is called on them...)

Answered this question the last time you asked.

Answer:  this is an embedded platform that needs such poking.  The
wireless device is _another_ device.

	Jeff

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Greg KH]

On Mon, Jun 05, 2006 at 09:18:18PM -0400, Jeff Garzik wrote:
> On Mon, Jun 05, 2006 at 01:53:09PM -0700, Greg KH wrote:
> > Why not just use the proper pci interface?  Why poke around in another
> > pci device to steal an irq, when that irq might not even be valid?
> > (irqs are not valid until pci_enable_device() is called on them...)
> 
> Answered this question the last time you asked.
> 
> Answer:  this is an embedded platform that needs such poking.  The
> wireless device is _another_ device.

Ugh, sorry, too many patches, too many different threads...  You are
right...

greg k-h

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Michael Buesch]

On Monday 05 June 2006 22:53, Greg KH wrote:
> On Mon, Jun 05, 2006 at 10:20:07PM +0200, Jiri Slaby wrote:
> > bcm43xx avoid pci_find_device
> > 
> > Change pci_find_device to safer pci_get_device with support for more
> > devices.
> > 
> > Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> > 
> > ---
> > commit 4b73c16f5411d97360d5f26f292ffddeb670ff75
> > tree 6e43c8bd02498eb1ceec6bdc64277fa8408da9e2
> > parent d59f9ea8489749f59cd0c7333a4784cab964daa8
> > author Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
> > committer Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
> > 
> >  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   21 ++++++++++++++++-----
> >  1 files changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> > index 22b8fa6..d1a9975 100644
> > --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> > +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> > @@ -2133,6 +2133,13 @@ out:
> >  	return err;
> >  }
> >  
> > +#ifdef CONFIG_BCM947XX
> > +static struct pci_device_id bcm43xx_47xx_ids[] = {
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
> > +	{ 0 }
> > +};
> > +#endif
> > +
> >  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
> >  {
> >  	int res;
> > @@ -2142,11 +2149,15 @@ static int bcm43xx_initialize_irq(struct
> >  	bcm->irq = bcm->pci_dev->irq;
> >  #ifdef CONFIG_BCM947XX
> >  	if (bcm->pci_dev->bus->number == 0) {
> > -		struct pci_dev *d = NULL;
> > -		/* FIXME: we will probably need more device IDs here... */
> > -		d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
> > -		if (d != NULL) {
> > -			bcm->irq = d->irq;
> > +		struct pci_dev *d;
> > +		struct pci_device_id *id;
> > +		for (id = bcm43xx_47xx_ids; id->vendor; id++) {
> > +			d = pci_get_device(id->vendor, id->device, NULL);
> > +			if (d != NULL) {
> > +				bcm->irq = d->irq;
> > +				pci_dev_put(d);
> > +				break;
> > +			}
> 
> This will not work if you have more than one of the same devices in the
> system.
> 
> Well, the original code will not either :(
> 
> Why not just use the proper pci interface?  Why poke around in another
> pci device to steal an irq, when that irq might not even be valid?
> (irqs are not valid until pci_enable_device() is called on them...)

Ok, if someone really wants to have this patch in mainline.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

But the whole purpose of this patch is really questionable.
* This code is only compiled for the OpenWRT Router kernel.
  This kernel does not use softmac, but dscape stack.
  So nobody will ever actually use this code.
  One could even argue, if the code should be removed from
  the softmac driver, but I think openwrt people use it for
  some kind of hacking, testing, whatever.
  It's not so much code, so I don't care. It does not add
  maintainance work.
* Do we really need to increment some reference counters?
  I mean, we are asking for a bus here. This bus is not
  hotpluggable or something and will never go away. Either it
  is soldered on the board or not. That is what we test here.

So, Jiri, if you really want to have this patch upstream, you
have my Signed-off-by. If nobody else cares, it will get lost
in the deep black netdev hole. ;)

-- 
Greetings Michael.

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Michael Buesch]

On Monday 05 June 2006 22:18, Jiri Slaby wrote:
> bcm43xx avoid pci_find_device
> 
> Change pci_find_device to safer pci_get_device with support for more
> devices.

I am wondering about the reference count.
>From docbook:

256  * pci_get_device - begin or continue searching for a PCI device by vendor/device id
257  * @vendor: PCI vendor id to match, or %PCI_ANY_ID to match all vendor ids
258  * @device: PCI device id to match, or %PCI_ANY_ID to match all device ids
259  * @from: Previous PCI device found in search, or %NULL for new search.
260  *
261  * Iterates through the list of known PCI devices.  If a PCI device is
262  * found with a matching @vendor and @device, the reference count to the
                                                      ^^^^^^^^^^^^^^^^^^^^^^
263  * device is incremented and a pointer to its device structure is returned.
       ^^^^^^^^^^^^^^^^^^^^^
264  * Otherwise, %NULL is returned.  A new search is initiated by passing %NULL
265  * to the @from argument.  Otherwise if @from is not %NULL, searches continue
266  * from next device on the global list.  The reference count for @from is
267  * always decremented if it is not %NULL.

Who is going to decrement it, once the device is not used anymore.
"not used anymore" is ifconfig down in the case of bcm43xx.
You will call pci_get_device on each ifconfig up.

> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> 
> ---
> commit 4b73c16f5411d97360d5f26f292ffddeb670ff75
> tree 6e43c8bd02498eb1ceec6bdc64277fa8408da9e2
> parent d59f9ea8489749f59cd0c7333a4784cab964daa8
> author Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
> committer Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
> 
>  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   21 ++++++++++++++++-----
>  1 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> index 22b8fa6..d1a9975 100644
> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
> @@ -2133,6 +2133,13 @@ out:
>  	return err;
>  }
>  
> +#ifdef CONFIG_BCM947XX
> +static struct pci_device_id bcm43xx_47xx_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
> +	{ 0 }
> +};
> +#endif
> +
>  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
>  {
>  	int res;
> @@ -2142,11 +2149,15 @@ static int bcm43xx_initialize_irq(struct
>  	bcm->irq = bcm->pci_dev->irq;
>  #ifdef CONFIG_BCM947XX
>  	if (bcm->pci_dev->bus->number == 0) {
> -		struct pci_dev *d = NULL;
> -		/* FIXME: we will probably need more device IDs here... */
> -		d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
> -		if (d != NULL) {
> -			bcm->irq = d->irq;
> +		struct pci_dev *d;
> +		struct pci_device_id *id;
> +		for (id = bcm43xx_47xx_ids; id->vendor; id++) {
> +			d = pci_get_device(id->vendor, id->device, NULL);
> +			if (d != NULL) {
> +				bcm->irq = d->irq;
> +				pci_dev_put(d);
> +				break;
> +			}
>  		}
>  	}
>  #endif
> 

-- 
Greetings Michael.

Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device

[Jiri Slaby]

Michael Buesch napsal(a):
> On Monday 05 June 2006 22:18, Jiri Slaby wrote:
>> bcm43xx avoid pci_find_device
>>
>> Change pci_find_device to safer pci_get_device with support for more
>> devices.
> 
> I am wondering about the reference count.
> From docbook:
> 
> 256  * pci_get_device - begin or continue searching for a PCI device by vendor/device id
> 257  * @vendor: PCI vendor id to match, or %PCI_ANY_ID to match all vendor ids
> 258  * @device: PCI device id to match, or %PCI_ANY_ID to match all device ids
> 259  * @from: Previous PCI device found in search, or %NULL for new search.
> 260  *
> 261  * Iterates through the list of known PCI devices.  If a PCI device is
> 262  * found with a matching @vendor and @device, the reference count to the
>                                                       ^^^^^^^^^^^^^^^^^^^^^^
> 263  * device is incremented and a pointer to its device structure is returned.
>        ^^^^^^^^^^^^^^^^^^^^^
> 264  * Otherwise, %NULL is returned.  A new search is initiated by passing %NULL
> 265  * to the @from argument.  Otherwise if @from is not %NULL, searches continue
> 266  * from next device on the global list.  The reference count for @from is
> 267  * always decremented if it is not %NULL.
> 
> Who is going to decrement it, once the device is not used anymore.
> "not used anymore" is ifconfig down in the case of bcm43xx.
> You will call pci_get_device on each ifconfig up.
> 
>> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>>
>> ---
>> commit 4b73c16f5411d97360d5f26f292ffddeb670ff75
>> tree 6e43c8bd02498eb1ceec6bdc64277fa8408da9e2
>> parent d59f9ea8489749f59cd0c7333a4784cab964daa8
>> author Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
>> committer Jiri Slaby <ku@bellona.localdomain> Mon, 05 Jun 2006 22:01:03 +0159
>>
>>  drivers/net/wireless/bcm43xx/bcm43xx_main.c |   21 ++++++++++++++++-----
>>  1 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_main.c b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> index 22b8fa6..d1a9975 100644
>> --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c
>> @@ -2133,6 +2133,13 @@ out:
>>  	return err;
>>  }
>>  
>> +#ifdef CONFIG_BCM947XX
>> +static struct pci_device_id bcm43xx_47xx_ids[] = {
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4324) },
>> +	{ 0 }
>> +};
>> +#endif
>> +
>>  static int bcm43xx_initialize_irq(struct bcm43xx_private *bcm)
>>  {
>>  	int res;
>> @@ -2142,11 +2149,15 @@ static int bcm43xx_initialize_irq(struct
>>  	bcm->irq = bcm->pci_dev->irq;
>>  #ifdef CONFIG_BCM947XX
>>  	if (bcm->pci_dev->bus->number == 0) {
>> -		struct pci_dev *d = NULL;
>> -		/* FIXME: we will probably need more device IDs here... */
>> -		d = pci_find_device(PCI_VENDOR_ID_BROADCOM, 0x4324, NULL);
>> -		if (d != NULL) {
>> -			bcm->irq = d->irq;
>> +		struct pci_dev *d;
>> +		struct pci_device_id *id;
>> +		for (id = bcm43xx_47xx_ids; id->vendor; id++) {
>> +			d = pci_get_device(id->vendor, id->device, NULL);
>> +			if (d != NULL) {
>> +				bcm->irq = d->irq;
>> +				pci_dev_put(d);
here?
>> +				break;
>> +			}
>>  		}
>>  	}
>>  #endif
>>
> 

regards,
-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E