[PATCH v3] i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR

[PATCH v3] i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR

[Mika Westerberg]

Many Intel systems the BIOS declares a SystemIO OpRegion below the SMBus
PCI device as can be seen in ACPI DSDT table from Lenovo Yoga 900:

  Device (SBUS)
  {
      OperationRegion (SMBI, SystemIO, (SBAR << 0x05), 0x10)
      Field (SMBI, ByteAcc, NoLock, Preserve)
      {
          HSTS,   8,
          Offset (0x02),
          HCON,   8,
          HCOM,   8,
          TXSA,   8,
          DAT0,   8,
          DAT1,   8,
          HBDR,   8,
          PECR,   8,
          RXSA,   8,
          SDAT,   16
      }

There are also bunch of AML methods that that the BIOS can use to access
these fields. Most of the systems in question AML methods accessing the
SMBI OpRegion are never used.

Now, because of this SMBI OpRegion many systems fail to load the SMBus
driver with an error looking like one below:

  ACPI Warning: SystemIO range 0x0000000000003040-0x000000000000305F
       conflicts with OpRegion 0x0000000000003040-0x000000000000304F
       (\_SB.PCI0.SBUS.SMBI) (20160108/utaddress-255)
  ACPI: If an ACPI driver is available for this device, you should use
       it instead of the native driver

The reason is that this SMBI OpRegion conflicts with the PCI BAR used by
the SMBus driver.

It turns out that we can install a custom SystemIO address space handler
for the SMBus device to intercept all accesses through that OpRegion. This
allows us to share the PCI BAR with the AML code if it for some reason is
using it. We do not expect that this OpRegion handler will ever be called
but if it is we print a warning and prevent all access from the SMBus
driver itself.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=110041
Reported-by: Andy Lutomirski <luto@kernel.org>
Reported-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: stable@vger.kernel.org
---
Changes to v2:

 - Return -EIO instead of -EPERM
 - Added ACK from Rafael
 - Added Link and Reported-by tags
 - Tagged for stable inclusion

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5652bf6ce9be..d69ad96460b5 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -247,6 +247,13 @@ struct i801_priv {
 	struct platform_device *mux_pdev;
 #endif
 	struct platform_device *tco_pdev;
+
+	/*
+	 * If set to true the host controller registers are reserved for
+	 * ACPI AML use. Protected by acpi_lock.
+	 */
+	bool acpi_reserved;
+	struct mutex acpi_lock;
 };
 
 #define FEATURE_SMBUS_PEC	(1 << 0)
@@ -720,6 +727,12 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	int ret = 0, xact = 0;
 	struct i801_priv *priv = i2c_get_adapdata(adap);
 
+	mutex_lock(&priv->acpi_lock);
+	if (priv->acpi_reserved) {
+		mutex_unlock(&priv->acpi_lock);
+		return -EIO;
+	}
+
 	pm_runtime_get_sync(&priv->pci_dev->dev);
 
 	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
@@ -822,6 +835,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 out:
 	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
 	pm_runtime_put_autosuspend(&priv->pci_dev->dev);
+	mutex_unlock(&priv->acpi_lock);
 	return ret;
 }
 
@@ -1260,6 +1274,89 @@ static void i801_add_tco(struct i801_priv *priv)
 	priv->tco_pdev = pdev;
 }
 
