pci_find_device --> pci_get_device
PATCH 1/1 pci_find_device --> pci_get_device
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
Signature · 3 lines
Jiri Slaby www.fi.muni.cz/~xslaby ~\-/~ [email protected] ~\-/~ 241B347EC88228DE51EE A49C4A73A25004CB2A10
Quoted 4 lines · Jiri Slaby
The patch is for mixed files from all over the tree.
Kernel version: 2.6.13-rc3-git4
Quoted 9 lines
* This patch removes from kernel tree
pci_find_deviceand changes it withpci_get_device. Next, it addspci_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 oldpci_find_device.
* Some code was unpretty, or ugly, so the patch provides more readable code, in some cases.
* Marks the function as deprecated inpci.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.
Signature · 5 lines
** [email protected] ** 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
Rogier Wolff napsal(a):
Quoted 19 lines · Jiri Slaby
Quoted 6 lines
The patch is for mixed files from all over the tree.
Kernel version: 2.6.13-rc3-git4
Quoted 11 lines
* This patch removes from kernel tree
pci_find_deviceand changes it withpci_get_device. Next, it addspci_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 oldpci_find_device.
* Some code was unpretty, or ugly, so the patch provides more readable code, in some cases.
* Marks the function as deprecated inpci.h
Quoted 3 lines
Hi Jiri,
Hello,
Quoted 4 lines
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?
Quoted 5 lines
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?
Quoted 3 lines
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?
Signature · 3 lines
Jiri Slaby www.fi.muni.cz/~xslaby ~\-/~ [email protected] ~\-/~ 241B347EC88228DE51EE A49C4A73A25004CB2A10
Quoted 7 lines · Jiri Slaby
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.
Signature · 5 lines
** [email protected] ** 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
Quoted 10 lines · 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_deviceand changes it withpci_get_device. Next, it addspci_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 oldpci_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.
Quoted 2 lines
* 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.
Quoted 1 line
* Marks the function as deprecated in
pci.h
This is a very good idea in my eyes.
Eike
Rolf Eike Beer napsal(a):
Quoted 14 lines
Quoted 10 lines · Jiri Slaby
Kernel version: 2.6.13-rc3-git4
* This patch removes from kernel tree
pci_find_deviceand changes it withpci_get_device. Next, it addspci_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 oldpci_find_device.
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
* 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 frominclude/linux/pci.hwhere possible.
Done.
Quoted 3 lines
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
Signature · 3 lines
Jiri Slaby www.fi.muni.cz/~xslaby ~\-/~ [email protected] ~\-/~ 241B347EC88228DE51EE A49C4A73A25004CB2A10
Am Dienstag, 19. Juli 2005 17:44 schrieb Jiri Slaby:
Quoted 17 lines
Rolf Eike Beer napsal(a):
Quoted 10 lines
Quoted 8 lines · Jiri Slaby
Kernel version: 2.6.13-rc3-git4
* This patch removes from kernel tree
pci_find_deviceand changes it withpci_get_device. Next, it addspci_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 oldpci_find_device.Quoted 5 lines
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.
Quoted 3 lines
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
Rolf Eike Beer napsal(a):
Quoted 3 lines
Your patch to
arch/sparc64/kernel/ebus.cis 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);
Quoted 3 lines
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
Signature · 3 lines
Jiri Slaby www.fi.muni.cz/~xslaby ~\-/~ [email protected] ~\-/~ 241B347EC88228DE51EE A49C4A73A25004CB2A10
Am Mittwoch, 20. Juli 2005 12:40 schrieb Jiri Slaby:
Quoted 5 lines
Rolf Eike Beer napsal(a):
Quoted 2 lines
Your patch to
arch/sparc64/kernel/ebus.cis broken, the removed and added parts do not match in behaviour.I can't still see the difference.
arch/sparc64/kernel/ebus.c +9 -2
@@ -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
Rolf Eike Beer napsal(a):
Quoted 27 lines
Am Mittwoch, 20. Juli 2005 12:40 schrieb Jiri Slaby:
Quoted 10 lines
Rolf Eike Beer napsal(a):
Quoted 4 lines
Your patch to
arch/sparc64/kernel/ebus.cis broken, the removed and added parts do not match in behaviour.I can't still see the difference.
arch/sparc64/kernel/ebus.c +2
-2@@ -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 ||
arch/sparc64/kernel/ebus.c static struct pci_dev *find_next_ebus(st · line 541
Quoted 10 lines
arch/sparc64/kernel/ebus.c +7
-0+ 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...
Signature · 3 lines
Jiri Slaby www.fi.muni.cz/~xslaby ~\-/~ [email protected] ~\-/~ 241B347EC88228DE51EE A49C4A73A25004CB2A10
Quoted 15 lines · Jiri Slaby
Rolf Eike Beer napsal(a):
Quoted 14 lines
Am Mittwoch, 20. Juli 2005 12:40 schrieb Jiri Slaby:
Quoted 5 lines
Rolf Eike Beer napsal(a):
Quoted 2 lines
Your patch to
arch/sparc64/kernel/ebus.cis broken, the removed and added parts do not match in behaviour.I can't still see the difference.
arch/sparc64/kernel/ebus.c +0
-0@@ -527,8 +527,15 @@ static struct pci_dev *find_next_ebus(st {struct pci_dev *pdev = start;
Quoted 9 lines
arch/sparc64/kernel/ebus.c static struct pci_dev *find_next_ebus(st · line 537
Quoted 9 lines
arch/sparc64/kernel/ebus.c +7
-2- 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.
Quoted 2 lines
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
Rolf Eike Beer napsal(a):
Quoted 13 lines
Quoted 4 lines · Jiri Slaby
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
Signature · 3 lines
Jiri Slaby www.fi.muni.cz/~xslaby ~\-/~ [email protected] ~\-/~ 241B347EC88228DE51EE A49C4A73A25004CB2A10