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

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-and-tested-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>
Tested-by: Jean Delvare <jdelvare@suse.de>
Reviewed-by: Jean Delvare <jdelvare@suse.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: stable@vger.kernel.org
---
Changes to v4:
 - Use ACPI_IO_MASK to mask function to get right value if in future we get
   something else added to that field.
 - Add Reviewed/Tested-by from Jean Delvare

Changes to v3:
 - Added Tested-by from Pali Rohár (The patch did not change that much so I
   though it is still valid)
 - Return -EBUSY instead of -EIO
 - Move dev_warns() to be inside if (!priv->acpi_reserved) block
 - Remove unnecessary variable "val"
 - Do not clear priv->acpi_reserved in i801_acpi_remove()
 - Return -ENODEV if we detect conflict
 - Call i801_acpi_remove() after i2c_del_adapter().

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 | 98 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 3 deletions(-)

On May 23 2016 or thereabouts, 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-and-tested-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>
> Tested-by: Jean Delvare <jdelvare@suse.de>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: stable@vger.kernel.org
> ---
> Changes to v4:
>  - Use ACPI_IO_MASK to mask function to get right value if in future we get
>    something else added to that field.
>  - Add Reviewed/Tested-by from Jean Delvare
> 
> Changes to v3:
>  - Added Tested-by from Pali Rohár (The patch did not change that much so I
>    though it is still valid)
>  - Return -EBUSY instead of -EIO
>  - Move dev_warns() to be inside if (!priv->acpi_reserved) block
>  - Remove unnecessary variable "val"
>  - Do not clear priv->acpi_reserved in i801_acpi_remove()
>  - Return -ENODEV if we detect conflict
>  - Call i801_acpi_remove() after i2c_del_adapter().
> 
> 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 | 98 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 95 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5652bf6ce9be..d613263d3f96 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 -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,83 @@ 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.
> +	 */
> +	mutex_lock(&priv->acpi_lock);
> +
> +	if (!priv->acpi_reserved) {
> +		priv->acpi_reserved = true;
> +
> +		dev_warn(&pdev->dev, "BIOS is accessing SMBus registers\n");
> +		dev_warn(&pdev->dev, "Driver SMBus register access inhibited\n");
> +
> +		/*
> +		 * BIOS is accessing the host controller so prevent it from
> +		 * suspending automatically from now on.
> +		 */
> +		pm_runtime_get_sync(&pdev->dev);
> +	}
> +
> +	if ((function & ACPI_IO_MASK) == ACPI_READ)
> +		status = acpi_os_read_port(address, (u32 *)value, bits);
> +	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)
> +		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 +1368,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 +1431,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) {
> +	err = i801_acpi_probe(priv);
> +	if (err)
>  		return -ENODEV;
> -	}

I'd say that once this has been set, we need to call
acpi_remove_address_space_handler() in case of failure later (in the 2
returns after).

The rest looks OK to me.

Cheers,
Benjamin

>  
>  	err = pcim_iomap_regions(dev, 1 << SMBBAR,
>  				 dev_driver_string(&dev->dev));
> @@ -1441,6 +1532,7 @@ static void i801_remove(struct pci_dev *dev)
>  
>  	i801_del_mux(priv);
>  	i2c_del_adapter(&priv->adapter);
> +	i801_acpi_remove(priv);
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  
>  	platform_device_unregister(priv->tco_pdev);
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Jun 08, 2016 at 06:29:13PM +0200, Benjamin Tissoires wrote:
> > -	err = acpi_check_resource_conflict(&dev->resource[SMBBAR]);
> > -	if (err) {
> > +	err = i801_acpi_probe(priv);
> > +	if (err)
> >  		return -ENODEV;
> > -	}
> 
> I'd say that once this has been set, we need to call
> acpi_remove_address_space_handler() in case of failure later (in the 2
> returns after).

