[PATCHv2 0/5] i2c: i801: Few cleanups

[PATCHv2 0/5] i2c: i801: Few cleanups

[Jarkko Nikula]

Hi Jean

Patches 2-3/5 are otherwise the same I sent last week but now on top
of 1/5.

Jarkko Nikula (5):
  i2c: i801: Don't break user-visible strings
  i2c: i801: Remove i801_driver forward declaration
  i2c: i801: Use managed devm_* memory and irq allocation
  i2c: i801: Remove pci_enable_device() call from i801_resume()
  i2c: i801: Use managed pcim_* PCI device initialization and
    reservation

 drivers/i2c/busses/i2c-i801.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

-- 
2.1.4

[PATCHv2 1/5] i2c: i801: Don't break user-visible strings

[Jarkko Nikula]

It makes more difficult to grep these error prints from sources if they are
split to multiple source lines.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 8fafb254e42a..7d1f4a478c54 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1192,8 +1192,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* Determine the address of the SMBus area */
 	priv->smba = pci_resource_start(dev, SMBBAR);
 	if (!priv->smba) {
-		dev_err(&dev->dev, "SMBus base address uninitialized, "
-			"upgrade BIOS\n");
+		dev_err(&dev->dev,
+			"SMBus base address uninitialized, upgrade BIOS\n");
 		err = -ENODEV;
 		goto exit;
 	}
@@ -1206,8 +1206,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	err = pci_request_region(dev, SMBBAR, i801_driver.name);
 	if (err) {
-		dev_err(&dev->dev, "Failed to request SMBus region "
-			"0x%lx-0x%Lx\n", priv->smba,
+		dev_err(&dev->dev,
+			"Failed to request SMBus region 0x%lx-0x%Lx\n",
+			priv->smba,
 			(unsigned long long)pci_resource_end(dev, SMBBAR));
 		goto exit;
 	}
-- 
2.1.4

[PATCHv2 2/5] i2c: i801: Remove i801_driver forward declaration

[Jarkko Nikula]

struct pci_driver i801_driver forward declaration is needed only for
accessing the name field. Remove it and use dev_driver_string() instead.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 7d1f4a478c54..5f827dfc671a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -223,8 +223,6 @@ struct i801_priv {
 #endif
 };
 
-static struct pci_driver i801_driver;
-
 #define FEATURE_SMBUS_PEC	(1 << 0)
 #define FEATURE_BLOCK_BUFFER	(1 << 1)
 #define FEATURE_BLOCK_PROC	(1 << 2)
@@ -1204,7 +1202,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		goto exit;
 	}
 
