[PATCH] pci_find_device --> pci_get_device

[PATCH] pci_find_device --> pci_get_device

[Jiri Slaby]

The patch is for mixed files from all over the tree.

Kernel version: 2.6.13-rc3-git4

* This patch removes from kernel tree pci_find_device and changes
it with pci_get_device. Next, it adds pci_dev_put, to decrease reference
count of the variable.
* Next, there are some (about 10 or so) gcc warning problems (i. e.
variable may be unitialized) solutions, which were around code with old
pci_find_device.
* Some code was unpretty, or ugly, so the patch provides more readable
code, in some cases.
* Marks the function as deprecated in pci.h

The patch is here, (because of its size -- about 120 KiB):
http://www.fi.muni.cz/~xslaby/lnx/lnx-pci_find-2.6.13-r3g4.patch
and its bzipped version, if you want (about 25 KiB):
http://www.fi.muni.cz/~xslaby/lnx/lnx-pci_find-2.6.13-r3g4.patch.bz2

Signed-off-by: Jiri Slaby <xslaby@fi.muni.cz>

-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

Re: [PATCH] pci_find_device --> pci_get_device

[Rogier Wolff]

On Tue, Jul 19, 2005 at 02:25:23AM +0200, Jiri Slaby wrote:
> The patch is for mixed files from all over the tree.
> 
> Kernel version: 2.6.13-rc3-git4
> 
> * This patch removes from kernel tree pci_find_device and changes
> it with pci_get_device. Next, it adds pci_dev_put, to decrease reference
> count of the variable.
> * Next, there are some (about 10 or so) gcc warning problems (i. e.
> variable may be unitialized) solutions, which were around code with old
> pci_find_device.
> * Some code was unpretty, or ugly, so the patch provides more readable
> code, in some cases.
> * Marks the function as deprecated in pci.h

Hi Jiri, 

The patch grabs reference counts to pdev structures, but almost never
decreases the reference counts. 

If you are working in a team, and want others to be able to continue
where you left off, you should add a comment, even if it is repetitive
to state what needs to be done. 

As far as I can see, you grab a reference to the "pdev" on 
initialization, and never release it. Or you only release it in 
certain error conditions. Would this make the driver unable to 
be unloaded and reloaded? That would not be good. 

	Roger. 

-- 
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2600998 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. 
Does it sit on the couch all day? Is it unemployed? Please be specific! 
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ

Re: [PATCH] pci_find_device --> pci_get_device

[Jiri Slaby]

Rogier Wolff napsal(a):

>On Tue, Jul 19, 2005 at 02:25:23AM +0200, Jiri Slaby wrote:
>  
>
>>The patch is for mixed files from all over the tree.
>>
>>Kernel version: 2.6.13-rc3-git4
>>
>>* This patch removes from kernel tree pci_find_device and changes
>>it with pci_get_device. Next, it adds pci_dev_put, to decrease reference
>>count of the variable.
>>* Next, there are some (about 10 or so) gcc warning problems (i. e.
>>variable may be unitialized) solutions, which were around code with old
>>pci_find_device.
>>* Some code was unpretty, or ugly, so the patch provides more readable
>>code, in some cases.
>>* Marks the function as deprecated in pci.h
>>    
>>
>
>Hi Jiri, 
>  
>
Hello,

>The patch grabs reference counts to pdev structures, but almost never
>decreases the reference counts. 
>  
>
I think, that if there is a loop, the pci_get_device always decrease it, see
pci_get_subsys, line 249 (2.6.13-rc3-git4). This function is called by
pci_get_device.
So the question is, if I really need to decrease count, if it occurs in
while (pdev = pci_get_device(..., pdev))
{
initialize(pdev);
// pci_dev_put(pdev); // uncomment or no?
}

Now one problem came on my mind with initialize(pdev) -- if there is
some global var my = pdev in that and it returns back and calls
pci_get_device, it counts down the reference count of pdev, but we need
it anywhere else. Shouldn't be there pci_dev_get to increase ref count and
decrease it either after it is unused or after module unloads or never in
other cases?