+#ifdef CONFIG_ACPI
+static acpi_status
+i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
+		     u64 *value, void *handler_context, void *region_context)
+{
+	struct i801_priv *priv = handler_context;
+	struct pci_dev *pdev = priv->pci_dev;
+	acpi_status status;
+
+	/*
+	 * Once BIOS AML code touches the OpRegion we warn and inhibit any
+	 * further access from the driver itself. This device is now owned
+	 * by the system firmware.
+	 */
+	dev_warn_once(&pdev->dev, "BIOS is accessing SMBus registers\n");
+	dev_warn_once(&pdev->dev, "Driver SMBus register access inhibited\n");
+
+	mutex_lock(&priv->acpi_lock);
+
+	if (!priv->acpi_reserved) {
+		priv->acpi_reserved = true;
+
+		/*
+		 * BIOS is accessing the host controller so prevent it from
+		 * suspending automatically from now on.
+		 */
+		pm_runtime_get_sync(&pdev->dev);
+	}
+
+	if (function == ACPI_READ) {
+		u32 val = (u32)*value;
+		status = acpi_os_read_port(address, &val, bits);
+		if (ACPI_SUCCESS(status))
+			*value = val;
+	} else {
+		status = acpi_os_write_port(address, (u32)*value, bits);
+	}
+
+	mutex_unlock(&priv->acpi_lock);
+
+	return status;
+}
+
+static int i801_acpi_probe(struct i801_priv *priv)
+{
+	struct acpi_device *adev;
+	acpi_status status;
+
+	adev = ACPI_COMPANION(&priv->pci_dev->dev);
+	if (adev) {
+		status = acpi_install_address_space_handler(adev->handle,
+				ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler,
+				NULL, priv);
+		if (ACPI_SUCCESS(status))
+			return 0;
+	}
+
+	return acpi_check_resource_conflict(&priv->pci_dev->resource[SMBBAR]);
+}
+
+static void i801_acpi_remove(struct i801_priv *priv)
+{
+	struct acpi_device *adev;
+
+	adev = ACPI_COMPANION(&priv->pci_dev->dev);
+	if (!adev)
+		return;
+
+	acpi_remove_address_space_handler(adev->handle,
+		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
+
+	mutex_lock(&priv->acpi_lock);
+	if (priv->acpi_reserved) {
+		priv->acpi_reserved = false;
+		pm_runtime_put(&priv->pci_dev->dev);
+	}
+	mutex_unlock(&priv->acpi_lock);
+}
+#else
+static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }
+static inline void i801_acpi_remove(struct i801_priv *priv) { }
+#endif
+
 static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	unsigned char temp;
@@ -1277,6 +1374,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	priv->adapter.dev.parent = &dev->dev;
 	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
 	priv->adapter.retries = 3;
+	mutex_init(&priv->acpi_lock);
 
 	priv->pci_dev = dev;
 	switch (dev->device) {
@@ -1339,10 +1437,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return -ENODEV;
 	}
 
-	err = acpi_check_resource_conflict(&dev->resource[SMBBAR]);
-	if (err) {
-		return -ENODEV;
-	}
+	err = i801_acpi_probe(priv);
+	if (err)
+		return err;
 
 	err = pcim_iomap_regions(dev, 1 << SMBBAR,
 				 dev_driver_string(&dev->dev));
@@ -1439,6 +1536,7 @@ static void i801_remove(struct pci_dev *dev)
 	pm_runtime_forbid(&dev->dev);
 	pm_runtime_get_noresume(&dev->dev);
 
+	i801_acpi_remove(priv);
 	i801_del_mux(priv);
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
-- 
2.8.1

Re: [PATCH v3] i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2458 bytes --]