-	err = pci_request_region(dev, SMBBAR, i801_driver.name);
+	err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev));
 	if (err) {
 		dev_err(&dev->dev,
 			"Failed to request SMBus region 0x%lx-0x%Lx\n",
@@ -1256,7 +1254,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		init_waitqueue_head(&priv->waitq);
 
 		err = request_irq(dev->irq, i801_isr, IRQF_SHARED,
-				  i801_driver.name, priv);
+				  dev_driver_string(&dev->dev), priv);
 		if (err) {
 			dev_err(&dev->dev, "Failed to allocate irq %d: %d\n",
 				dev->irq, err);
-- 
2.1.4

[PATCHv2 3/5] i2c: i801: Use managed devm_* memory and irq allocation

[Jarkko Nikula]

This simplifies the error and remove paths.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5f827dfc671a..b1d725d758bb 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1138,7 +1138,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	int err, i;
 	struct i801_priv *priv;
 
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
@@ -1253,8 +1253,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (priv->features & FEATURE_IRQ) {
 		init_waitqueue_head(&priv->waitq);
 
-		err = request_irq(dev->irq, i801_isr, IRQF_SHARED,
-				  dev_driver_string(&dev->dev), priv);
+		err = devm_request_irq(&dev->dev, dev->irq, i801_isr,
+				       IRQF_SHARED,
+				       dev_driver_string(&dev->dev), priv);
 		if (err) {
 			dev_err(&dev->dev, "Failed to allocate irq %d: %d\n",
 				dev->irq, err);
@@ -1275,7 +1276,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
 		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
-		goto exit_free_irq;
+		goto exit_release;
 	}
 
 	i801_probe_optional_slaves(priv);
@@ -1286,12 +1287,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	return 0;
 
-exit_free_irq:
-	if (priv->features & FEATURE_IRQ)
-		free_irq(dev->irq, priv);
+exit_release:
 	pci_release_region(dev, SMBBAR);
 exit:
-	kfree(priv);
 	return err;
 }
 
@@ -1303,11 +1301,8 @@ static void i801_remove(struct pci_dev *dev)
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 
-	if (priv->features & FEATURE_IRQ)
-		free_irq(dev->irq, priv);
 	pci_release_region(dev, SMBBAR);
 
-	kfree(priv);
 	/*
 	 * do not call pci_disable_device(dev) since it can cause hard hangs on
 	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
-- 
2.1.4

[PATCHv2 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()

[Jarkko Nikula]

Since pci_disable_device() is not called from i801_suspend() and power
state is set already it means that subsequent pci_enable_device() calls do
practically nothing but monotonically increase struct pci_dev enable_cnt.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index b1d725d758bb..5fb35464f693 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1324,7 +1324,7 @@ static int i801_resume(struct pci_dev *dev)
 {
 	pci_set_power_state(dev, PCI_D0);
 	pci_restore_state(dev);
-	return pci_enable_device(dev);
+	return 0;
 }
 #else
 #define i801_suspend NULL
-- 
2.1.4

[PATCHv2 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation

[Jarkko Nikula]

Simplifies the code a bit and makes easier to disable PCI device on driver
detach by removing the pcim_pin_device() call in the future if needed.

Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it
made some systems to hang during power-off. See commit d6fcb3b9cf77
("[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled")
and
http://marc.info/?l=linux-kernel&m=115160053309535&w=2

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5fb35464f693..9f7b69743233 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1180,12 +1180,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 	priv->features &= ~disable_features;
 
-	err = pci_enable_device(dev);
+	err = pcim_enable_device(dev);
 	if (err) {
 		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
 			err);
 		goto exit;
 	}
+	pcim_pin_device(dev);
 
 	/* Determine the address of the SMBus area */
 	priv->smba = pci_resource_start(dev, SMBBAR);
@@ -1202,7 +1203,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		goto exit;
 	}
 
-	err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev));
+	err = pcim_iomap_regions(dev, 1 << SMBBAR, dev_driver_string(&dev->dev));
 	if (err) {
 		dev_err(&dev->dev,
 			"Failed to request SMBus region 0x%lx-0x%Lx\n",
@@ -1276,7 +1277,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
 		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
-		goto exit_release;
+		goto exit;
 	}
 
 	i801_probe_optional_slaves(priv);
@@ -1287,8 +1288,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	return 0;
 
-exit_release:
-	pci_release_region(dev, SMBBAR);
 exit:
 	return err;
 }
@@ -1301,8 +1300,6 @@ static void i801_remove(struct pci_dev *dev)
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 
-	pci_release_region(dev, SMBBAR);
-
 	/*
 	 * do not call pci_disable_device(dev) since it can cause hard hangs on
 	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
-- 
2.1.4

Re: [PATCHv2 1/5] i2c: i801: Don't break user-visible strings

[Jean Delvare]

On Wed, 11 Feb 2015 14:32:04 +0200, Jarkko Nikula wrote:
> It makes more difficult to grep these error prints from sources if they are
> split to multiple source lines.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 8fafb254e42a..7d1f4a478c54 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1192,8 +1192,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	/* Determine the address of the SMBus area */
>  	priv->smba = pci_resource_start(dev, SMBBAR);
>  	if (!priv->smba) {
> -		dev_err(&dev->dev, "SMBus base address uninitialized, "
> -			"upgrade BIOS\n");
> +		dev_err(&dev->dev,
> +			"SMBus base address uninitialized, upgrade BIOS\n");
>  		err = -ENODEV;
>  		goto exit;
>  	}
> @@ -1206,8 +1206,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  
>  	err = pci_request_region(dev, SMBBAR, i801_driver.name);
>  	if (err) {
> -		dev_err(&dev->dev, "Failed to request SMBus region "
> -			"0x%lx-0x%Lx\n", priv->smba,
> +		dev_err(&dev->dev,
> +			"Failed to request SMBus region 0x%lx-0x%Lx\n",
> +			priv->smba,
>  			(unsigned long long)pci_resource_end(dev, SMBBAR));
>  		goto exit;
>  	}

Looks good.

Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCHv2 3/5] i2c: i801: Use managed devm_* memory and irq allocation

[Jean Delvare]