Indeed - I wonder how many mistakes one patch can contain :-(

Let me fix this and submit yet another version.

> The rest looks OK to me.

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

Hi Benjamin,

On Wed, 8 Jun 2016 18:29:13 +0200, Benjamin Tissoires wrote:
> >  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  {
> >  	unsigned char temp;
> > @@ -1277,6 +1368,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 +1431,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) {
> > +	err = i801_acpi_probe(priv);
> > +	if (err)
> >  		return -ENODEV;
> > -	}
> 
> I'd say that once this has been set, we need to call
> acpi_remove_address_space_handler() in case of failure later (in the 2
> returns after).

Good catch, sorry for missing it during my review.

The first error return can probably be left unchanged by calling
i801_acpi_probe() after it rather than before. The second will need a
call to i801_acpi_remove(priv) for sure.

On Monday 13 June 2016 11:19:46 Jean Delvare wrote:
> Hi Benjamin,
> 
> On Wed, 8 Jun 2016 18:29:13 +0200, Benjamin Tissoires wrote:
> > >  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > >  {
> > >  	unsigned char temp;
> > > @@ -1277,6 +1368,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 +1431,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) {
> > > +	err = i801_acpi_probe(priv);
> > > +	if (err)
> > >  		return -ENODEV;
> > > -	}
> > 
> > I'd say that once this has been set, we need to call
> > acpi_remove_address_space_handler() in case of failure later (in the 2
> > returns after).
> 
> Good catch, sorry for missing it during my review.
> 
> The first error return can probably be left unchanged by calling
> i801_acpi_probe() after it rather than before. The second will need a
> call to i801_acpi_remove(priv) for sure.

Maybe we should call acpi_remove_address_space_handler() after first
ACPI access to driver?

On Mon, Jun 13, 2016 at 11:45:00AM +0200, Pali Rohár wrote:
> On Monday 13 June 2016 11:19:46 Jean Delvare wrote:
> > Hi Benjamin,
> > 
> > On Wed, 8 Jun 2016 18:29:13 +0200, Benjamin Tissoires wrote:
> > > >  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > > >  {
> > > >  	unsigned char temp;
> > > > @@ -1277,6 +1368,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 +1431,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) {
> > > > +	err = i801_acpi_probe(priv);
> > > > +	if (err)
> > > >  		return -ENODEV;
> > > > -	}
> > > 
> > > I'd say that once this has been set, we need to call
> > > acpi_remove_address_space_handler() in case of failure later (in the 2
> > > returns after).
> > 
> > Good catch, sorry for missing it during my review.
> > 
> > The first error return can probably be left unchanged by calling
> > i801_acpi_probe() after it rather than before. The second will need a
> > call to i801_acpi_remove(priv) for sure.
> 
> Maybe we should call acpi_remove_address_space_handler() after first
> ACPI access to driver?

I fixed it already in v6 which got applied by Wolfram.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Monday 13 June 2016 12:46:18 Mika Westerberg wrote:
> On Mon, Jun 13, 2016 at 11:45:00AM +0200, Pali Rohár wrote:
> > On Monday 13 June 2016 11:19:46 Jean Delvare wrote:
> > > Hi Benjamin,
> > > 
> > > On Wed, 8 Jun 2016 18:29:13 +0200, Benjamin Tissoires wrote:
> > > > >  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > > > >  {
> > > > >  	unsigned char temp;
> > > > > @@ -1277,6 +1368,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 +1431,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) {
> > > > > +	err = i801_acpi_probe(priv);
> > > > > +	if (err)
> > > > >  		return -ENODEV;
> > > > > -	}
> > > > 
> > > > I'd say that once this has been set, we need to call
> > > > acpi_remove_address_space_handler() in case of failure later (in the 2
> > > > returns after).
> > > 
> > > Good catch, sorry for missing it during my review.
> > > 
> > > The first error return can probably be left unchanged by calling
> > > i801_acpi_probe() after it rather than before. The second will need a
> > > call to i801_acpi_remove(priv) for sure.
> > 
> > Maybe we should call acpi_remove_address_space_handler() after first
> > ACPI access to driver?
> 
> I fixed it already in v6 which got applied by Wolfram.

I mean to call acpi_remove_address_space_handler() in
i801_acpi_io_handler() after acpi_reserved is properly set.

As once acpi_reserved is set address space handler is not needed
anymore.

On Mon, Jun 13, 2016 at 11:48:30AM +0200, Pali Rohár wrote:
> I mean to call acpi_remove_address_space_handler() in
> i801_acpi_io_handler() after acpi_reserved is properly set.
> 
> As once acpi_reserved is set address space handler is not needed
> anymore.

It is still needed as we handle all AML OpRegion access in this driver
from that point forward. Unless I'm missing something.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Mika,

On Mon, 13 Jun 2016 12:54:04 +0300, Mika Westerberg wrote:
> On Mon, Jun 13, 2016 at 11:48:30AM +0200, Pali Rohár wrote:
> > I mean to call acpi_remove_address_space_handler() in
> > i801_acpi_io_handler() after acpi_reserved is properly set.
> > 
> > As once acpi_reserved is set address space handler is not needed
> > anymore.
> 
> It is still needed as we handle all AML OpRegion access in this driver
> from that point forward. Unless I'm missing something.

I think Pali is correct. The only purpose of handling the region is to
detect that it is being accessed so we can set priv->acpi_reserved.
Once it is set, i801_acpi_io_handler becomes transparent: it forwards
the requests without doing anything with them. The very same would
happen if we would unregister the handler at that point, but without the
extra overhead.

So while the current code does work fine, unregistering the handler
when we set priv->acpi_reserved would be more optimal.

Unless both Pali and myself are missing something, that is.

On Friday 24 June 2016 16:12:38 Jean Delvare wrote:
> Hi Mika,
> 
> On Mon, 13 Jun 2016 12:54:04 +0300, Mika Westerberg wrote:
> > On Mon, Jun 13, 2016 at 11:48:30AM +0200, Pali Rohár wrote:
> > > I mean to call acpi_remove_address_space_handler() in
> > > i801_acpi_io_handler() after acpi_reserved is properly set.
> > > 
> > > As once acpi_reserved is set address space handler is not needed
> > > anymore.
> > 
> > It is still needed as we handle all AML OpRegion access in this driver
> > from that point forward. Unless I'm missing something.
> 
> I think Pali is correct. The only purpose of handling the region is to
> detect that it is being accessed so we can set priv->acpi_reserved.
> Once it is set, i801_acpi_io_handler becomes transparent: it forwards
> the requests without doing anything with them. The very same would
> happen if we would unregister the handler at that point, but without the
> extra overhead.
> 
> So while the current code does work fine, unregistering the handler
> when we set priv->acpi_reserved would be more optimal.
> 
> Unless both Pali and myself are missing something, that is.

Yes, this is what I mean...

On Fri, Jun 24, 2016 at 04:12:38PM +0200, Jean Delvare wrote:
> Hi Mika,
> 
> On Mon, 13 Jun 2016 12:54:04 +0300, Mika Westerberg wrote:
> > On Mon, Jun 13, 2016 at 11:48:30AM +0200, Pali Rohár wrote:
> > > I mean to call acpi_remove_address_space_handler() in
> > > i801_acpi_io_handler() after acpi_reserved is properly set.
> > > 
> > > As once acpi_reserved is set address space handler is not needed
> > > anymore.
> > 
> > It is still needed as we handle all AML OpRegion access in this driver
> > from that point forward. Unless I'm missing something.
> 
> I think Pali is correct. The only purpose of handling the region is to
> detect that it is being accessed so we can set priv->acpi_reserved.
> Once it is set, i801_acpi_io_handler becomes transparent: it forwards
> the requests without doing anything with them. The very same would
> happen if we would unregister the handler at that point, but without the
> extra overhead.
> 
> So while the current code does work fine, unregistering the handler
> when we set priv->acpi_reserved would be more optimal.
> 
> Unless both Pali and myself are missing something, that is.

I'm not sure unregistering the handler actually resets back to the
default handler. Besides, this patch has been already merged for a while
so it requires a followup patch on top.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Mika,

On Wed, 29 Jun 2016 13:39:51 +0300, Mika Westerberg wrote:
> On Fri, Jun 24, 2016 at 04:12:38PM +0200, Jean Delvare wrote:
> > I think Pali is correct. The only purpose of handling the region is to
> > detect that it is being accessed so we can set priv->acpi_reserved.
> > Once it is set, i801_acpi_io_handler becomes transparent: it forwards
> > the requests without doing anything with them. The very same would
> > happen if we would unregister the handler at that point, but without the
> > extra overhead.
> > 
> > So while the current code does work fine, unregistering the handler
> > when we set priv->acpi_reserved would be more optimal.
> > 
> > Unless both Pali and myself are missing something, that is.
> 
> I'm not sure unregistering the handler actually resets back to the
> default handler.

I'm no ACPI expert. I read the code of
acpi_remove_address_space_handler() and a few other related ACPI
functions and can't claim I understood it all. But indeed it doesn't
look like it restores the original behavior. Probably
acpi_install_address_space_handler(..., ACPI_ADR_SPACE_SYSTEM_IO,
ACPI_DEFAULT_HANDLER, ...) should be used instead.

This raises another question though: if
acpi_remove_address_space_handler() doesn't restore the previous
behavior then we shouldn't be calling it when the driver is being
unloaded either. As I understand it, it breaks the ACPI handling of the
device.

However I can't test it, as the installed handler is never called
on my system. Can anyone test unloading the i2c-i801 driver on a system
where ACPI actually accesses the device?

After looking at the ACPI code, I am no longer convinced that restoring
the default handler would improve performance. The default handler
itself (acpi_ex_system_io_space_handler) has a lot of overhead. OTOH
this makes me wonder if it is really correct to call
acpi_os_read_port() and acpi_os_write_port() directly.
acpi_ex_system_io_space_handler() calls acpi_hw_read_port() and
acpi_hw_write_port() which perform additional checks. Actually it would
seem safer to call acpi_ex_system_io_space_handler() instead... if it
was exported. Oh well.

> Besides, this patch has been already merged for a while
> so it requires a followup patch on top.

Correct, whatever we do.

On Monday 04 July 2016 10:22:12 Jean Delvare wrote:
> However I can't test it, as the installed handler is never called
> on my system. Can anyone test unloading the i2c-i801 driver on a system
> where ACPI actually accesses the device?

I remember that HP EliteBooks with HDD accelerometer protection use
hp_accel ACPI driver which use that ACPI SBUS device. But I do not own
EliteBooks anymore... Maybe someone else can test? What I found probably
find are ACPI DSDT dumps from those machines, but do not know if that
can help...

On Mon, Jul 04, 2016 at 10:22:12AM +0200, Jean Delvare wrote:
> Hi Mika,
> 
> On Wed, 29 Jun 2016 13:39:51 +0300, Mika Westerberg wrote:
> > On Fri, Jun 24, 2016 at 04:12:38PM +0200, Jean Delvare wrote:
> > > I think Pali is correct. The only purpose of handling the region is to
> > > detect that it is being accessed so we can set priv->acpi_reserved.
> > > Once it is set, i801_acpi_io_handler becomes transparent: it forwards
> > > the requests without doing anything with them. The very same would
> > > happen if we would unregister the handler at that point, but without the
> > > extra overhead.
> > > 
> > > So while the current code does work fine, unregistering the handler
> > > when we set priv->acpi_reserved would be more optimal.
> > > 
> > > Unless both Pali and myself are missing something, that is.
> > 
> > I'm not sure unregistering the handler actually resets back to the
> > default handler.
> 
> I'm no ACPI expert. I read the code of
> acpi_remove_address_space_handler() and a few other related ACPI
> functions and can't claim I understood it all. But indeed it doesn't
> look like it restores the original behavior. Probably
> acpi_install_address_space_handler(..., ACPI_ADR_SPACE_SYSTEM_IO,
> ACPI_DEFAULT_HANDLER, ...) should be used instead.
> 
> This raises another question though: if
> acpi_remove_address_space_handler() doesn't restore the previous
> behavior then we shouldn't be calling it when the driver is being
> unloaded either. As I understand it, it breaks the ACPI handling of the
> device.
> 
> However I can't test it, as the installed handler is never called
> on my system. Can anyone test unloading the i2c-i801 driver on a system
> where ACPI actually accesses the device?

The whole point of this patch is that we expect that nobody never uses
that OpRegion. I'm 99% sure you don't find a single machine where it is
actually in use.

The fallback code is there just to be sure we do not blow things up if
it turns out some machine is using that.

I think the best we can do is to lock down the module (prevent it from
unloading) if we notice that the OpRegion is used.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday 05 July 2016 12:14:55 Mika Westerberg wrote:
> The whole point of this patch is that we expect that nobody never
> uses that OpRegion. I'm 99% sure you don't find a single machine
> where it is actually in use.

HP EliteBook 8460p uses it for sure! Here are DSDT snips:


                Method (\_SB.PCI0.LPCB.SMAB, 3, Serialized)
                {
                    If (LEqual (And (Arg0, 0x01), 0x00))
                    {
                        Store (0x01, Local0)
                        Store (\_SB.PCI0.SBUS.SWRB (Arg0, Arg1, Arg2), Local1)
                        If (Local1)
                        {
                            Store (0x00, Local0)
                        }
                    }
                    Else
                    {
                        Store (\_SB.PCI0.SBUS.SRDB (Arg0, Arg1), Local0)
                    }

                    Return (Local0)
                }

...

                Method (ALRD, 1, Serialized)
                {
                    Store (\_SB.PCI0.LPCB.SMAB (0x33, Arg0, 0x00), Local0)
                    Return (Local0)
                }

                Method (ALWR, 2, Serialized)
                {
                    Store (\_SB.PCI0.LPCB.SMAB (0x32, Arg0, Arg1), Local0)
                    Return (Local0)
                }


And ALRD and ALWR methods are used by hp_accel.ko kernel driver.

On Tue, Jul 05, 2016 at 01:30:23PM +0200, Pali Rohár wrote:
> On Tuesday 05 July 2016 12:14:55 Mika Westerberg wrote:
> > The whole point of this patch is that we expect that nobody never
> > uses that OpRegion. I'm 99% sure you don't find a single machine
> > where it is actually in use.
> 
> HP EliteBook 8460p uses it for sure! Here are DSDT snips:
> 
> 
>                 Method (\_SB.PCI0.LPCB.SMAB, 3, Serialized)
>                 {
>                     If (LEqual (And (Arg0, 0x01), 0x00))
>                     {
>                         Store (0x01, Local0)
>                         Store (\_SB.PCI0.SBUS.SWRB (Arg0, Arg1, Arg2), Local1)
>                         If (Local1)
>                         {
>                             Store (0x00, Local0)
>                         }
>                     }
>                     Else
>                     {
>                         Store (\_SB.PCI0.SBUS.SRDB (Arg0, Arg1), Local0)
>                     }
> 
>                     Return (Local0)
>                 }
> 

Crap, well that is in that 1% then ;-)

