Re: [PATCH 3/91] get rid of pci_find_{device,subsys} arch/ia64

Re: [PATCH 3/91] get rid of pci_find_{device,subsys} arch/ia64

[Christoph Hellwig]

On Tue, Mar 29, 2005 at 05:50:09PM +0530, Amit Gud wrote:
> Replace pci_find_{device,subsys} with pci_get_{device,subsys}.

should be for_each_pci_dev

[PATCH 3/91] get rid of pci_find_{device,subsys} arch/ia64

[Amit Gud]

Replace pci_find_{device,subsys} with pci_get_{device,subsys}.



Signed-off-by: Amit Gud <gud@eth.net>

---
diff -upr orig-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c work-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c
--- orig-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c	2005-03-15 21:00:10.000000000 +0530
+++ work-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c	2005-03-24 15:29:23.539955472 +0530
@@ -365,8 +365,9 @@ static int __init sn_pci_init(void)
 	 */
 
 	while ((pci_dev -		pci_find_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev)) != NULL) {
+		pci_get_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev)) != NULL) {
 		sn_pci_fixup_slot(pci_dev);
+		pci_dev_put(dev);
 	}
 
 	sn_ioif_inited = 1;	/* sn I/O infrastructure now initialized */

Re: [PATCH 3/91] get rid of pci_find_{device,subsys} arch/ia64

[Amit Gud]

Christoph Hellwig wrote:
> On Tue, Mar 29, 2005 at 05:50:09PM +0530, Amit Gud wrote:
> 
>>Replace pci_find_{device,subsys} with pci_get_{device,subsys}.
> 
> 
> should be for_each_pci_dev
> 

Yes, will address such replacements seperately.


Thanks,
AG
--
May the source be with you.

Re: [PATCH 3/91] get rid of pci_find_{device,subsys} arch/ia64

[Greg KH]

On Tue, Mar 29, 2005 at 05:50:09PM +0530, Amit Gud wrote:
> Replace pci_find_{device,subsys} with pci_get_{device,subsys}.
> 
> 
> 
> Signed-off-by: Amit Gud <gud@eth.net>
> 
> ---
> diff -upr orig-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c work-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c
> --- orig-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c	2005-03-15 21:00:10.000000000 +0530
> +++ work-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c	2005-03-24 15:29:23.539955472 +0530
> @@ -365,8 +365,9 @@ static int __init sn_pci_init(void)
>  	 */
>  
>  	while ((pci_dev > -		pci_find_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev)) != NULL) {
> +		pci_get_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev)) != NULL) {
>  		sn_pci_fixup_slot(pci_dev);
> +		pci_dev_put(dev);
>  	}

Argh, no, this is wrong.  And I think you did this in every one of the
patches you sent out, so please send a retraction for every one of them
too, I don't feel like doing it...

You should not do a pci_dev_put() in a loop like this.  Please go read
the documentation for the pci_get_device() function, it explains how to
use it...

So, sorry, but I'm not going to apply any of these patches.

Next time try sending just a few of them out, to see what the reaction
is, and if you are doing it properly.

greg k-h

Re: [PATCH 3/91] get rid of pci_find_{device,subsys} arch/ia64

[Amit Gud]

Greg KH wrote:
> On Tue, Mar 29, 2005 at 05:50:09PM +0530, Amit Gud wrote:
> 
>>Replace pci_find_{device,subsys} with pci_get_{device,subsys}.
>>
>>
>>
>>Signed-off-by: Amit Gud <gud@eth.net>
>>
>>---
>>diff -upr orig-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c work-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c
>>--- orig-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c	2005-03-15 21:00:10.000000000 +0530
>>+++ work-2.6.12-rc1/arch/ia64/sn/kernel/io_init.c	2005-03-24 15:29:23.539955472 +0530
>>@@ -365,8 +365,9 @@ static int __init sn_pci_init(void)
>> 	 */
>> 
>> 	while ((pci_dev >>-		pci_find_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev)) != NULL) {
>>+		pci_get_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev)) != NULL) {
>> 		sn_pci_fixup_slot(pci_dev);
>>+		pci_dev_put(dev);
>> 	}
> 
> 
> Argh, no, this is wrong.  And I think you did this in every one of the
> patches you sent out, so please send a retraction for every one of them
> too, I don't feel like doing it...
> 
> You should not do a pci_dev_put() in a loop like this.  Please go read
> the documentation for the pci_get_device() function, it explains how to
> use it...

Here admittedly, the variable is wrong (should be pci_dev and not dev). But
I don't see why pci_dev_put() should not be used in a loop. If I know it right, pci_get_device()
finds a particular device starting from the last argument and increments the reference count and pci_dev_put just decrements it
(which if becomes zero *pci_dev->dev->kobj is kfree()d, but the orginal struct pci_dev is still there (not-NULL), so
pci_get_device can start finding the device (starting from pci_dev), from where it had left off last time).
pci_find_device() is a non-safe way of finding a device, where it finds a particluar device but doesn't bother
incrementing the reference count.

So if we are to replace pci_find_device with pci_get_device, how should we go about in this particular case, which
apparently is the case with many other places where pci_find_device is used. The way I did seemed logical to me:
find the device - accept it - work with it - jerk it off.
Am I missing out something very obvious?


AG
--
May the source be with you.

Re: [PATCH 3/91] get rid of pci_find_{device,subsys} arch/ia64

[Greg KH]

On Wed, Mar 30, 2005 at 12:29:00PM +0530, Amit Gud wrote:
> So if we are to replace pci_find_device with pci_get_device, how should we 
> go about in this particular case, which
> apparently is the case with many other places where pci_find_device is 
> used. The way I did seemed logical to me:
> find the device - accept it - work with it - jerk it off.
> Am I missing out something very obvious?

Please read the last sentence of the documentation for the
pci_get_device() function.  I think you missed that :)

That sentence tells us that you do not need to call pci_dev_put() within
a pci_get_device() while loop.

So please, email all of the maintainers who you sent those patches to
and ask them to not apply them...

thanks,

greg k-h

Re: [PATCH 3/91] get rid of pci_find_{device,subsys} arch/ia64

[Amit Gud]

Amit Gud wrote:
 > replace pci_find_{device,subsys} with pci_get_{device,subsys}
 >

Please don't apply, its incorrect.

Sorry for the inconvenience.


Best,
AG
--
May the source be with you.