Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1710229 > unrolled thread

Re: [PATCH] i2c: i801: Allow ACPI SystemIO OpRegion to conflict harder

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] i2c: i801: Allow ACPI SystemIO OpRegion to conflict  harder Wolfram Sang <wsa@the-dreams.de> - 2017-08-12 13:20 +0200
    Re: [PATCH] i2c: i801: Allow ACPI SystemIO OpRegion to conflict  harder Lyude Paul <lyude@redhat.com> - 2017-08-14 18:10 +0200
      Re: [PATCH] i2c: i801: Allow ACPI SystemIO OpRegion to conflict  harder Wolfram Sang <wsa@the-dreams.de> - 2017-08-17 21:50 +0200

#1710229 — Re: [PATCH] i2c: i801: Allow ACPI SystemIO OpRegion to conflict harder

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jun 26, 2017 at 04:40:08PM -0400, Lyude wrote:
> There's quite a number of machines on the market, mainly Lenovo
> ThinkPads, that make the horrible mistake in their firmware of reusing
> the PCIBAR space reserved for the SMBus for things that are completely
> unrelated to the SMBus controller, such as the OpRegion used for i915.
> This is very bad and entirely evil, but with Lenovo's historically poor
> track record of fixing their firmware, it is extremely unlikely this is
> ever going to be properly fixed.
> 
> So, while it would be nice if we could just cut off the SMBus controller
> and call it a day this unfortunately breaks RMI4 mode completely for
> most of the ThinkPads. Even though this behavior is extremely wrong, for
> whatever reason sharing the PCIBAR between the OpRegion and SMBus seems
> to be just fine. Regardless however, I think it's safe to assume that
> when the BIOS accesses the PCIBAR space of the SMBus controller like
> this that it's trying to get to something else that we mapped the SMBus
> controller over.
> 
> On my X1 Carbon, this assumption appears to be correct. I've traced down
> the firmware accesses to being caused by the firmware mistakenly placing
> the SWSCI mailbox for i915 on top of the SMBus host controller region.
> And indeed, blacklisting i915 causes the firmware to never attempt to
> touch this region.
> 
> So, in order to try to workaround this and not break either SMBus or
> i915, we temporarily unmap the PCI device for the SMBus controller,
> do the thing that the firmware wanted to do, then remap the device and
> report a firmware bug.
> 
> Signed-off-by: Lyude <lyude@redhat.com>

No full name? Or is it your full name?

> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Benjamin Tissoires <btissoir@redhat.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: intel-gfx@lists.freedesktop.org

I don't know this matter at all. I'd need comments from these people on
CC to proceed with this one.

> ---
> So: unfortunately
> 
> a7ae81952cda (i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR)
> 
> Seems to prevent the ThinkPad X1 Carbon 4th gen and the T460s from actually
> using their SMBus controllers at all. As mentioned above, I've traced the issue
> down to the firmware responding to the SWSCI by sticking data in places it
> shouldn't, e.g. the SMBus registers.
> 
> I'm entirely unsure if this patch is the correct fix for this, and wouldn't be
> at all surprised if it's just as bad of a patch as I think it is ;P. So I
> figured I'd send it to intel-gfx and the authors of the original version of this
> patch to get their take on it and see if there might be something less hacky we
> can do to fix this.
> 
>  drivers/i2c/busses/i2c-i801.c | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 6484fa6..bfbe0f9 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1406,33 +1406,42 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>  {
>  	struct i801_priv *priv = handler_context;
>  	struct pci_dev *pdev = priv->pci_dev;
> +	struct device *dev = &pdev->dev;
>  	acpi_status status;
> +	int err;
>  
> -	/*
> -	 * 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;
> +	/*
> +	 * BIOS AML code should never actually touch the SMBus registers,
> +	 * however crappy firmware (mainly Lenovo's) can make the mistake of
> +	 * mapping things over the SMBus region that should definitely not be
> +	 * there (such as the OpRegion for Intel GPUs).
> +	 * This is extremely bad firmware behavior, but it is unlikely this will
> +	 * ever get fixed by Lenovo.
> +	 */
> +	dev_warn_once(dev,
> +		      FW_BUG "OpRegion overlaps with SMBus registers, working around\n");
>  
> -		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);
> -	}
> +	pm_runtime_get_sync(dev);
> +	pcim_iounmap_regions(pdev, 1 << SMBBAR);
>  
>  	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);
>  
> +	err = pcim_iomap_regions(pdev, 1 << SMBBAR,
> +				 dev_driver_string(&pdev->dev));
> +	if (err) {
> +		dev_err(&pdev->dev,
> +			FW_BUG "Failed to restore SMBus region 0x%lx-0x%Lx. SMBus is now broken.\n",
> +			priv->smba,
> +			(unsigned long long)pci_resource_end(pdev, SMBBAR));
> +		priv->acpi_reserved = true;
> +	}
> +
> +	pm_runtime_put(dev);
>  	mutex_unlock(&priv->acpi_lock);
>  
>  	return status;
> -- 
> 2.9.4
> 

[toc] | [next] | [standalone]

#1711119