> ...
> 
>                 Method (ALRD, 1, Serialized)
>                 {
>                     Store (\_SB.PCI0.LPCB.SMAB (0x33, Arg0, 0x00), Local0)
>                     Return (Local0)
>                 }
> 
>                 Method (ALWR, 2, Serialized)
>                 {
>                     Store (\_SB.PCI0.LPCB.SMAB (0x32, Arg0, Arg1), Local0)
>                     Return (Local0)
>                 }
> 
> 
> And ALRD and ALWR methods are used by hp_accel.ko kernel driver.

So are you able to test what happens when you unload the driver? I think
the safest thing to do is that we just pin the driver in the kernel once
we notice the OpRegion is being accessed. Does anyone else have better
ideas?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday 05 July 2016 13:51:42 Mika Westerberg wrote:
> On Tue, Jul 05, 2016 at 01:30:23PM +0200, Pali Rohár wrote:
> > On Tuesday 05 July 2016 12:14:55 Mika Westerberg wrote:
> > > The whole point of this patch is that we expect that nobody never
> > > uses that OpRegion. I'm 99% sure you don't find a single machine
> > > where it is actually in use.
> > 
> > HP EliteBook 8460p uses it for sure! Here are DSDT snips:
> >                 Method (\_SB.PCI0.LPCB.SMAB, 3, Serialized)
> >                 {
> >                 
> >                     If (LEqual (And (Arg0, 0x01), 0x00))
> >                     {
> >                     
> >                         Store (0x01, Local0)
> >                         Store (\_SB.PCI0.SBUS.SWRB (Arg0, Arg1,
> >                         Arg2), Local1) If (Local1)
> >                         {
> >                         
> >                             Store (0x00, Local0)
> >                         
> >                         }
> >                     
> >                     }
> >                     Else
> >                     {
> >                     
> >                         Store (\_SB.PCI0.SBUS.SRDB (Arg0, Arg1),
> >                         Local0)
> >                     
> >                     }
> >                     
> >                     Return (Local0)
> >                 
> >                 }
> 
> Crap, well that is in that 1% then ;-)

