Regression in -git / [PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled

Regression in -git / [PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled

[Daniel Ritz]

yet another small regression in current git...so only 2 left for me:)
rgds
-daniel

-----

[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled

Commit 02dd7ae2892e5ceff111d032769c78d3377df970:
	[PATCH] i2c-i801: Merge setup function
has a missing return 0 in the _probe() function. this means the error
path is always executed and pci_disable_device() is called even when
the device just got successfully enabled.

having the SMBus device disabled makes some systems (eg. Fujitsu-Siemens
Lifebook E8010) hang hard during power-off.

Intead of reverting the whole commit this patch fixes it up:
- don't ever call pci_disable_device(), also not in the _remove() function
  to avoid hangs
- fix missing pci_release_region() in error path

Signed-off-by: Daniel Ritz <daniel.ritz@gmx.ch>

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 3e0d04d..8b46ef7 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -488,7 +488,7 @@ static int __devinit i801_probe(struct p
 		dev_err(&dev->dev, "SMBus base address uninitialized, "
 			"upgrade BIOS\n");
 		err = -ENODEV;
-		goto exit_disable;
+		goto exit;
 	}
 
 	err = pci_request_region(dev, SMBBAR, i801_driver.name);
@@ -496,7 +496,7 @@ static int __devinit i801_probe(struct p
 		dev_err(&dev->dev, "Failed to request SMBus region "
 			"0x%lx-0x%lx\n", i801_smba,
 			pci_resource_end(dev, SMBBAR));
-		goto exit_disable;
+		goto exit;
 	}
 
 	pci_read_config_byte(I801_dev, SMBHSTCFG, &temp);
@@ -520,11 +520,12 @@ static int __devinit i801_probe(struct p
 	err = i2c_add_adapter(&i801_adapter);
 	if (err) {
 		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
-		goto exit_disable;
+		goto exit_release;
 	}
+	return 0;
 
-exit_disable:
-	pci_disable_device(dev);
+exit_release:
+	pci_release_region(dev, SMBBAR);
 exit:
 	return err;
 }
@@ -533,7 +534,10 @@ static void __devexit i801_remove(struct
 {
 	i2c_del_adapter(&i801_adapter);
 	pci_release_region(dev, SMBBAR);
-	pci_disable_device(dev);
+	/*
+	 * do not call pci_disable_device(dev) since it can cause hard hangs on
+	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
+	 */
 }
 
 static struct pci_driver i801_driver = {

Re: Regression in -git / [PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled

[Jean Delvare]

Hi Daniel,

I see that your patch was already merged, but I would like to reply
anyway.

> [PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled
> 
> Commit 02dd7ae2892e5ceff111d032769c78d3377df970:
> 	[PATCH] i2c-i801: Merge setup function
> has a missing return 0 in the _probe() function. this means the error
> path is always executed and pci_disable_device() is called even when
> the device just got successfully enabled.

Oops, good catch, thanks. I'm quite ashamed for letting this go
through :(

> having the SMBus device disabled makes some systems (eg. Fujitsu-Siemens
> Lifebook E8010) hang hard during power-off.
> 
> Intead of reverting the whole commit this patch fixes it up:
> - don't ever call pci_disable_device(), also not in the _remove() function
>   to avoid hangs

This is weird, and would certainly deserve additional investigation.
Disabling the PCI device when we no more need it is the right thing to
do and almost all pci drivers do that by now - except I2C bus drivers,
this is the first one I was attempting to convert.

Do you have any idea why disabling the SMBus causes the problem you
observe? Could be that your BIOS attempts to use the SMBus at power
down time, but I wonder what for. Do you have anything special on this
SMBus? Proprietary EEPROM? Real-time clock? I have two laptops using
this driver (one Sony, one Dell) and none exhibited the problem you
described.

> - fix missing pci_release_region() in error path

Another bug I let go through :( Thanks again.

-- 
Jean Delvare

Re: Regression in -git / [PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled

[Daniel Ritz]

hi

On Thursday 29 June 2006 14.04, Jean Delvare wrote:
> Hi Daniel,
> 
> I see that your patch was already merged, but I would like to reply
> anyway.
> 
> > [PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled
> > 
> > Commit 02dd7ae2892e5ceff111d032769c78d3377df970:
> > 	[PATCH] i2c-i801: Merge setup function
> > has a missing return 0 in the _probe() function. this means the error
> > path is always executed and pci_disable_device() is called even when
> > the device just got successfully enabled.
> 
> Oops, good catch, thanks. I'm quite ashamed for letting this go
> through :(
> 
> > having the SMBus device disabled makes some systems (eg. Fujitsu-Siemens
> > Lifebook E8010) hang hard during power-off.
> > 
> > Intead of reverting the whole commit this patch fixes it up:
> > - don't ever call pci_disable_device(), also not in the _remove() function
> >   to avoid hangs
> 
> This is weird, and would certainly deserve additional investigation.
> Disabling the PCI device when we no more need it is the right thing to
> do and almost all pci drivers do that by now - except I2C bus drivers,

basically i agree, but...

> this is the first one I was attempting to convert.
> 
> Do you have any idea why disabling the SMBus causes the problem you
> observe? Could be that your BIOS attempts to use the SMBus at power

no idea. the last message that is display is "Shutdown: hda" then the
cursor blinks for 2 more seconds, then complete freeze. also enabling
all the debugging options in driver model, pm, i2c does not give me anything
more (it should...the messages during boot are there)...

> down time, but I wonder what for. Do you have anything special on this
> SMBus? Proprietary EEPROM? Real-time clock? I have two laptops using
> this driver (one Sony, one Dell) and none exhibited the problem you
> described.

sensors-detect says that there is the smartbattery thingy connected...
maybe the BIOS is querying the battery? dunno...

anyway i'll try again to find out what's causing the hang...

rgds
-daniel

Re: Regression in -git / [PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled

[Linus Torvalds]

On Thu, 29 Jun 2006, Daniel Ritz wrote:
> > 
> > Do you have any idea why disabling the SMBus causes the problem you
> > observe? Could be that your BIOS attempts to use the SMBus at power
> 
> no idea. the last message that is display is "Shutdown: hda" then the
> cursor blinks for 2 more seconds, then complete freeze. also enabling
> all the debugging options in driver model, pm, i2c does not give me anything
> more (it should...the messages during boot are there)...

The SMBus devices may well be used by ACPI (or even SMM, although that is 
probably rare these days).

I don't actually think we should necessarily shut off motherboard devices 
like that, for this reason.

		Linus