Fix parport_serial_pci_resume() ignoring return value from
pci_enable_device().
(I guess that the parport_serial_pci_remove() is the right way(tm) to
remove the device from the system in non-destructive way even in case
pci_enable_device() failed. Tim?)
Signed-off-by: Jiri Kosina <[email protected]>
---
drivers/parport/parport_serial.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/drivers/parport/parport_serial.c b/drivers/parport/parport_serial.c
index 78c0a26..ea76b5e 100644
--- a/drivers/parport/parport_serial.c
+++ b/drivers/parport/parport_serial.c
@@ -392,6 +392,7 @@ static int parport_serial_pci_suspend(st
static int parport_serial_pci_resume(struct pci_dev *dev)
{
struct parport_serial_private *priv = pci_get_drvdata(dev);
+ int err;
pci_set_power_state(dev, PCI_D0);
pci_restore_state(dev);
@@ -399,7 +400,14 @@ static int parport_serial_pci_resume(str
/*
* The device may have been disabled. Re-enable it.
*/
- pci_enable_device(dev);
+ err = pci_enable_device(dev);
+ if (err) {
+ printk(KERN_ERR "parport: Cannot enable PCI device %s during resume\n",
+ pci_name(dev));
+ parport_serial_pci_remove(dev);
+ return err;
+ }
+
if (priv->serial)
pciserial_resume_ports(priv->serial);
--
Jiri Kosina
On Fri, Oct 13, 2006 at 01:44:24AM +0200, Jiri Kosina wrote:
> (I guess that the parport_serial_pci_remove() is the right way(tm) to
> remove the device from the system in non-destructive way even in case
> pci_enable_device() failed. Tim?)
I suspect all these kind of patches are introducing additional problems.
This one certainly is. Who's auditing all these patches? I mean _properly_
auditing them rather than just saying "that's a good idea"?
In this case, you're calling parport_serial_pci_remove() in the failure
path. That's fine, but this opens the possibility of it being called
twice - once on resume failure and once when the device/driver is
removed. If this happens, we dereference a NULL pointer. *BAD*.
So, the original without this resume fix is probably far better than
with the fix.
So, patch violently rejected.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
On Fri, 13 Oct 2006, Russell King wrote:
> In this case, you're calling parport_serial_pci_remove() in the failure
> path. That's fine, but this opens the possibility of it being called
> twice - once on resume failure and once when the device/driver is
> removed. If this happens, we dereference a NULL pointer. *BAD*.
You are right, I missed this.
I am not currently sure what the proper fix is, though. We might be also
ending doing very bad things, when we ignore the error returned by
pci_enable_device() and proceed operating on non-existing device.
Thanks for spotting this,
--
Jiri Kosina
On Fri, 13 Oct 2006, Russell King wrote:
> I suspect all these kind of patches are introducing additional problems.
> This one certainly is. Who's auditing all these patches? I mean _properly_
> auditing them rather than just saying "that's a good idea"?
For the fm801 gameport case it is fine - it's just one more check in the
_probe() function, performing the same error path as other cases do.
ALSA case has already been solved by calling snd_card_disconnect() on
error path.
But I agree that sk98lin and parport_serial_pci patches are likely broken.
Thanks,
--
Jiri Kosina