On Monday 09 May 2016 10:17:14 Mika Westerberg wrote:
> Many Intel systems the BIOS declares a SystemIO OpRegion below the
> SMBus PCI device as can be seen in ACPI DSDT table from Lenovo Yoga
> 900:
> 
>   Device (SBUS)
>   {
>       OperationRegion (SMBI, SystemIO, (SBAR << 0x05), 0x10)
>       Field (SMBI, ByteAcc, NoLock, Preserve)
>       {
>           HSTS,   8,
>           Offset (0x02),
>           HCON,   8,
>           HCOM,   8,
>           TXSA,   8,
>           DAT0,   8,
>           DAT1,   8,
>           HBDR,   8,
>           PECR,   8,
>           RXSA,   8,
>           SDAT,   16
>       }
> 
> There are also bunch of AML methods that that the BIOS can use to
> access these fields. Most of the systems in question AML methods
> accessing the SMBI OpRegion are never used.
> 
> Now, because of this SMBI OpRegion many systems fail to load the
> SMBus driver with an error looking like one below:
> 
>   ACPI Warning: SystemIO range 0x0000000000003040-0x000000000000305F
>        conflicts with OpRegion 0x0000000000003040-0x000000000000304F
>        (\_SB.PCI0.SBUS.SMBI) (20160108/utaddress-255)
>   ACPI: If an ACPI driver is available for this device, you should
> use it instead of the native driver
> 
> The reason is that this SMBI OpRegion conflicts with the PCI BAR used
> by the SMBus driver.
> 
> It turns out that we can install a custom SystemIO address space
> handler for the SMBus device to intercept all accesses through that
> OpRegion. This allows us to share the PCI BAR with the AML code if
> it for some reason is using it. We do not expect that this OpRegion
> handler will ever be called but if it is we print a warning and
> prevent all access from the SMBus driver itself.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=110041
> Reported-by: Andy Lutomirski <luto@kernel.org>
> Reported-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: stable@vger.kernel.org
> ---
> Changes to v2:
> 
>  - Return -EIO instead of -EPERM
>  - Added ACK from Rafael
>  - Added Link and Reported-by tags
>  - Tagged for stable inclusion

Working fine on my Latitude E6440, add my:

Tested-By: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR

[Jean Delvare]

Hi Mika,

First of all, thank you very much for working on this problem, this is
highly appreciated.

On Mon,  9 May 2016 11:17:14 +0300, Mika Westerberg wrote:
> Many Intel systems the BIOS declares a SystemIO OpRegion below the SMBus
> PCI device as can be seen in ACPI DSDT table from Lenovo Yoga 900:
> 
>   Device (SBUS)
>   {
>       OperationRegion (SMBI, SystemIO, (SBAR << 0x05), 0x10)
>       Field (SMBI, ByteAcc, NoLock, Preserve)
>       {
>           HSTS,   8,
>           Offset (0x02),
>           HCON,   8,
>           HCOM,   8,
>           TXSA,   8,
>           DAT0,   8,
>           DAT1,   8,
>           HBDR,   8,
>           PECR,   8,
>           RXSA,   8,
>           SDAT,   16
>       }
> 
> There are also bunch of AML methods that that the BIOS can use to access
> these fields. Most of the systems in question AML methods accessing the
> SMBI OpRegion are never used.
> 
> Now, because of this SMBI OpRegion many systems fail to load the SMBus
> driver with an error looking like one below:
> 
>   ACPI Warning: SystemIO range 0x0000000000003040-0x000000000000305F
>        conflicts with OpRegion 0x0000000000003040-0x000000000000304F
>        (\_SB.PCI0.SBUS.SMBI) (20160108/utaddress-255)
>   ACPI: If an ACPI driver is available for this device, you should use
>        it instead of the native driver
> 
> The reason is that this SMBI OpRegion conflicts with the PCI BAR used by
> the SMBus driver.
> 
> It turns out that we can install a custom SystemIO address space handler
> for the SMBus device to intercept all accesses through that OpRegion. This
> allows us to share the PCI BAR with the AML code if it for some reason is
> using it. We do not expect that this OpRegion handler will ever be called
> but if it is we print a warning and prevent all access from the SMBus
> driver itself.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=110041
> Reported-by: Andy Lutomirski <luto@kernel.org>
> Reported-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: stable@vger.kernel.org
> ---
> Changes to v2:
> 
>  - Return -EIO instead of -EPERM
>  - Added ACK from Rafael
>  - Added Link and Reported-by tags
>  - Tagged for stable inclusion
> 
>  drivers/i2c/busses/i2c-i801.c | 106 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 102 insertions(+), 4 deletions(-)

I have tested this on my Dell OptiPlex 9020 MT system, and it works
well. Gives me access to the SPD EEPROMs on my memory modules.

Below is my review.

> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5652bf6ce9be..d69ad96460b5 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -247,6 +247,13 @@ struct i801_priv {
>  	struct platform_device *mux_pdev;
>  #endif
>  	struct platform_device *tco_pdev;
> +
> +	/*
> +	 * If set to true the host controller registers are reserved for
> +	 * ACPI AML use. Protected by acpi_lock.
> +	 */
> +	bool acpi_reserved;
> +	struct mutex acpi_lock;
>  };
>  
>  #define FEATURE_SMBUS_PEC	(1 << 0)
> @@ -720,6 +727,12 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	int ret = 0, xact = 0;
>  	struct i801_priv *priv = i2c_get_adapdata(adap);
>  
> +	mutex_lock(&priv->acpi_lock);
> +	if (priv->acpi_reserved) {
> +		mutex_unlock(&priv->acpi_lock);
> +		return -EIO;

I see this has been discussed before, but I don't think EIO is
appropriate here. You didn't even try to issue an I/O to the device, so
how could it fail? EBUSY would better reflect the situation IMHO.

> +	}
> +
>  	pm_runtime_get_sync(&priv->pci_dev->dev);
>  
>  	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
> @@ -822,6 +835,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  out:
>  	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
>  	pm_runtime_put_autosuspend(&priv->pci_dev->dev);
> +	mutex_unlock(&priv->acpi_lock);
>  	return ret;
>  }
>  
> @@ -1260,6 +1274,89 @@ static void i801_add_tco(struct i801_priv *priv)
>  	priv->tco_pdev = pdev;
>  }
>  
> +#ifdef CONFIG_ACPI
> +static acpi_status
> +i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
> +		     u64 *value, void *handler_context, void *region_context)
> +{
> +	struct i801_priv *priv = handler_context;
> +	struct pci_dev *pdev = priv->pci_dev;
> +	acpi_status status;
> +
> +	/*
> +	 * Once BIOS AML code touches the OpRegion we warn and inhibit any
> +	 * further access from the driver itself. This device is now owned
> +	 * by the system firmware.
> +	 */
> +	dev_warn_once(&pdev->dev, "BIOS is accessing SMBus registers\n");
> +	dev_warn_once(&pdev->dev, "Driver SMBus register access inhibited\n");

Given that you have priv->acpi_reserved to record if we've been there
before, maybe you could move the warnings below, and use simple
dev_warn? I suspect it's cheaper than dev_warn_once.

> +
> +	mutex_lock(&priv->acpi_lock);
> +
> +	if (!priv->acpi_reserved) {
> +		priv->acpi_reserved = true;
> +
> +		/*
> +		 * BIOS is accessing the host controller so prevent it from
> +		 * suspending automatically from now on.
> +		 */
> +		pm_runtime_get_sync(&pdev->dev);
> +	}
> +
> +	if (function == ACPI_READ) {
> +		u32 val = (u32)*value;

I'm confused. acpi_os_read_port is writing the result of the read to
"val", it doesn't read from it, so I don't think it needs to be
initialized?

Also, looking at the acpi_os_read_port() call in
drivers/acpi/apei/apei-base.c, it would seem you can cast *value
directly in-place, without using a temporary variable. This would limit
the overhead.

> +		status = acpi_os_read_port(address, &val, bits);
> +		if (ACPI_SUCCESS(status))
> +			*value = val;
> +	} else {
> +		status = acpi_os_write_port(address, (u32)*value, bits);
> +	}
> +
> +	mutex_unlock(&priv->acpi_lock);
> +
> +	return status;
> +}
> +
> +static int i801_acpi_probe(struct i801_priv *priv)
> +{
> +	struct acpi_device *adev;
> +	acpi_status status;
> +
> +	adev = ACPI_COMPANION(&priv->pci_dev->dev);
> +	if (adev) {
> +		status = acpi_install_address_space_handler(adev->handle,
> +				ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler,
> +				NULL, priv);
> +		if (ACPI_SUCCESS(status))
> +			return 0;
> +	}
> +
> +	return acpi_check_resource_conflict(&priv->pci_dev->resource[SMBBAR]);
> +}
> +
> +static void i801_acpi_remove(struct i801_priv *priv)
> +{
> +	struct acpi_device *adev;
> +
> +	adev = ACPI_COMPANION(&priv->pci_dev->dev);
> +	if (!adev)
> +		return;
> +
> +	acpi_remove_address_space_handler(adev->handle,
> +		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
> +
> +	mutex_lock(&priv->acpi_lock);
> +	if (priv->acpi_reserved) {
> +		priv->acpi_reserved = false;

Is this actually needed? priv is about to be destroyed anyway.

> +		pm_runtime_put(&priv->pci_dev->dev);
> +	}
> +	mutex_unlock(&priv->acpi_lock);
> +}
> +#else
> +static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }
> +static inline void i801_acpi_remove(struct i801_priv *priv) { }
> +#endif
> +
>  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	unsigned char temp;
> @@ -1277,6 +1374,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	priv->adapter.dev.parent = &dev->dev;
>  	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
>  	priv->adapter.retries = 3;
> +	mutex_init(&priv->acpi_lock);
>  
>  	priv->pci_dev = dev;
>  	switch (dev->device) {
> @@ -1339,10 +1437,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		return -ENODEV;
>  	}
>  
> -	err = acpi_check_resource_conflict(&dev->resource[SMBBAR]);
> -	if (err) {
> -		return -ENODEV;
> -	}
> +	err = i801_acpi_probe(priv);
> +	if (err)
> +		return err;