I bet that every HP notebook with accelerometer which is used by 
hp_accel.ko driver is affected by this problem. And then it will be more 
then 1% :-)

> > ...
> > 
> >                 Method (ALRD, 1, Serialized)
> >                 {
> >                 
> >                     Store (\_SB.PCI0.LPCB.SMAB (0x33, Arg0, 0x00),
> >                     Local0) Return (Local0)
> >                 
> >                 }
> >                 
> >                 Method (ALWR, 2, Serialized)
> >                 {
> >                 
> >                     Store (\_SB.PCI0.LPCB.SMAB (0x32, Arg0, Arg1),
> >                     Local0) Return (Local0)
> >                 
> >                 }
> > 
> > And ALRD and ALWR methods are used by hp_accel.ko kernel driver.
> 
> So are you able to test what happens when you unload the driver?

As I wrote in previous email, I do not own these EliteBooks anymore, so 
cannot test it. Just have DSDT dump...

On Tuesday 05 July 2016 13:56:58 Pali Rohár wrote:
> On Tuesday 05 July 2016 13:51:42 Mika Westerberg wrote:
> > So are you able to test what happens when you unload the driver?
> 
> As I wrote in previous email, I do not own these EliteBooks anymore,
> so cannot test it. Just have DSDT dump...

What about contacting last contributors to hp_accel.c driver? They 
probably could test i801 changes if accelerometer still works.

