[PATCH 0/3 #3] avoid pci_find_device

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