Before your patch we would return -ENODEV in case of conflict. Now we
are returning -EBUSY instead. I see no reason for this change. Or if
you think this is actually needed, that seems independent from what
your patch is doing, so it should be a separate patch.

>  
>  	err = pcim_iomap_regions(dev, 1 << SMBBAR,
>  				 dev_driver_string(&dev->dev));
> @@ -1439,6 +1536,7 @@ static void i801_remove(struct pci_dev *dev)
>  	pm_runtime_forbid(&dev->dev);
>  	pm_runtime_get_noresume(&dev->dev);
>  
> +	i801_acpi_remove(priv);
>  	i801_del_mux(priv);
>  	i2c_del_adapter(&priv->adapter);

This looks racy. Until i2c_del_adapter() is called, the SMBus may be
used. So I think you should call i801_acpi_remove() after
i2c_del_adapter().

>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);

Thanks again,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v3] i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR

[Mika Westerberg]

On Wed, May 11, 2016 at 11:39:33AM +0200, Jean Delvare wrote:
> Hi Mika,
> 
> First of all, thank you very much for working on this problem, this is
> highly appreciated.

You're welcome :)

> On Mon,  9 May 2016 11:17:14 +0300, Mika Westerberg wrote:
> > Many Intel systems the BIOS declares a SystemIO OpRegion below the SMBus
> > PCI device as can be seen in ACPI DSDT table from Lenovo Yoga 900:
> > 
> >   Device (SBUS)
> >   {
> >       OperationRegion (SMBI, SystemIO, (SBAR << 0x05), 0x10)
> >       Field (SMBI, ByteAcc, NoLock, Preserve)
> >       {
> >           HSTS,   8,
> >           Offset (0x02),
> >           HCON,   8,
> >           HCOM,   8,
> >           TXSA,   8,
> >           DAT0,   8,
> >           DAT1,   8,
> >           HBDR,   8,
> >           PECR,   8,
> >           RXSA,   8,
> >           SDAT,   16
> >       }
> > 
> > There are also bunch of AML methods that that the BIOS can use to access
> > these fields. Most of the systems in question AML methods accessing the
> > SMBI OpRegion are never used.
> > 
> > Now, because of this SMBI OpRegion many systems fail to load the SMBus
> > driver with an error looking like one below:
> > 
> >   ACPI Warning: SystemIO range 0x0000000000003040-0x000000000000305F
> >        conflicts with OpRegion 0x0000000000003040-0x000000000000304F
> >        (\_SB.PCI0.SBUS.SMBI) (20160108/utaddress-255)
> >   ACPI: If an ACPI driver is available for this device, you should use
> >        it instead of the native driver
> > 
> > The reason is that this SMBI OpRegion conflicts with the PCI BAR used by
> > the SMBus driver.
> > 
> > It turns out that we can install a custom SystemIO address space handler
> > for the SMBus device to intercept all accesses through that OpRegion. This
> > allows us to share the PCI BAR with the AML code if it for some reason is
> > using it. We do not expect that this OpRegion handler will ever be called
> > but if it is we print a warning and prevent all access from the SMBus
> > driver itself.
> > 
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=110041
> > Reported-by: Andy Lutomirski <luto@kernel.org>
> > Reported-by: Pali Rohár <pali.rohar@gmail.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> > Changes to v2:
> > 
> >  - Return -EIO instead of -EPERM
> >  - Added ACK from Rafael
> >  - Added Link and Reported-by tags
> >  - Tagged for stable inclusion
> > 
> >  drivers/i2c/busses/i2c-i801.c | 106 ++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 102 insertions(+), 4 deletions(-)
> 
> I have tested this on my Dell OptiPlex 9020 MT system, and it works
> well. Gives me access to the SPD EEPROMs on my memory modules.

Good to know. Thanks for testing.

> Below is my review.
> 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index 5652bf6ce9be..d69ad96460b5 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -247,6 +247,13 @@ struct i801_priv {
> >  	struct platform_device *mux_pdev;
> >  #endif
> >  	struct platform_device *tco_pdev;
> > +
> > +	/*
> > +	 * If set to true the host controller registers are reserved for
> > +	 * ACPI AML use. Protected by acpi_lock.
> > +	 */
> > +	bool acpi_reserved;
> > +	struct mutex acpi_lock;
> >  };
> >  
> >  #define FEATURE_SMBUS_PEC	(1 << 0)
> > @@ -720,6 +727,12 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> >  	int ret = 0, xact = 0;
> >  	struct i801_priv *priv = i2c_get_adapdata(adap);
> >  
> > +	mutex_lock(&priv->acpi_lock);
> > +	if (priv->acpi_reserved) {
> > +		mutex_unlock(&priv->acpi_lock);
> > +		return -EIO;
> 
> I see this has been discussed before, but I don't think EIO is
> appropriate here. You didn't even try to issue an I/O to the device, so
> how could it fail? EBUSY would better reflect the situation IMHO.

That works for me as well. Rafael suggested -EIO and it was originally
-EPERM but I have no problems changing it to return -EBUSY.

> > +	}
> > +
> >  	pm_runtime_get_sync(&priv->pci_dev->dev);
> >  
> >  	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
> > @@ -822,6 +835,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> >  out:
> >  	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
> >  	pm_runtime_put_autosuspend(&priv->pci_dev->dev);
> > +	mutex_unlock(&priv->acpi_lock);
> >  	return ret;
> >  }
> >  
> > @@ -1260,6 +1274,89 @@ static void i801_add_tco(struct i801_priv *priv)
> >  	priv->tco_pdev = pdev;
> >  }
> >  
> > +#ifdef CONFIG_ACPI
> > +static acpi_status
> > +i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
> > +		     u64 *value, void *handler_context, void *region_context)
> > +{
> > +	struct i801_priv *priv = handler_context;
> > +	struct pci_dev *pdev = priv->pci_dev;
> > +	acpi_status status;
> > +
> > +	/*
> > +	 * Once BIOS AML code touches the OpRegion we warn and inhibit any
> > +	 * further access from the driver itself. This device is now owned
> > +	 * by the system firmware.
> > +	 */
> > +	dev_warn_once(&pdev->dev, "BIOS is accessing SMBus registers\n");
> > +	dev_warn_once(&pdev->dev, "Driver SMBus register access inhibited\n");
> 
> Given that you have priv->acpi_reserved to record if we've been there
> before, maybe you could move the warnings below, and use simple
> dev_warn? I suspect it's cheaper than dev_warn_once.

OK, I'll move them to the block below.

> > +
> > +	mutex_lock(&priv->acpi_lock);
> > +
> > +	if (!priv->acpi_reserved) {
> > +		priv->acpi_reserved = true;
> > +
> > +		/*
> > +		 * BIOS is accessing the host controller so prevent it from
> > +		 * suspending automatically from now on.
> > +		 */
> > +		pm_runtime_get_sync(&pdev->dev);
> > +	}
> > +
> > +	if (function == ACPI_READ) {
> > +		u32 val = (u32)*value;
> 
> I'm confused. acpi_os_read_port is writing the result of the read to
> "val", it doesn't read from it, so I don't think it needs to be
> initialized?

Indeed, that's not needed at all.

> Also, looking at the acpi_os_read_port() call in
> drivers/acpi/apei/apei-base.c, it would seem you can cast *value
> directly in-place, without using a temporary variable. This would limit
> the overhead.

OK.

> > +		status = acpi_os_read_port(address, &val, bits);
> > +		if (ACPI_SUCCESS(status))
> > +			*value = val;
> > +	} else {
> > +		status = acpi_os_write_port(address, (u32)*value, bits);
> > +	}
> > +
> > +	mutex_unlock(&priv->acpi_lock);
> > +
> > +	return status;
> > +}
> > +
> > +static int i801_acpi_probe(struct i801_priv *priv)
> > +{
> > +	struct acpi_device *adev;
> > +	acpi_status status;
> > +
> > +	adev = ACPI_COMPANION(&priv->pci_dev->dev);
> > +	if (adev) {
> > +		status = acpi_install_address_space_handler(adev->handle,
> > +				ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler,
> > +				NULL, priv);
> > +		if (ACPI_SUCCESS(status))
> > +			return 0;
> > +	}
> > +
> > +	return acpi_check_resource_conflict(&priv->pci_dev->resource[SMBBAR]);
> > +}
> > +
> > +static void i801_acpi_remove(struct i801_priv *priv)
> > +{
> > +	struct acpi_device *adev;
> > +
> > +	adev = ACPI_COMPANION(&priv->pci_dev->dev);
> > +	if (!adev)
> > +		return;
> > +
> > +	acpi_remove_address_space_handler(adev->handle,
> > +		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
> > +
> > +	mutex_lock(&priv->acpi_lock);
> > +	if (priv->acpi_reserved) {
> > +		priv->acpi_reserved = false;
> 
> Is this actually needed? priv is about to be destroyed anyway.

It is not needed. I'll remove it.

> > +		pm_runtime_put(&priv->pci_dev->dev);
> > +	}
> > +	mutex_unlock(&priv->acpi_lock);
> > +}
> > +#else
> > +static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }
> > +static inline void i801_acpi_remove(struct i801_priv *priv) { }
> > +#endif
> > +
> >  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  {
> >  	unsigned char temp;
> > @@ -1277,6 +1374,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  	priv->adapter.dev.parent = &dev->dev;
> >  	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
> >  	priv->adapter.retries = 3;
> > +	mutex_init(&priv->acpi_lock);
> >  
> >  	priv->pci_dev = dev;
> >  	switch (dev->device) {
> > @@ -1339,10 +1437,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  		return -ENODEV;
> >  	}
> >  
> > -	err = acpi_check_resource_conflict(&dev->resource[SMBBAR]);
> > -	if (err) {
> > -		return -ENODEV;
> > -	}
> > +	err = i801_acpi_probe(priv);
> > +	if (err)
> > +		return err;
> 
> Before your patch we would return -ENODEV in case of conflict. Now we
> are returning -EBUSY instead. I see no reason for this change. Or if
> you think this is actually needed, that seems independent from what
> your patch is doing, so it should be a separate patch.

Returning -ENODEV seems to be right thing to do. I'll change it in the
next version.

> >  
> >  	err = pcim_iomap_regions(dev, 1 << SMBBAR,
> >  				 dev_driver_string(&dev->dev));
> > @@ -1439,6 +1536,7 @@ static void i801_remove(struct pci_dev *dev)
> >  	pm_runtime_forbid(&dev->dev);
> >  	pm_runtime_get_noresume(&dev->dev);
> >  
> > +	i801_acpi_remove(priv);
> >  	i801_del_mux(priv);
> >  	i2c_del_adapter(&priv->adapter);
> 
> This looks racy. Until i2c_del_adapter() is called, the SMBus may be
> used. So I think you should call i801_acpi_remove() after
> i2c_del_adapter().

That's right. I'll move the call to happen after i2c_del_adapter() is
called.

Thanks for the review.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html