Re: [Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 6/28/24 09:48, Dieter Mummenschanz wrote:
> Hi,
> any chance we can upstream the patch before 6.10 goes final? At least it 
> would fix suspend on older devices (I219-V [8086:15bc] (rev 10)).
> Kind Regards,
> Dieter

would be great, do you want to add your Tested-by?

> 
> On 6/21/2024 10:55 AM, Brandt, Todd E wrote:
>  > I just built and tested your patch on the latest 6.10.0-rc3 tip. It 
> seems to have fixed the issue on three of our machines, but the issue 
> still occurs on our Meteor Lake SDV board (otcpl-mtl-s).

My understanding was that the very first regression was for Meteor Lake,
with subsequent fix (bfd546a552e1) and yet another now (this thread),
with both of the fixes targeted to help on prior platforms. According to
your comment, this patch seems to help with "three of our machines", I 
read this as those are the "older devices" that Vitaly refers to.

So just from your comment I would say that this patch reduces the scope
of affected platforms to a subset of what it was prior to the very first
fix (bfd546a552e1), is that correct?
if so, you could provide your Tested-by tag.

>  >
>  > [ 130.302511] e1000e: EEE TX LPI TIMER: 00000011
>  > [ 130.390014] e1000e 0000:80:1f.6: PM: pci_pm_suspend(): 
> e1000e_pm_suspend [e1000e] returns -2
>  > [ 130.390033] e1000e 0000:80:1f.6: PM: dpm_run_callback(): 
> pci_pm_suspend returns -2
>  > [ 130.390039] e1000e 0000:80:1f.6: PM: failed to suspend async: error -2
>  > [ 130.574807] PM: suspend of devices aborted after 293.955 msecs
>  > [ 130.574817] PM: start suspend of devices aborted after 376.596 msecs
>  > [ 130.574820] PM: Some devices failed to suspend, or early wake event 
> detected
>  >
>  > $> lspci -nn -s 80:1f.6
>  > 80:1f.6 Ethernet controller [0200]: Intel Corporation Device [8086:550d]
> 
> I see that the bus of your device is 80 and not 0 as usual, do you use
> virtualization features? If so, can you please disable them and retest?
> 

Would be good to know more to aid future debug, or perhaps it should be
a blocker here? (IOW what's the scope)

>  >
>  > -----Original Message-----
>  > From: Lifshits, Vitaly <vitaly.lifshits@intel.com>
>  > Sent: Wednesday, June 19, 2024 11:37 PM
>  > To: intel-wired-lan@osuosl.org
>  > Cc: hui.wang@canonical.com; Lifshits, Vitaly 
> <vitaly.lifshits@intel.com>; Brandt, Todd E <todd.e.brandt@intel.com>; 
> Dieter Mummenschanz <dmummenschanz@web.de>
>  > Subject: [PATCH iwl-net v2 1/1] e1000e: fix force smbus during 
> suspend flow
>  >
>  > Commit 861e8086029e ("e1000e: move force SMBUS from enable ulp 
> function to avoid PHY loss issue") resolved a PHY access loss during 
> suspend on Meteor Lake consumer platforms, but it affected corporate 
> systems incorrectly.
>  >
>  > A better fix, working for both consumer and corporate systems, was 
> proposed in commit bfd546a552e1 ("e1000e: move force SMBUS near the end 
> of enable_ulp function"). However, it introduced a regression on older 
> devices, such as [8086:15B8], [8086:15F9], [8086:15BE].

Paul was right that such lists are best provided in sorted form, but
for free-form text with just three items, does not matter that much.

>  >
>  > This patch aims to fix the secondary regression, by limiting the 
> scope of the changes to Meteor Lake platforms only.
>  >
>  > Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of 
> enable_ulp function")
>  > Reported-by: Todd Brandt <todd.e.brandt@intel.com>
>  > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940 
> <https://bugzilla.kernel.org/show_bug.cgi?id=218940>
>  > Reported-by: Dieter Mummenschanz <dmummenschanz@web.de>
>  > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936 
> <https://bugzilla.kernel.org/show_bug.cgi?id=218936>
>  > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>