On Wed, 11 Feb 2015 14:32:06 +0200, Jarkko Nikula wrote:
> This simplifies the error and remove paths.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5f827dfc671a..b1d725d758bb 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1138,7 +1138,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	int err, i;
>  	struct i801_priv *priv;
>  
> -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> @@ -1253,8 +1253,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (priv->features & FEATURE_IRQ) {
>  		init_waitqueue_head(&priv->waitq);
>  
> -		err = request_irq(dev->irq, i801_isr, IRQF_SHARED,
> -				  dev_driver_string(&dev->dev), priv);
> +		err = devm_request_irq(&dev->dev, dev->irq, i801_isr,
> +				       IRQF_SHARED,
> +				       dev_driver_string(&dev->dev), priv);
>  		if (err) {
>  			dev_err(&dev->dev, "Failed to allocate irq %d: %d\n",
>  				dev->irq, err);
> @@ -1275,7 +1276,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	err = i2c_add_adapter(&priv->adapter);
>  	if (err) {
>  		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
> -		goto exit_free_irq;
> +		goto exit_release;
>  	}
>  
>  	i801_probe_optional_slaves(priv);
> @@ -1286,12 +1287,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  
>  	return 0;
>  
> -exit_free_irq:
> -	if (priv->features & FEATURE_IRQ)
> -		free_irq(dev->irq, priv);
> +exit_release:
>  	pci_release_region(dev, SMBBAR);
>  exit:
> -	kfree(priv);
>  	return err;
>  }
>  
> @@ -1303,11 +1301,8 @@ static void i801_remove(struct pci_dev *dev)
>  	i2c_del_adapter(&priv->adapter);
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  
> -	if (priv->features & FEATURE_IRQ)
> -		free_irq(dev->irq, priv);
>  	pci_release_region(dev, SMBBAR);
>  
> -	kfree(priv);
>  	/*
>  	 * do not call pci_disable_device(dev) since it can cause hard hangs on
>  	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)

Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCHv2 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()

[Jean Delvare]

Hi Jarkko,

On Wed, 11 Feb 2015 14:32:07 +0200, Jarkko Nikula wrote:
> Since pci_disable_device() is not called from i801_suspend() and power
> state is set already it means that subsequent pci_enable_device() calls do
> practically nothing but monotonically increase struct pci_dev enable_cnt.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index b1d725d758bb..5fb35464f693 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1324,7 +1324,7 @@ static int i801_resume(struct pci_dev *dev)
>  {
>  	pci_set_power_state(dev, PCI_D0);
>  	pci_restore_state(dev);
> -	return pci_enable_device(dev);
> +	return 0;
>  }
>  #else
>  #define i801_suspend NULL

This looks reasonable but have you tested this change on a range of
actual laptops to make sure it has no unexpected side effect?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCHv2 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation

[Jean Delvare]

Hi Jarkko,

On Wed, 11 Feb 2015 14:32:08 +0200, Jarkko Nikula wrote:
> Simplifies the code a bit and makes easier to disable PCI device on driver
> detach by removing the pcim_pin_device() call in the future if needed.
> 
> Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it
> made some systems to hang during power-off. See commit d6fcb3b9cf77
> ("[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled")
> and
> http://marc.info/?l=linux-kernel&m=115160053309535&w=2
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

The checkpatch script complains:

WARNING: line over 80 characters
#90: FILE: drivers/i2c/busses/i2c-i801.c:1206:
+	err = pcim_iomap_regions(dev, 1 << SMBBAR, dev_driver_string(&dev->dev));

> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5fb35464f693..9f7b69743233 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1180,12 +1180,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	}
>  	priv->features &= ~disable_features;
>  
> -	err = pci_enable_device(dev);
> +	err = pcim_enable_device(dev);
>  	if (err) {
>  		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
>  			err);
>  		goto exit;
>  	}
> +	pcim_pin_device(dev);
>  
>  	/* Determine the address of the SMBus area */
>  	priv->smba = pci_resource_start(dev, SMBBAR);

What is the benefit of this change, compared to just leaving the call
to pci_enable_device() in place?

> @@ -1202,7 +1203,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		goto exit;
>  	}
>  
> -	err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev));
> +	err = pcim_iomap_regions(dev, 1 << SMBBAR, dev_driver_string(&dev->dev));
>  	if (err) {
>  		dev_err(&dev->dev,
>  			"Failed to request SMBus region 0x%lx-0x%Lx\n",
> @@ -1276,7 +1277,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	err = i2c_add_adapter(&priv->adapter);
>  	if (err) {
>  		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
> -		goto exit_release;
> +		goto exit;
>  	}
>  
>  	i801_probe_optional_slaves(priv);
> @@ -1287,8 +1288,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  
>  	return 0;
>  
> -exit_release:
> -	pci_release_region(dev, SMBBAR);
>  exit:
>  	return err;
>  }

Now that the exit path is empty, wouldn't it make sense to return
directly on error? My understanding is that this is one of the benefits
of managed device resources.

> @@ -1301,8 +1300,6 @@ static void i801_remove(struct pci_dev *dev)
>  	i2c_del_adapter(&priv->adapter);
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  
> -	pci_release_region(dev, SMBBAR);
> -
>  	/*
>  	 * do not call pci_disable_device(dev) since it can cause hard hangs on
>  	 * some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCHv2 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()

[Jarkko Nikula]

Hi

On 02/13/2015 12:33 PM, Jean Delvare wrote:
> Hi Jarkko,
>
> On Wed, 11 Feb 2015 14:32:07 +0200, Jarkko Nikula wrote:
>> Since pci_disable_device() is not called from i801_suspend() and power
>> state is set already it means that subsequent pci_enable_device() calls do
>> practically nothing but monotonically increase struct pci_dev enable_cnt.
>>
>> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>   drivers/i2c/busses/i2c-i801.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index b1d725d758bb..5fb35464f693 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1324,7 +1324,7 @@ static int i801_resume(struct pci_dev *dev)
>>   {
>>   	pci_set_power_state(dev, PCI_D0);
>>   	pci_restore_state(dev);
>> -	return pci_enable_device(dev);
>> +	return 0;
>>   }
>>   #else
>>   #define i801_suspend NULL
>
> This looks reasonable but have you tested this change on a range of
> actual laptops to make sure it has no unexpected side effect?
>
Unfortunately I have only limited amount of test hw which are working 
fine even if PCI device is disabled so I cannot hit those issues that 
were seen in the past.

I suppose this patch unlikely cause regression since if you look at the 
call chain pci_enable_device() -> pci_enable_device_flags() it doesn't 
go beyond taking the current power state since enable_cnt is always 
greater than 1.

drivers/pci/pci.c: pci_enable_device_flags():

if (dev->pm_cap) {
	u16 pmcsr;
	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
	dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
}

if (atomic_inc_return(&dev->enable_cnt) > 1)
	return 0;		/* already enabled */

To me it seems current power state taking is practically no-op when 
device is enabled since pci_set_power_state() calls in i801_suspend() 
and i801_resume() have already cached it.

-- 
Jarkko

Re: [PATCHv2 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation

[Jarkko Nikula]

Hi

On 02/13/2015 12:47 PM, Jean Delvare wrote:
> Hi Jarkko,
>
> On Wed, 11 Feb 2015 14:32:08 +0200, Jarkko Nikula wrote:
>> Simplifies the code a bit and makes easier to disable PCI device on driver
>> detach by removing the pcim_pin_device() call in the future if needed.
>>
>> Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it
>> made some systems to hang during power-off. See commit d6fcb3b9cf77
>> ("[PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled")
>> and
>> http://marc.info/?l=linux-kernel&m=115160053309535&w=2
>>
>> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>   drivers/i2c/busses/i2c-i801.c | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 deletions(-)
>
> The checkpatch script complains:
>
> WARNING: line over 80 characters
> #90: FILE: drivers/i2c/busses/i2c-i801.c:1206:
> +	err = pcim_iomap_regions(dev, 1 << SMBBAR, dev_driver_string(&dev->dev));
>
Ah, I was blind to see this. Will fix.

>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 5fb35464f693..9f7b69743233 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1180,12 +1180,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>   	}
>>   	priv->features &= ~disable_features;
>>
>> -	err = pci_enable_device(dev);
>> +	err = pcim_enable_device(dev);
>>   	if (err) {
>>   		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
>>   			err);
>>   		goto exit;
>>   	}
>> +	pcim_pin_device(dev);
>>
>>   	/* Determine the address of the SMBus area */
>>   	priv->smba = pci_resource_start(dev, SMBBAR);
>
> What is the benefit of this change, compared to just leaving the call
> to pci_enable_device() in place?
>
If you mean the patch itself I think easy pcim_pin_device() removal is 
the biggest benefit if we ever want to experiment does hang during 
power-off problem still exist in some machines. All PCI, ACPI and 
power-off code have evolved in 9 years so original problem may not exist 
anymore. Who knows.

