i2c: i801: Few cleanups

PATCH Superseded Jarkko Nikula 2 participants 14 messages Feb 11, 2015 – Feb 16, 2015 9 messages not in the archive

Subsystems (2)

I2C subsystem host drivers I2C/smbus controller drivers for pc

lore

Share & tools

lore

Jarkko Nikula

author

2015-02-11 12:32 UTC #0 lore raw

v2 cover i2c: i801: Few cleanups

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(-)

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): 1423657928-25534-1-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]

Jarkko Nikula

author

2015-02-11 12:32 UTC #2 lore raw

PATCH v2 1/5 i2c: i801: Don't break user-visible strings

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/[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/i2c/busses/i2c-i801.c +5 -4

index 8fafb254e42a..7d1f4a478c54 100644
@@ -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;
 	}

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): 1423657928-25534-2-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]

Jean Delvare

Reviewed-by

2015-02-13 10:00 UTC #4 lore raw

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

Quoted 25 lines · 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/[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/i2c/busses/i2c-i801.c +2 -2

index 8fafb254e42a..7d1f4a478c54 100644
@@ -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)

drivers/i2c/busses/i2c-i801.c static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 1214

Quoted 10 lines

drivers/i2c/busses/i2c-i801.c +3 -2

 	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 <[email protected]>

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Jarkko Nikula

author

2015-02-11 12:32 UTC #5 lore raw

PATCH v2 2/5 i2c: i801: Remove i801_driver forward declaration

In reply to a message not in the archive

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/[email protected]>

Reviewed-by Jean Delvare <[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/i2c/busses/i2c-i801.c +2 -4

index 7d1f4a478c54..5f827dfc671a 100644
@@ -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);

Signature · 1 line

2.1.4

Jarkko Nikula

author

2015-02-11 12:32 UTC #6 lore raw

PATCH v2 3/5 i2c: i801: Use managed devm_* memory and irq allocation

In reply to a message not in the archive

This simplifies the error and remove paths.

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

drivers/i2c/busses/i2c-i801.c +6 ~~-11~~

index 5f827dfc671a..b1d725d758bb 100644
@@ -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)

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): 1423657928-25534-4-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]

Jean Delvare

Reviewed-by

2015-02-13 10:30 UTC #8 lore raw

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

Quoted 57 lines · Jarkko Nikula

This simplifies the error and remove paths.

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

drivers/i2c/busses/i2c-i801.c +6 -8

index 5f827dfc671a..b1d725d758bb 100644
@@ -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);

drivers/i2c/busses/i2c-i801.c static void i801_remove(struct pci_dev *dev) · line 1308

Quoted 10 lines

drivers/i2c/busses/i2c-i801.c +0 -3

 	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 <[email protected]>

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Jarkko Nikula

author

2015-02-11 12:32 UTC #9 lore raw

PATCH v2 4/5 i2c: i801: Remove pci_enable_device() call from i801_resume()

In reply to a message not in the archive

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/[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/i2c/busses/i2c-i801.c +1 -1

index b1d725d758bb..5fb35464f693 100644
@@ -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

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): 1423657928-25534-5-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]

Jean Delvare

2015-02-13 10:33 UTC #11 lore raw

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

Hi Jarkko,

Quoted 14 lines · 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/[email protected]>
---
 drivers/i2c/busses/i2c-i801.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-i801.c +0 -0
index b1d725d758bb..5fb35464f693 100644

drivers/i2c/busses/i2c-i801.c static int i801_resume(struct pci_dev *dev) · line 1330

Quoted 8 lines

drivers/i2c/busses/i2c-i801.c +1 -1

@@ -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?

Signature · 2 lines

Jean Delvare
SUSE L3 Support

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jarkko Nikula

author

2015-02-13 11:13 UTC #13 lore raw

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

Quoted 12 lines · Jean Delvare

Hi Jarkko,
Quoted 9 lines · 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/[email protected]>
---
  drivers/i2c/busses/i2c-i801.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 17 lines

