[PATCH] i2c: i801: Fix SMBus ENXIO on resume from suspend.

[PATCH] i2c: i801: Fix SMBus ENXIO on resume from suspend.

[Alban Browaeys]

Disabling I2C_EN on resume as it was restored in
suspend when we set pci config to its original value.

ENXIO landing in rmi_smb_get_version,  i2c-i802 debug
pointing at i801_check_post and guesswork from reading
probe which has this tweak.

i801_smbus 0000:00:1f.3: No response
rmi4_smbus 0-002c: failed to get SMBus version number!
i801_smbus 0000:00:1f.3: No response
rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed to read current IRQ mask.
i801_smbus 0000:00:1f.3: No response
rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6.
rmi4_f01 rmi4-00.fn01: Resume failed with code -6.
rmi4_physical rmi4-00: Failed to suspend functions: -6
rmi4_smbus 0-002c: Failed to resume device: -6
rmi4_f03 rmi4-00.fn03: rmi_f03_pt_write: Failed to write to F03 TX register (-6).
i801_smbus 0000:00:1f.3: No response
rmi4_f03 rmi4-00.fn03: rmi_f03_pt_write: Failed to write to F03 TX register (-6).

Signed-off-by: Alban Browaeys <alban.browaeys@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 9e12a53ef7b8..b7b6c8ee2176 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1707,9 +1707,14 @@ static int i801_suspend(struct device *dev)
 
 static int i801_resume(struct device *dev)
 {
+	unsigned char temp;
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct i801_priv *priv = pci_get_drvdata(pci_dev);
 
+	pci_read_config_byte(pci_dev, SMBHSTCFG, &temp);
+	temp &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
+	pci_write_config_byte(pci_dev, SMBHSTCFG, temp);
+
 	i801_enable_host_notify(&priv->adapter);
 
 	return 0;
-- 
2.15.0

Re: [PATCH] i2c: i801: Fix SMBus ENXIO on resume from suspend.

[Alban Browaeys]

My apologies.

This fix is a partial one: it only fixes the issue once
we unload then reload i2c-i801 .
This as when we load a second time  orig_hstcfg is set to the value the
first module load leftover.
Thus I will investigate the initial orig_hstcfg at boot to further the
fix.
I bet this only a few missing bits like HST_EN.

Best regards
Alban


Le mercredi 15 novembre 2017 à 18:01 +0100, Alban Browaeys a écrit :
> Disabling I2C_EN on resume as it was restored in
> suspend when we set pci config to its original value.
> 
> ENXIO landing in rmi_smb_get_version,  i2c-i802 debug
> pointing at i801_check_post and guesswork from reading
> probe which has this tweak.
> 
> i801_smbus 0000:00:1f.3: No response
> rmi4_smbus 0-002c: failed to get SMBus version number!
> i801_smbus 0000:00:1f.3: No response
> rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed to read
> current IRQ mask.
> i801_smbus 0000:00:1f.3: No response
> rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6.
> rmi4_f01 rmi4-00.fn01: Resume failed with code -6.
> rmi4_physical rmi4-00: Failed to suspend functions: -6
> rmi4_smbus 0-002c: Failed to resume device: -6
> rmi4_f03 rmi4-00.fn03: rmi_f03_pt_write: Failed to write to F03 TX
> register (-6).
> i801_smbus 0000:00:1f.3: No response
> rmi4_f03 rmi4-00.fn03: rmi_f03_pt_write: Failed to write to F03 TX
> register (-6).
> 
> Signed-off-by: Alban Browaeys <alban.browaeys@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-
> i801.c
> index 9e12a53ef7b8..b7b6c8ee2176 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1707,9 +1707,14 @@ static int i801_suspend(struct device *dev)
>  
>  static int i801_resume(struct device *dev)
>  {
> +	unsigned char temp;
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	struct i801_priv *priv = pci_get_drvdata(pci_dev);
>  
> +	pci_read_config_byte(pci_dev, SMBHSTCFG, &temp);
> +	temp &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
> +	pci_write_config_byte(pci_dev, SMBHSTCFG, temp);
> +
>  	i801_enable_host_notify(&priv->adapter);
>  
>  	return 0;

Re: [PATCH] i2c: i801: Fix SMBus ENXIO on resume from suspend.

[Jean Delvare]

On Wed, 15 Nov 2017 21:25:25 +0100, Alban Browaeys wrote:
> This fix is a partial one: it only fixes the issue once
> we unload then reload i2c-i801 .
> This as when we load a second time  orig_hstcfg is set to the value the
> first module load leftover.
> Thus I will investigate the initial orig_hstcfg at boot to further the
> fix.
> I bet this only a few missing bits like HST_EN.

Volker, I think you had a fix for that one? But I can't find it in the
list archive :-(

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Fix SMBus ENXIO on resume from suspend.

[Alban Browaeys]

I am on one of those Intel Haswell with the ACPI OpRegion conflict, the
acpi handler was added to cope with.

Could it be the embedded controller code mess with the smbus ?
When I print the OpRegion fields via acpi-call I get different values
after suspend/resume from S3 than at runtime at various points.

I already checked that the acpi SBUS methods are not called.
To do so I define a global name set to zero and set it to 1 in
SBUS.STRT (called by all SBUS method to verify the ready state).

Could it be the EC access the region directly ?
The acpi handler from i2c-i801 does not trigger. Is it in effect when
suspending or resuming ?

The reproducer seems hard to get right, I am still investigating.
It involves resume from suspend (S3) and a yet to define state.
Most of the time after an acpi reset (magic sysrq 'b').
And / or early load of i2x-i801, rmi_smbus and psmouse.

To call the acpi methods I echo to /proc/acpi/call from the acpi-call
module. 

I tried https://github.com/pali/i2c-acpi-sbus .
It fails early as acpi sbus methods check the inuse host status flag.
It is always on thus they bail out. i2c-i801 does not test inuse
(0x40).


Best regards
Alban


Le lundi 20 novembre 2017 à 10:15 +0100, Jean Delvare a écrit :
> On Wed, 15 Nov 2017 21:25:25 +0100, Alban Browaeys wrote:
> > This fix is a partial one: it only fixes the issue once
> > we unload then reload i2c-i801 .
> > This as when we load a second time  orig_hstcfg is set to the value
> > the
> > first module load leftover.
> > Thus I will investigate the initial orig_hstcfg at boot to further
> > the
> > fix.
> > I bet this only a few missing bits like HST_EN.
> 
> Volker, I think you had a fix for that one? But I can't find it in
> the
> list archive :-(
> 

Re: [PATCH] i2c: i801: Fix SMBus ENXIO on resume from suspend.

[Jean Delvare]

On Tue, 21 Nov 2017 09:47:41 +0100, Volker Rümelin wrote:
> > Volker, I think you had a fix for that one? But I can't find it in the
> > list archive :-(
> Yes, on 13 Jun 2017 I sent a patch to the linux-i2c mailing list. But
> the mailing list server told me: Your address is not liked source for
> email. At this point I gave up.

I see, that's why it's in my mailbox but missing from the archive.
 
> If you are still interested I can try to resend the patch with my
> google email account.

I'll review it, so it will become visible :-)

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Fix SMBus ENXIO on resume from suspend.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]


> > > Volker, I think you had a fix for that one? But I can't find it in the
> > > list archive :-(
> > Yes, on 13 Jun 2017 I sent a patch to the linux-i2c mailing list. But
> > the mailing list server told me: Your address is not liked source for
> > email. At this point I gave up.
> 
> I see, that's why it's in my mailbox but missing from the archive.
>  
> > If you are still interested I can try to resend the patch with my
> > google email account.
> 
> I'll review it, so it will become visible :-)

So, Volker's patch showed up on the ML meanwhile, nice :)

I don't get if we need this patch as well or if this gets covered with
Volker's patch as well?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: i801: Fix SMBus ENXIO on resume from suspend.

[Alban Browaeys]

Thanks for Volker's patch.
I applied it and it does not help on this Intel Haswell box (Lenovo
Thinkpad Yoga S1). My initial patch did not helped either.

I was confused as the bug reproduce easely only if the i2c-i801,
rmi_smbus and psmouse are loaded once then resume from suspend.
Afterwards (even unload module) I sometimes get around the bug.

Could ACPI/EC OpRegion accesses happen without the kernel handler
triggering (while suspending and / or resuming) ?

Thanks
Alban



Le lundi 20 novembre 2017 à 15:18 +0100, Alban Browaeys a écrit :
> I am on one of those Intel Haswell with the ACPI OpRegion conflict,
> the
> acpi handler was added to cope with.
> 
> Could it be the embedded controller code mess with the smbus ?
> When I print the OpRegion fields via acpi-call I get different values
> after suspend/resume from S3 than at runtime at various points.
> 
> I already checked that the acpi SBUS methods are not called.
> To do so I define a global name set to zero and set it to 1 in
> SBUS.STRT (called by all SBUS method to verify the ready state).
> 
> Could it be the EC access the region directly ?
> The acpi handler from i2c-i801 does not trigger. Is it in effect when
> suspending or resuming ?
> 
> The reproducer seems hard to get right, I am still investigating.
> It involves resume from suspend (S3) and a yet to define state.
> Most of the time after an acpi reset (magic sysrq 'b').
> And / or early load of i2x-i801, rmi_smbus and psmouse.
> 
> To call the acpi methods I echo to /proc/acpi/call from the acpi-call
> module. 
> 
> I tried https://github.com/pali/i2c-acpi-sbus .
> It fails early as acpi sbus methods check the inuse host status flag.
> It is always on thus they bail out. i2c-i801 does not test inuse
> (0x40).
> 
> 
> Best regards
> Alban
> 
> 
> Le lundi 20 novembre 2017 à 10:15 +0100, Jean Delvare a écrit :
> > On Wed, 15 Nov 2017 21:25:25 +0100, Alban Browaeys wrote:
> > > This fix is a partial one: it only fixes the issue once
> > > we unload then reload i2c-i801 .
> > > This as when we load a second time  orig_hstcfg is set to the
> > > value
> > > the
> > > first module load leftover.
> > > Thus I will investigate the initial orig_hstcfg at boot to
> > > further
> > > the
> > > fix.
> > > I bet this only a few missing bits like HST_EN.
> > 
> > Volker, I think you had a fix for that one? But I can't find it in
> > the
> > list archive :-(

Re: [PATCH] i2c: i801: Fix SMBus ENXIO on resume from suspend.

[Alban Browaeys]

Le jeudi 07 décembre 2017 à 11:57 +0100, Wolfram Sang a écrit :
> > > > Volker, I think you had a fix for that one? But I can't find it
> > > > in the
> > > > list archive :-(
> > > 
> > > Yes, on 13 Jun 2017 I sent a patch to the linux-i2c mailing list.
> > > But
> > > the mailing list server told me: Your address is not liked source
> > > for
> > > email. At this point I gave up.
> > 
> > I see, that's why it's in my mailbox but missing from the archive.
> >  
> > > If you are still interested I can try to resend the patch with my
> > > google email account.
> > 
> > I'll review it, so it will become visible :-)
> 
> So, Volker's patch showed up on the ML meanwhile, nice :)
> 
> I don't get if we need this patch as well or if this gets covered
> with
> Volker's patch as well?

Mine is a rough duplicate of Volker.

But it turns out none of those take care of the ENXIO issue. I thought
otherwise at first but this bug is not easely reproducible except on
cold boot.
I am at loss as to how to fix my ENXIO issue.

Thanks,
Alban