Without this patch pcm_disable_device() needs to be added to error paths 
and i801_remove(). (of course trivial but it's always more nice to get 
regression from one-liner).

If you mean plain pcim_enable_device() it is required for other pcim_ 
functions since it allocates pcim_release which takes care of cleanup 
bunch of things including pci_release_region().

E.g. pcim_iomap_regions() without pcim_enable_device() causes an oops 
when doing "cat /proc/ioports" after module removal since 
pcim_iomap_release() only unmaps but does not release regions.

>> @@ -1202,7 +1203,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>   		goto exit;
>>   	}
>>
>> -	err = pci_request_region(dev, SMBBAR, dev_driver_string(&dev->dev));
>> +	err = pcim_iomap_regions(dev, 1 << SMBBAR, dev_driver_string(&dev->dev));
>>   	if (err) {
>>   		dev_err(&dev->dev,
>>   			"Failed to request SMBus region 0x%lx-0x%Lx\n",
>> @@ -1276,7 +1277,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>   	err = i2c_add_adapter(&priv->adapter);
>>   	if (err) {
>>   		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
>> -		goto exit_release;
>> +		goto exit;
>>   	}
>>
>>   	i801_probe_optional_slaves(priv);
>> @@ -1287,8 +1288,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>
>>   	return 0;
>>
>> -exit_release:
>> -	pci_release_region(dev, SMBBAR);
>>   exit:
>>   	return err;
>>   }
>
> Now that the exit path is empty, wouldn't it make sense to return
> directly on error? My understanding is that this is one of the benefits
> of managed device resources.
>
Yes it does. Will fix too.

-- 
Jarkko

Re: [PATCHv2 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation

[Jean Delvare]

On Fri, 13 Feb 2015 13:47:07 +0200, Jarkko Nikula wrote:
> On 02/13/2015 12:47 PM, Jean Delvare wrote:
> > On Wed, 11 Feb 2015 14:32:08 +0200, Jarkko Nikula wrote:
> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >> index 5fb35464f693..9f7b69743233 100644
> >> --- a/drivers/i2c/busses/i2c-i801.c
> >> +++ b/drivers/i2c/busses/i2c-i801.c
> >> @@ -1180,12 +1180,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>   	}
> >>   	priv->features &= ~disable_features;
> >>
> >> -	err = pci_enable_device(dev);
> >> +	err = pcim_enable_device(dev);
> >>   	if (err) {
> >>   		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
> >>   			err);
> >>   		goto exit;
> >>   	}
> >> +	pcim_pin_device(dev);
> >>
> >>   	/* Determine the address of the SMBus area */
> >>   	priv->smba = pci_resource_start(dev, SMBBAR);
> >
> > What is the benefit of this change, compared to just leaving the call
> > to pci_enable_device() in place?
>
> If you mean the patch itself I think easy pcim_pin_device() removal is 
> the biggest benefit if we ever want to experiment does hang during 
> power-off problem still exist in some machines. All PCI, ACPI and 
> power-off code have evolved in 9 years so original problem may not exist 
> anymore. Who knows.
> 
> Without this patch pcm_disable_device() needs to be added to error paths 
> and i801_remove(). (of course trivial but it's always more nice to get 
> regression from one-liner).
> 
> If you mean plain pcim_enable_device() it is required for other pcim_ 
> functions since it allocates pcim_release which takes care of cleanup 
> bunch of things including pci_release_region().
> 
> E.g. pcim_iomap_regions() without pcim_enable_device() causes an oops 
> when doing "cat /proc/ioports" after module removal since 
> pcim_iomap_release() only unmaps but does not release regions.

