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@xxxxxxxxxxxxxxx> --- 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.
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.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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