On Tue, Jul 05, 2016 at 02:00:58PM +0200, Pali Rohár wrote:
> On Tuesday 05 July 2016 13:56:58 Pali Rohár wrote:
> > On Tuesday 05 July 2016 13:51:42 Mika Westerberg wrote:
> > > So are you able to test what happens when you unload the driver?
> > 
> > As I wrote in previous email, I do not own these EliteBooks anymore,
> > so cannot test it. Just have DSDT dump...
> 
> What about contacting last contributors to hp_accel.c driver? They 
> probably could test i801 changes if accelerometer still works.

Good idea.

Added Martin and Dominique who did the last additions to that driver
(hp_accel.c).

Do you guys still have your HP machines around? If yes, maybe you can
try v4.7-rc3+ (it should include commit a7ae81952cda ("i2c: i801: Allow
ACPI SystemIO OpRegion to conflict with PCI BAR")) so that you unload
i2c-i801.ko and see if accelerometer still works?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday 05 July 2016 14:00:58 Pali Rohár wrote:
> On Tuesday 05 July 2016 13:56:58 Pali Rohár wrote:
> > On Tuesday 05 July 2016 13:51:42 Mika Westerberg wrote:
> > > So are you able to test what happens when you unload the driver?
> > 
> > As I wrote in previous email, I do not own these EliteBooks anymore,
> > so cannot test it. Just have DSDT dump...
> 
> What about contacting last contributors to hp_accel.c driver? They 
> probably could test i801 changes if accelerometer still works.

Or another option how to test this: Use acpi_call module which exports
file /proc/acpi/call which can be used to issue ACPI method call from
userspace.

If there are problems, there will be errors in dmesg or return value
from /proc/acpi/call is some error...

On Thursday, July 14, 2016 01:52:18 PM Pali Rohár wrote:
> On Tuesday 05 July 2016 14:00:58 Pali Rohár wrote:
> > On Tuesday 05 July 2016 13:56:58 Pali Rohár wrote:
> > > On Tuesday 05 July 2016 13:51:42 Mika Westerberg wrote:
> > > > So are you able to test what happens when you unload the driver?
> > > 
> > > As I wrote in previous email, I do not own these EliteBooks anymore,
> > > so cannot test it. Just have DSDT dump...
> > 
> > What about contacting last contributors to hp_accel.c driver? They 
> > probably could test i801 changes if accelerometer still works.
> 
> Or another option how to test this: Use acpi_call module which exports
> file /proc/acpi/call which can be used to issue ACPI method call from
> userspace.
> 
> If there are problems, there will be errors in dmesg or return value
> from /proc/acpi/call is some error...

That is plain dangerous, because some objects may not be evaulated without
preparation.

We had something like that before and dropped it.

We have an AML debugger in the kernel now, though, which in principle may
be used for such diagnostics I think.

Thanks,
Rafael

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

2016-07-05 16:31 GMT+02:00 Mika Westerberg <mika.westerberg@linux.intel.com>:
> On Tue, Jul 05, 2016 at 02:00:58PM +0200, Pali Rohár wrote:
>> On Tuesday 05 July 2016 13:56:58 Pali Rohár wrote:
>> > On Tuesday 05 July 2016 13:51:42 Mika Westerberg wrote:
>> > > So are you able to test what happens when you unload the driver?
>> >
>> > As I wrote in previous email, I do not own these EliteBooks anymore,
>> > so cannot test it. Just have DSDT dump...
>>
>> What about contacting last contributors to hp_accel.c driver? They
>> probably could test i801 changes if accelerometer still works.
>
> Good idea.
>
> Added Martin and Dominique who did the last additions to that driver
> (hp_accel.c).
>
> Do you guys still have your HP machines around? If yes, maybe you can
> try v4.7-rc3+ (it should include commit a7ae81952cda ("i2c: i801: Allow
> ACPI SystemIO OpRegion to conflict with PCI BAR")) so that you unload
> i2c-i801.ko and see if accelerometer still works?

I still have the HP ProBook 440 G3. I used v4.7-rc7 for the test. I
tested all combinations of loading/unloading both hp_accel and
i2c_i801 modules and whenever the hp_accel was loaded the
accelerometer worked fine.

Regards,
-Martin Vajnar
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sunday 24 July 2016 12:08:25 Martin Vajnar wrote:
> 2016-07-05 16:31 GMT+02:00 Mika Westerberg <mika.westerberg@linux.intel.com>:
> > On Tue, Jul 05, 2016 at 02:00:58PM +0200, Pali Rohár wrote:
> >> On Tuesday 05 July 2016 13:56:58 Pali Rohár wrote:
> >> > On Tuesday 05 July 2016 13:51:42 Mika Westerberg wrote:
> >> > > So are you able to test what happens when you unload the driver?
> >> >
> >> > As I wrote in previous email, I do not own these EliteBooks anymore,
> >> > so cannot test it. Just have DSDT dump...
> >>
> >> What about contacting last contributors to hp_accel.c driver? They
> >> probably could test i801 changes if accelerometer still works.
> >
> > Good idea.
> >
> > Added Martin and Dominique who did the last additions to that driver
> > (hp_accel.c).
> >
> > Do you guys still have your HP machines around? If yes, maybe you can
> > try v4.7-rc3+ (it should include commit a7ae81952cda ("i2c: i801: Allow
> > ACPI SystemIO OpRegion to conflict with PCI BAR")) so that you unload
> > i2c-i801.ko and see if accelerometer still works?
> 
> I still have the HP ProBook 440 G3. I used v4.7-rc7 for the test. I
> tested all combinations of loading/unloading both hp_accel and
> i2c_i801 modules and whenever the hp_accel was loaded the
> accelerometer worked fine.
> 
> Regards,
> -Martin Vajnar

Great! Thank you for testing.

On Monday 04 July 2016 10:22:12 Jean Delvare wrote:
> Hi Mika,
> 
> On Wed, 29 Jun 2016 13:39:51 +0300, Mika Westerberg wrote:
> > On Fri, Jun 24, 2016 at 04:12:38PM +0200, Jean Delvare wrote:
> > > I think Pali is correct. The only purpose of handling the region is to
> > > detect that it is being accessed so we can set priv->acpi_reserved.
> > > Once it is set, i801_acpi_io_handler becomes transparent: it forwards
> > > the requests without doing anything with them. The very same would
> > > happen if we would unregister the handler at that point, but without the
> > > extra overhead.
> > > 
> > > So while the current code does work fine, unregistering the handler
> > > when we set priv->acpi_reserved would be more optimal.
> > > 
> > > Unless both Pali and myself are missing something, that is.
> > 
> > I'm not sure unregistering the handler actually resets back to the
> > default handler.
> 
> I'm no ACPI expert. I read the code of
> acpi_remove_address_space_handler() and a few other related ACPI
> functions and can't claim I understood it all. But indeed it doesn't
> look like it restores the original behavior. Probably
> acpi_install_address_space_handler(..., ACPI_ADR_SPACE_SYSTEM_IO,
> ACPI_DEFAULT_HANDLER, ...) should be used instead.
> 
> This raises another question though: if
> acpi_remove_address_space_handler() doesn't restore the previous
> behavior then we shouldn't be calling it when the driver is being
> unloaded either. As I understand it, it breaks the ACPI handling of the
> device.
> 
> However I can't test it, as the installed handler is never called
> on my system. Can anyone test unloading the i2c-i801 driver on a system
> where ACPI actually accesses the device?
> 
> After looking at the ACPI code, I am no longer convinced that restoring
> the default handler would improve performance. The default handler
> itself (acpi_ex_system_io_space_handler) has a lot of overhead. OTOH
> this makes me wonder if it is really correct to call
> acpi_os_read_port() and acpi_os_write_port() directly.
> acpi_ex_system_io_space_handler() calls acpi_hw_read_port() and
> acpi_hw_write_port() which perform additional checks. Actually it would
> seem safer to call acpi_ex_system_io_space_handler() instead... if it
> was exported. Oh well.
> 
> > Besides, this patch has been already merged for a while
> > so it requires a followup patch on top.
> 
> Correct, whatever we do.
> 

Now Martin Vajnar confirmed that accelerometer on his notebook working
fine with that patch. But it does not mean that we should not fix
address space handler code in i801 correctly...

Can some ACPI expert look at it? Jean already wrote some useful
informations.