I meant the latter. If there's a good reason for calling
pcim_enable_device then alright.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCHv2 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()

[Jean Delvare]

Hi Jarkko,

On Fri, 13 Feb 2015 13:13:18 +0200, Jarkko Nikula wrote:
> On 02/13/2015 12:33 PM, Jean Delvare wrote:
> > This looks reasonable but have you tested this change on a range of
> > actual laptops to make sure it has no unexpected side effect?
>
> Unfortunately I have only limited amount of test hw which are working 
> fine even if PCI device is disabled so I cannot hit those issues that 
> were seen in the past.
> 
> I suppose this patch unlikely cause regression since if you look at the 
> call chain pci_enable_device() -> pci_enable_device_flags() it doesn't 
> go beyond taking the current power state since enable_cnt is always 
> greater than 1.
> 
> drivers/pci/pci.c: pci_enable_device_flags():
> 
> if (dev->pm_cap) {
> 	u16 pmcsr;
> 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> 	dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> }
> 
> if (atomic_inc_return(&dev->enable_cnt) > 1)
> 	return 0;		/* already enabled */
> 
> To me it seems current power state taking is practically no-op when 
> device is enabled since pci_set_power_state() calls in i801_suspend() 
> and i801_resume() have already cached it.

OK, you convinced me. I'll still test the updated driver on my two
IBM/Lenovo Thinkpad laptops (T60p and X230) to make sure we didn't miss
anything.

Thanks,
-- 
Jean Delvare
SUSE L3 Support