On Sat, 2017-08-12 at 13:15 +0200, Wolfram Sang wrote:
> On Mon, Jun 26, 2017 at 04:40:08PM -0400, Lyude wrote:
> > There's quite a number of machines on the market, mainly Lenovo
> > ThinkPads, that make the horrible mistake in their firmware of
> > reusing
> > the PCIBAR space reserved for the SMBus for things that are
> > completely
> > unrelated to the SMBus controller, such as the OpRegion used for
> > i915.
> > This is very bad and entirely evil, but with Lenovo's historically
> > poor
> > track record of fixing their firmware, it is extremely unlikely
> > this is
> > ever going to be properly fixed.
> > 
> > So, while it would be nice if we could just cut off the SMBus
> > controller
> > and call it a day this unfortunately breaks RMI4 mode completely
> > for
> > most of the ThinkPads. Even though this behavior is extremely
> > wrong, for
> > whatever reason sharing the PCIBAR between the OpRegion and SMBus
> > seems
> > to be just fine. Regardless however, I think it's safe to assume
> > that
> > when the BIOS accesses the PCIBAR space of the SMBus controller
> > like
> > this that it's trying to get to something else that we mapped the
> > SMBus
> > controller over.
> > 
> > On my X1 Carbon, this assumption appears to be correct. I've traced
> > down
> > the firmware accesses to being caused by the firmware mistakenly
> > placing
> > the SWSCI mailbox for i915 on top of the SMBus host controller
> > region.
> > And indeed, blacklisting i915 causes the firmware to never attempt
> > to
> > touch this region.
> > 
> > So, in order to try to workaround this and not break either SMBus
> > or
> > i915, we temporarily unmap the PCI device for the SMBus controller,
> > do the thing that the firmware wanted to do, then remap the device
> > and
> > report a firmware bug.
> > 
> > Signed-off-by: Lyude <lyude@redhat.com>
> 
> No full name? Or is it your full name?
Looks like I forgot to change my desktop's S-B identity to full name,
but it shouldn't be a big deal. I've got tons of other patches already
upstream like this.
> 
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Benjamin Tissoires <btissoir@redhat.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Jean Delvare <jdelvare@suse.de>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: intel-gfx@lists.freedesktop.org
> 
> I don't know this matter at all. I'd need comments from these people
> on
> CC to proceed with this one.
> 
> > ---
> > So: unfortunately
> > 
> > a7ae81952cda (i2c: i801: Allow ACPI SystemIO OpRegion to conflict
> > with PCI BAR)
> > 
> > Seems to prevent the ThinkPad X1 Carbon 4th gen and the T460s from
> > actually
> > using their SMBus controllers at all. As mentioned above, I've
> > traced the issue
> > down to the firmware responding to the SWSCI by sticking data in
> > places it
> > shouldn't, e.g. the SMBus registers.
> > 
> > I'm entirely unsure if this patch is the correct fix for this, and
> > wouldn't be
> > at all surprised if it's just as bad of a patch as I think it is
> > ;P. So I
> > figured I'd send it to intel-gfx and the authors of the original
> > version of this
> > patch to get their take on it and see if there might be something
> > less hacky we
> > can do to fix this.
> > 
> >  drivers/i2c/busses/i2c-i801.c | 41 +++++++++++++++++++++++++----
> > ------------
> >  1 file changed, 25 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c
> > b/drivers/i2c/busses/i2c-i801.c
> > index 6484fa6..bfbe0f9 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1406,33 +1406,42 @@ i801_acpi_io_handler(u32 function,
> > acpi_physical_address address, u32 bits,
> >  {
> >  	struct i801_priv *priv = handler_context;
> >  	struct pci_dev *pdev = priv->pci_dev;
> > +	struct device *dev = &pdev->dev;
> >  	acpi_status status;
> > +	int err;
> >  
> > -	/*
> > -	 * 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;
> > +	/*
> > +	 * BIOS AML code should never actually touch the SMBus
> > registers,
> > +	 * however crappy firmware (mainly Lenovo's) can make the
> > mistake of
> > +	 * mapping things over the SMBus region that should
> > definitely not be
> > +	 * there (such as the OpRegion for Intel GPUs).
> > +	 * This is extremely bad firmware behavior, but it is
> > unlikely this will
> > +	 * ever get fixed by Lenovo.
> > +	 */
> > +	dev_warn_once(dev,
> > +		      FW_BUG "OpRegion overlaps with SMBus
> > registers, working around\n");
> >  
> > -		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);
> > -	}
> > +	pm_runtime_get_sync(dev);
> > +	pcim_iounmap_regions(pdev, 1 << SMBBAR);
> >  
> >  	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);
> >  
> > +	err = pcim_iomap_regions(pdev, 1 << SMBBAR,
> > +				 dev_driver_string(&pdev->dev));
> > +	if (err) {
> > +		dev_err(&pdev->dev,
> > +			FW_BUG "Failed to restore SMBus region
> > 0x%lx-0x%Lx. SMBus is now broken.\n",
> > +			priv->smba,
> > +			(unsigned long long)pci_resource_end(pdev,
> > SMBBAR));
> > +		priv->acpi_reserved = true;
> > +	}
> > +
> > +	pm_runtime_put(dev);
> >  	mutex_unlock(&priv->acpi_lock);
> >  
> >  	return status;
> > -- 
> > 2.9.4
> > 

[toc] | [prev] | [next] | [standalone]

#1714346

[Multipart message — attachments visible in raw view] — view raw

> > > Signed-off-by: Lyude <lyude@redhat.com>
> > 
> > No full name? Or is it your full name?
> Looks like I forgot to change my desktop's S-B identity to full name,
> but it shouldn't be a big deal. I've got tons of other patches already
> upstream like this.

I'd much prefer a full name. But more importantly, what I definately
need is some review/ack/whatever because I don't know enough about this
stuff (other than knowing it is a fragile area).

Maybe you could resend with full name and the acpi list on CC?

Regards,

   Wolfram

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web