>If you are working in a team, and want others to be able to continue
>where you left off, you should add a comment, even if it is repetitive
>to state what needs to be done. 
>  
>
I don't know, if you think it global, or if am I here with other fellows 
(no, I'm not).
I don't know what kind of comment you have on your mind. Could you, please,
specify it more.
I only changed names of called functions and added some pci_dev_put, 
what should
I comment?

>As far as I can see, you grab a reference to the "pdev" on 
>initialization, and never release it. Or you only release it in 
>certain error conditions. Would this make the driver unable to 
>be unloaded and reloaded? That would not be good. 
>  
>
The only thing, that pci_dev_put do, is that it releases memory of pdev 
structure, if
reference count is 0 (as far as I can see, there is no real doc, which 
could ensure me,
it is my piece of knowledge found out from source code, maybe wrong??). 
It does not
do anything with the device.
So, the question is the same as above... Is it right to NOT release, 
when it's in a loop?

-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

Re: [PATCH] pci_find_device --> pci_get_device

[Rogier Wolff]

On Tue, Jul 19, 2005 at 12:53:38PM +0200, Jiri Slaby wrote:
> Rogier Wolff napsal(a):

> I don't know, if you think it global, or if am I here with other
> fellows (no, I'm not).  I don't know what kind of comment you have
> on your mind. Could you, please, specify it more.  I only changed
> names of called functions and added some pci_dev_put, what should I
> comment?

I meant: that IF I'm right that there needs to be more pci_dev_put, 
that needs to be noted in the source. 

If you know that there needs to be a put, but you have decided you
don't know where to put it exactly, then that needs to be in a comment
so that someone looking at the code after you will be able to put it
in. Otherwise the new person will think: He might have had a smarter
plan and will do the pci_dev_put somewhere else. 

	Roger. 


-- 
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2600998 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. 
Does it sit on the couch all day? Is it unemployed? Please be specific! 
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ

Re: [PATCH] pci_find_device --> pci_get_device

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

Jiri Slaby wrote:
>The patch is for mixed files from all over the tree.
>
>Kernel version: 2.6.13-rc3-git4
>
>* This patch removes from kernel tree pci_find_device and changes
>it with pci_get_device. Next, it adds pci_dev_put, to decrease reference
>count of the variable.
>* Next, there are some (about 10 or so) gcc warning problems (i. e.
>variable may be unitialized) solutions, which were around code with old
>pci_find_device.

Is this the reason why you initialize members of static structs? If this is 
uninitialized it will end in the bss section and will be zeroed before the 
kernel uses is. If you do it will go into data section and add more bloat to 
the binary. At least this is the explanation I got once why not to do this.

Many of the callers of pci_find_device() look like they are not ported to the 
2.6 driver API and do the scanning for devices themself. I think it would be 
a good idea to try to convert them to the new driver model instead of 
replacing this. When you mark this deprecated and they still use the old 
function everyone using this will see that there is some work to do.

>* Some code was unpretty, or ugly, so the patch provides more readable
>code, in some cases.

If you try to beautify code then please use for_each_pci_dev() macro from 
include/linux/pci.h where possible.

When you want something of this getting included you have to split that into 
pieces. Use extra patches for changes in coding style and functionality.

>* Marks the function as deprecated in pci.h

This is a very good idea in my eyes.

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] pci_find_device --> pci_get_device

[Jiri Slaby]

Rolf Eike Beer napsal(a):

>Jiri Slaby wrote:
>  
>
>>Kernel version: 2.6.13-rc3-git4
>>
>>* This patch removes from kernel tree pci_find_device and changes
>>it with pci_get_device. Next, it adds pci_dev_put, to decrease reference
>>count of the variable.
>>* Next, there are some (about 10 or so) gcc warning problems (i. e.
>>variable may be unitialized) solutions, which were around code with old
>>pci_find_device.
>>    
>>
>
>Is this the reason why you initialize members of static structs? If this is 
>uninitialized it will end in the bss section and will be zeroed before the 
>kernel uses is. If you do it will go into data section and add more bloat to 
>the binary. At least this is the explanation I got once why not to do this.
>  
>
I can't find now changes of initialization static variables, but i have 
deleted section
dealing up with gcc warning from patch, it would go on a queue later.

>Many of the callers of pci_find_device() look like they are not ported to the 
>2.6 driver API and do the scanning for devices themself. I think it would be 
>a good idea to try to convert them to the new driver model instead of 
>replacing this. When you mark this deprecated and they still use the old 
>function everyone using this will see that there is some work to do.
>  
>
I don't know now the difference between API accurately, but I'll study 
it in a few days
and delete this sections from patch, alternatively rewrite the code.

>>* Some code was unpretty, or ugly, so the patch provides more readable
>>code, in some cases.
>>    
>>
>
>If you try to beautify code then please use for_each_pci_dev() macro from 
>include/linux/pci.h where possible.
>  
>
Done.

>When you want something of this getting included you have to split that into 
>pieces. Use extra patches for changes in coding style and functionality.
>  
>
Yeah, the patch now provides only changes of pci_find problems.
[Gcc will be discussed later, after doing more changes and completing patch
against gcc 4, where are more warnings...]


OK. The new patch is:
http://www.fi.muni.cz/~xslaby/lnx/lnx-pci_find-2.6.13-r3g4_1.patch
and bzipped
http://www.fi.muni.cz/~xslaby/lnx/lnx-pci_find-2.6.13-r3g4_1.patch.bz2
[Kernel version is the same (2.6.13-rc3-git4)]

And the patch now contains only:
* This patch removes from kernel tree pci_find_device and changes
it with pci_get_device. Next, it adds pci_dev_put, to decrease reference
count of the variable.
* Some code was unpretty, or ugly, so the patch provides more readable
code, in some cases.
* Marks the function (pci_find_device) as deprecated in pci.h

-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

Re: [PATCH] pci_find_device --> pci_get_device

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

Am Dienstag, 19. Juli 2005 17:44 schrieb Jiri Slaby:
>Rolf Eike Beer napsal(a):
>>Jiri Slaby wrote:
>>>Kernel version: 2.6.13-rc3-git4
>>>
>>>* This patch removes from kernel tree pci_find_device and changes
>>>it with pci_get_device. Next, it adds pci_dev_put, to decrease reference
>>>count of the variable.
>>>* Next, there are some (about 10 or so) gcc warning problems (i. e.
>>>variable may be unitialized) solutions, which were around code with old
>>>pci_find_device.
>>
>>Is this the reason why you initialize members of static structs? If this is
>>uninitialized it will end in the bss section and will be zeroed before the
>>kernel uses is. If you do it will go into data section and add more bloat
>> to the binary. At least this is the explanation I got once why not to do
>> this.
>
>I can't find now changes of initialization static variables, but i have
>deleted section
>dealing up with gcc warning from patch, it would go on a queue later.

Sorry, I misread the diff. That are static functions with the variables in 
them, not static structs.

Your patch to arch/sparc64/kernel/ebus.c is broken, the removed and added 
parts do not match in behaviour.

If you add braces after if's please add the '{' in the same line as the if 
itself. This will make the diff bigger, but then this matches 
Documentation/Coding-style.

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] pci_find_device --> pci_get_device

[Jiri Slaby]

Rolf Eike Beer napsal(a):

>Your patch to arch/sparc64/kernel/ebus.c is broken, the removed and added 
>parts do not match in behaviour.
>  
>
I can't still see the difference.

if (pdev && (pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS))
    *is_rio_p = 1;
else
    *is_rio_p = 0;
AND
*is_rio_p = !!(pdev && (pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS));

aren't the same or these:

    while (pdev = pci_get_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev))
        if (pdev->device == PCI_DEVICE_ID_SUN_EBUS ||
            pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS)
            break;
