parisc: superio: do not use PCI resources before pci_enable_device()
Subsystems (2)
PATCH 7/11 parisc: superio: do not use PCI resources before pci_enable_device()
IRQ and resource[] may not have correct values until after PCI hotplug setup occurs at pci_enable_device() time.
The semantic match that finds this problem is as follows:
// <smpl>
@@
identifier x;
identifier request ~= "pci_request.*|pci_resource.*"; @@
(
* x->irq
|
* x->resource
|
* request(x, ...)
)
...*pci_enable_device(x)
// </smpl>
--- drivers/parisc/superio.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/parisc/superio.c +3 -2
@@ -169,8 +169,6 @@ superio_init(struct pci_dev *pcidev)/* ...then properly fixup the USB to point at suckyio PIC */ sio->usb_pdev->irq =superio_fixup_irq (sio->usb_pdev); - printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", - pci_name(pdev), pdev->irq);pci_read_config_dword (pdev,SIO_SP1BAR , &sio->sp1_base); sio->sp1_base &= ~1 ;
@@ -204,6 +202,9 @@ superio_init(struct pci_dev *pcidev) ret =pci_enable_device (pdev);BUG_ON (ret <0 );/* not too much we can do about this... */ + printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", + pci_name(pdev), pdev->irq); +/* * Next project is programming the onboard interrupt controllers. * PDC hasn't done this for us, since it's using polled I/O.
Signature · 1 line
1.7.0.4
James Bottomley
maintainerQuoted 17 lines · Kulikov Vasiliy
drivers/parisc/superio.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)drivers/parisc/superio.c +0
-2@@ -169,8 +169,6 @@ superio_init(struct pci_dev *pcidev)/* ...then properly fixup the USB to point at suckyio PIC */ sio->usb_pdev->irq =superio_fixup_irq (sio->usb_pdev); - printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", - pci_name(pdev), pdev->irq);pci_read_config_dword (pdev,SIO_SP1BAR , &sio->sp1_base); sio->sp1_base &= ~1 ;
drivers/parisc/superio.c superio_init(struct pci_dev *pcidev) · line 210
Quoted 9 lines
drivers/parisc/superio.c +3
-0@@ -204,6 +202,9 @@ superio_init(struct pci_dev *pcidev) ret =pci_enable_device (pdev);BUG_ON (ret <0 );/* not too much we can do about this... */ + printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", + pci_name(pdev), pdev->irq); +/* * Next project is programming the onboard interrupt controllers. * PDC hasn't done this for us, since it's using polled I/O.
This really makes no sense; we want the information printed before we start all the discovery prints, not after. I assume lots of initialisers want this too, so it looks particularly silly not to have pci_name() returning the correct value until enable time.
If this is just the name initially assigned may change, this definitely isn't true for parisc, and even for other things, it would be better to print something than nothing.
James
Quoted 21 lines · James Bottomley
Quoted 20 lines · Kulikov Vasiliy
drivers/parisc/superio.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)drivers/parisc/superio.c +0
-2@@ -169,8 +169,6 @@ superio_init(struct pci_dev *pcidev)/* ...then properly fixup the USB to point at suckyio PIC */ sio->usb_pdev->irq =superio_fixup_irq (sio->usb_pdev); - printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", - pci_name(pdev), pdev->irq);pci_read_config_dword (pdev,SIO_SP1BAR , &sio->sp1_base); sio->sp1_base &= ~1 ;@@ -204,6 +202,9 @@ superio_init(struct pci_dev *pcidev) ret =pci_enable_device (pdev);BUG_ON (ret <0 );/* not too much we can do about this... */
Quoted 9 lines
drivers/parisc/superio.c superio_init(struct pci_dev *pcidev) · line 210
Quoted 6 lines
drivers/parisc/superio.c +3
-0+ printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", + pci_name(pdev), pdev->irq); +/* * Next project is programming the onboard interrupt controllers. * PDC hasn't done this for us, since it's using polled I/O. This really makes no sense; we want the information printed before we start all the discovery prints, not after.
Right, I missed it.
Quoted 7 lines
I assume lots of
initialisers want this too, so it looks particularly silly not to havepci_name()returning the correct value until enable time.If this is just the name initially assigned may change, this definitely isn't true for parisc, and even for other things, it would be better to print something than nothing.
Not name, but pdev-irq. I was not searching for pci_name() as you can see in coccinelle script :)
As it is only printing wrong value, the worst case is fooling user that reads logs.
Is it OK to move pci_enable_device() just before this printk()?
Quoted 4 lines
James
James Bottomley
maintainerQuoted 42 lines · Vasiliy Kulikov
Quoted 21 lines · James Bottomley
Quoted 20 lines · Kulikov Vasiliy
drivers/parisc/superio.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)drivers/parisc/superio.c +0
-2@@ -169,8 +169,6 @@ superio_init(struct pci_dev *pcidev)/* ...then properly fixup the USB to point at suckyio PIC */ sio->usb_pdev->irq =superio_fixup_irq (sio->usb_pdev); - printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", - pci_name(pdev), pdev->irq);pci_read_config_dword (pdev,SIO_SP1BAR , &sio->sp1_base); sio->sp1_base &= ~1 ;@@ -204,6 +202,9 @@ superio_init(struct pci_dev *pcidev) ret =pci_enable_device (pdev);BUG_ON (ret <0 );/* not too much we can do about this... */ Quoted 9 lines
drivers/parisc/superio.c superio_init(struct pci_dev *pcidev) · line 210
Quoted 6 lines
drivers/parisc/superio.c +3
-0+ printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", + pci_name(pdev), pdev->irq); +/* * Next project is programming the onboard interrupt controllers. * PDC hasn't done this for us, since it's using polled I/O. This really makes no sense; we want the information printed before we start all the discovery prints, not after.
Right, I missed it.
Quoted 7 lines
I assume lots of
initialisers want this too, so it looks particularly silly not to havepci_name()returning the correct value until enable time.If this is just the name initially assigned may change, this definitely isn't true for parisc, and even for other things, it would be better to print something than nothing.
Quoted 8 lines
Not name, but pdev-irq. I was not searching for
pci_name()as you can see in coccinelle script :)As it is only printing wrong value, the worst case is fooling user that reads logs.
Is it OK to move
pci_enable_device()just before thisprintk()?
Not really ... we need the device set up before it gets enabled.
If it's only the irq, then just leave it ... the name is more important and an uninitialised IRQ returns -1 which is a reasonable signal of not being set up yet ... but I think you'll find it will be pre setup on all platforms.
However,
Quoted 33 lines · On Tue, 03 Aug 2010 11:11:20 -0500 James Bottomley…
Quoted 17 lines · Kulikov Vasiliy
drivers/parisc/superio.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)drivers/parisc/superio.c +0
-2@@ -169,8 +169,6 @@ superio_init(struct pci_dev *pcidev)/* ...then properly fixup the USB to point at suckyio PIC */ sio->usb_pdev->irq =superio_fixup_irq (sio->usb_pdev); - printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", - pci_name(pdev), pdev->irq);pci_read_config_dword (pdev,SIO_SP1BAR , &sio->sp1_base); sio->sp1_base &= ~1 ;drivers/parisc/superio.c superio_init(struct pci_dev *pcidev) · line 210
Quoted 9 lines
drivers/parisc/superio.c +3
-0@@ -204,6 +202,9 @@ superio_init(struct pci_dev *pcidev) ret =pci_enable_device (pdev);BUG_ON (ret <0 );/* not too much we can do about this... */ + printk(KERN_INFO PFX "Found NS87560 Legacy I/O device at %s (IRQ %i)\n", + pci_name(pdev), pdev->irq); +/* * Next project is programming the onboard interrupt controllers. * PDC hasn't done this for us, since it's using polled I/O. This really makes no sense; we want the information printed before we start all the discovery prints, not after. I assume lots of initialisers want this too, so it looks particularly silly not to have
pci_name()returning the correct value until enable time.
Quoted 3 lines
If this is just the name initially assigned may change, this definitely isn't true for parisc, and even for other things, it would be better to print something than nothing.
Right. Some of these "enable before use" patches are ok, for example in some cases the IRQ may not be fully set up or resources may not be enabled, so re-ordering things makes sense.
But just printing these values at init time should be harmless. So perhaps a smarter static check script is in order.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center