drivers/i2c/busses/i2c-i801.c static int i801_resume(struct pci_dev *dev) · line 1330
Quoted 13 lines
drivers/i2c/busses/i2c-i801.c +1 -1
index b1d725d758bb..5fb35464f693 100644
@@ -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.

Signature · 1 line

Jarkko

A message that is not in the archive (sent off-list or not yet received): 54DDDC4E.5010602-VuQAYsv1563Yd54FQh9/[email protected]

Jean Delvare

2015-02-16 07:41 UTC #15 lore raw

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

Hi Jarkko,

Quoted 21 lines · Jarkko Nikula

Quoted 2 lines · Jean Delvare
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);
}

Quoted 6 lines

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,

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Jarkko Nikula

author

2015-02-11 12:32 UTC #16 lore raw

PATCH v2 5/5 i2c: i801: Use managed pcim_* PCI device initialization and reservation

In reply to a message not in the archive

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/[email protected]>

---
 drivers/i2c/busses/i2c-i801.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/i2c/busses/i2c-i801.c +4 -7

index 5fb35464f693..9f7b69743233 100644
@@ -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)

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): 1423657928-25534-6-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]

Jean Delvare

2015-02-13 10:47 UTC #18 lore raw

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

Hi Jarkko,

Quoted 9 lines · 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

Quoted 4 lines

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>
---
 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));

Quoted 9 lines

drivers/i2c/busses/i2c-i801.c +0 -1

index 5fb35464f693..9f7b69743233 100644
@@ -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);

drivers/i2c/busses/i2c-i801.c static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 1192

Quoted 10 lines

drivers/i2c/busses/i2c-i801.c +2 -0

+	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?

Quoted 19 lines

drivers/i2c/busses/i2c-i801.c +2 -2

@@ -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);

drivers/i2c/busses/i2c-i801.c static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 1293

Quoted 8 lines

drivers/i2c/busses/i2c-i801.c +0 -2

@@ -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.

Quoted 9 lines

drivers/i2c/busses/i2c-i801.c +0 -2

@@ -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)

Signature · 2 lines

Jean Delvare
SUSE L3 Support

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jarkko Nikula

author

2015-02-13 11:47 UTC #20 lore raw

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

Quoted 22 lines · Jean Delvare

Hi Jarkko,
Quoted 9 lines · 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
Quoted 4 lines
Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>
---
  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:

drivers/i2c/busses/i2c-i801.c static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 1206

Quoted 1 line

drivers/i2c/busses/i2c-i801.c +1 -0

+	err = pcim_iomap_regions(dev, 1 << SMBBAR, dev_driver_string(&dev->dev));

Ah, I was blind to see this. Will fix.

Quoted 17 lines

drivers/i2c/busses/i2c-i801.c +2 -1

index 5fb35464f693..9f7b69743233 100644
@@ -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);

Quoted 6 lines

drivers/i2c/busses/i2c-i801.c static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 1192
Quoted 2 lines
drivers/i2c/busses/i2c-i801.c +0 -0
  	/* 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.

Quoted 22 lines

drivers/i2c/busses/i2c-i801.c +2 -2

@@ -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;

Quoted 10 lines

drivers/i2c/busses/i2c-i801.c static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 1293
Quoted 5 lines
drivers/i2c/busses/i2c-i801.c +0 -2
-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.

Signature · 1 line

Jarkko

A message that is not in the archive (sent off-list or not yet received): 54DDE43B.70902-VuQAYsv1563Yd54FQh9/[email protected]

Jean Delvare

2015-02-13 15:20 UTC #22 lore raw

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

Quoted 35 lines · Jarkko Nikula

Quoted 18 lines · Jean Delvare
Quoted 17 lines · Jarkko Nikula
drivers/i2c/busses/i2c-i801.c +2 -1
index 5fb35464f693..9f7b69743233 100644
@@ -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);
 
Quoted 5 lines
drivers/i2c/busses/i2c-i801.c static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 1192
Quoted 2 lines
drivers/i2c/busses/i2c-i801.c +0 -0
  	/* 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).

Quoted 7 lines

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.

Signature · 2 lines

Jean Delvare
SUSE L3 Support