AND
  do {
        pdev = pci_find_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev);
        if (pdev &&
            (pdev->device == PCI_DEVICE_ID_SUN_EBUS ||
             pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS))
            break;
    } while (pdev != NULL);

>If you add braces after if's please add the '{' in the same line as the if 
>itself. This will make the diff bigger, but then this matches 
>Documentation/Coding-style.
>  
>
Ok, this has been improved.

* Added for_each_pci_dev wherever it is appropriate.

New patch:
http://www.fi.muni.cz/~xslaby/lnx/lnx-pci_find-2.6.13-r3g4_2.patch
http://www.fi.muni.cz/~xslaby/lnx/lnx-pci_find-2.6.13-r3g4_2.patch.bz2

-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

Re: [PATCH] pci_find_device --> pci_get_device

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

Am Mittwoch, 20. Juli 2005 12:40 schrieb Jiri Slaby:
>Rolf Eike Beer napsal(a):
>>Your patch to arch/sparc64/kernel/ebus.c is broken, the removed and added
>>parts do not match in behaviour.
>
>I can't still see the difference.

diff --git a/arch/sparc64/kernel/ebus.c b/arch/sparc64/kernel/ebus.c
--- a/arch/sparc64/kernel/ebus.c
+++ b/arch/sparc64/kernel/ebus.c
@@ -527,8 +527,15 @@ static struct pci_dev *find_next_ebus(st
 {
 	struct pci_dev *pdev = start;
 
-	do {
-		pdev = pci_find_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev);
+    while (pdev = pci_get_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev))
+		if (pdev->device == PCI_DEVICE_ID_SUN_EBUS ||
+			pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS)
+			break;
+
+	*is_rio_p = !!(pdev && (pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS));
+	
+/*	do { // BEFORE \/           AFTER ^
+ *		pdev = pci_find_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev);
 		if (pdev &&
 		    (pdev->device == PCI_DEVICE_ID_SUN_EBUS ||
 		     pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS))

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] pci_find_device --> pci_get_device

[Jiri Slaby]

Rolf Eike Beer napsal(a):

>Am Mittwoch, 20. Juli 2005 12:40 schrieb Jiri Slaby:
>  
>
>>Rolf Eike Beer napsal(a):
>>    
>>
>>>Your patch to arch/sparc64/kernel/ebus.c is broken, the removed and added
>>>parts do not match in behaviour.
>>>      
>>>
>>I can't still see the difference.
>>    
>>
>
>diff --git a/arch/sparc64/kernel/ebus.c b/arch/sparc64/kernel/ebus.c
>--- a/arch/sparc64/kernel/ebus.c
>+++ b/arch/sparc64/kernel/ebus.c
>@@ -527,8 +527,15 @@ static struct pci_dev *find_next_ebus(st
> {
> 	struct pci_dev *pdev = start;
> 
>-	do {
>-		pdev = pci_find_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev);
>+    while (pdev = pci_get_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev))
>+		if (pdev->device == PCI_DEVICE_ID_SUN_EBUS ||
>+			pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS)
>+			break;
>+
>+	*is_rio_p = !!(pdev && (pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS));
>+	
>+/*	do { // BEFORE \/           AFTER ^
>+ *		pdev = pci_find_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev);
> 		if (pdev &&
> 		    (pdev->device == PCI_DEVICE_ID_SUN_EBUS ||
> 		     pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS))
>  
>
The code was:
  do {
      pdev = pci_find_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev);
        if (pdev &&
            (pdev->device == PCI_DEVICE_ID_SUN_EBUS ||
             pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS))
            break;
    } while (pdev != NULL);

    if (pdev && (pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS))
        *is_rio_p = 1;
    else
        *is_rio_p = 0;

and I changed to:
    while (pdev = pci_get_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev))
        if (pdev->device == PCI_DEVICE_ID_SUN_EBUS ||
            pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS)
            break;

    *is_rio_p = !!(pdev && (pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS));

Is there any difference? I don't see any, but... The reading of diff 
file in this case is not the best, maybe...

-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

Re: [PATCH] pci_find_device --> pci_get_device

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

Jiri Slaby wrote:
>Rolf Eike Beer napsal(a):
>>Am Mittwoch, 20. Juli 2005 12:40 schrieb Jiri Slaby:
>>>Rolf Eike Beer napsal(a):
>>>>Your patch to arch/sparc64/kernel/ebus.c is broken, the removed and added
>>>>parts do not match in behaviour.
>>>
>>>I can't still see the difference.
>>
>>diff --git a/arch/sparc64/kernel/ebus.c b/arch/sparc64/kernel/ebus.c
>>--- a/arch/sparc64/kernel/ebus.c
>>+++ b/arch/sparc64/kernel/ebus.c
>>@@ -527,8 +527,15 @@ static struct pci_dev *find_next_ebus(st
>> {
>> 	struct pci_dev *pdev = start;
>>
>>-	do {
>>-		pdev = pci_find_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev);
>>+    while (pdev = pci_get_device(PCI_VENDOR_ID_SUN, PCI_ANY_ID, pdev))
>>+		if (pdev->device == PCI_DEVICE_ID_SUN_EBUS ||
>>+			pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS)
>>+			break;
>>+	*is_rio_p = !!(pdev && (pdev->device == PCI_DEVICE_ID_SUN_RIO_EBUS));
>>+
>>+/*	do { // BEFORE \/           AFTER ^

This looks like some sed command went wrong. And I missed that you were 
starting a comment here.

>Is there any difference? I don't see any, but... The reading of diff
>file in this case is not the best, maybe...

Yes, that was the problem. I would prefer if you could just remove the code 
instead of commenting it out. This would have made this clearer.

If my editor doesn't fool me you are using spaces for indentation of the 
while. There has to be a tab.

Question to the sparc folks: is it really needed to preserve the order of the 
ebusses? Or would it be possible to do

pdev = pci_find_device(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_EBUS)

first and if this returns NULL start again with 

pdev = pci_find_device(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_RIO_EBUS)

?

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] pci_find_device --> pci_get_device

[Jiri Slaby]

Rolf Eike Beer napsal(a):

>Jiri Slaby wrote:
>  
>
>>Is there any difference? I don't see any, but... The reading of diff
>>file in this case is not the best, maybe...
>>    
>>
>
>Yes, that was the problem. I would prefer if you could just remove the code 
>instead of commenting it out. This would have made this clearer.
>
>If my editor doesn't fool me you are using spaces for indentation of the 
>while. There has to be a tab.
>  
>
I'm really sorry. I made tabs from spaces and removes ebus old code:
http://www.fi.muni.cz/~xslaby/lnx/lnx-pci_find-2.6.13-r3g4_3.patch
http://www.fi.muni.cz/~xslaby/lnx/lnx-pci_find-2.6.13-r3g4_3.patch.bz2

-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10