Re: [External]Re:Re: [PATCH] [v2,1/1] Fix WWAN device disabled issue after S3 deep

[Mark Pearson]

On 2021-08-12 7:35 a.m., Slark Xiao wrote:
> At 2021-08-12 16:03:50, "Hans de Goede" <hdegoede@xxxxxxxxxx> wrote:
> > Hi,
> >
> > On 8/11/21 11:34 AM, Slark Xiao wrote:
> > > When WWAN device wake from S3 deep, under thinkpad platform,
> > > WWAN would be disabled. This disable status could be checked
> > >   by command 'nmcli r wwan' or 'rfkill list'.
> > > Issue analysis as below:
> > >    When host resume from S3 deep, thinkpad_acpi driver would
> > > call hotkey_resume() function. Finnaly, it will use
> > > wan_get_status to check the current status of WWAN device.
> > > During this resume progress, wan_get_status would always
> > > return off even WWAN boot up completely.
> > >    If wan_get_status() return off, rfkill_set_sw_state() would set WWAN's
> > > status as disabled.
> > >    This may be a fault of LENOVO BIOS.
> > >    Workaround is add a WWAN device check before rfkill_set_sw_state().
> > > If it's a Foxconn WWAN device, then we will ignore to do a status update.
> > >
> > > Signed-off-by: Slark Xiao <slark_xiao@xxxxxxx>
> >
> > Thank you for debugging this and thank you for the patch.
> >
> > I'm not in favor of using a pci-device-id list here. Maybe we should
> > simply just never update the sw-rfkill state after a suspend-resume ?
> >
> > I mean the sw_state should be unchanged after a suspend/resume.
> >
> > Only the hw_state on older devices which still have a physical
> > radio on/off slider on the side might have changed during suspend.
> >
> > So I think it might be better to just drop the tpacpi_rfk_update_swstate
> > call all together from the resume path?
> >
> > Mark do you have any input here?
> >
> > Regards,
> >
> > Hans
> Hi Hans,
>    Thanks you for your recognition.
>    I think your solution would be better. My solution only fix the WWAN device behavior from Foxconn.
>    And Mark, you can contact with gicay@xxxxxxxxxx for the details.
>
> Thanks
> Slark Xiao
Thanks Hans & Slark,

Slark - so I assume are you working with the Foxconn team and Lenovo on 
this issue? I know we've been tracking the fact that suspend doesn't 
work with S3 but haven't been paying attention to the details.

My main concern is 'This may be a fault of LENOVO BIOS' - if the BIOS is 
wrong then we should fix the BIOS rather than kludging the kernel for 
one modem on one platform. Let me know if this needs escalating to the 
BIOS team (or work with Grace).

I've added in Nitin as he's my goto for WWAN related issues and might be 
interested.

Mark

> > > ---
> > >   drivers/platform/x86/thinkpad_acpi.c | 16 ++++++++++++++--
> > >   1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> > > index 603156a6e3ed..e3b7bc0e7a33 100644
> > > --- a/drivers/platform/x86/thinkpad_acpi.c
> > > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > > @@ -1159,6 +1159,13 @@ struct tpacpi_rfk_ops {
> > >   
> > >   static struct tpacpi_rfk *tpacpi_rfkill_switches[TPACPI_RFK_SW_MAX];
> > >   
> > > +/*Foxconn SDX55 T77W175 products. All available device ID*/
> > > +static const struct pci_device_id foxconn_device_ids[] = {
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_FOXCONN, 0xE0AB) },
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_FOXCONN, 0xE0AF) },
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_FOXCONN, 0xE0B4) },
> > > +	{}
> > > +};
> > >   /* Query FW and update rfkill sw state for a given rfkill switch */
> > >   static int tpacpi_rfk_update_swstate(const struct tpacpi_rfk *tp_rfk)
> > >   {
> > > @@ -1182,8 +1189,13 @@ static void tpacpi_rfk_update_swstate_all(void)
> > >   {
> > >   	unsigned int i;
> > >   
> > > -	for (i = 0; i < TPACPI_RFK_SW_MAX; i++)
> > > -		tpacpi_rfk_update_swstate(tpacpi_rfkill_switches[i]);
> > > +	for (i = 0; i < TPACPI_RFK_SW_MAX; i++) {
> > > +		if (pci_dev_present(foxconn_device_ids) && i == 1)
> > > +			pr_info("Find Foxconn wwan device, ignore to update rfkill switch status\n");
> > > +		else
> > > +			tpacpi_rfk_update_swstate(tpacpi_rfkill_switches[i]);
> > > +
> > > +	}
> > >   }
> > >   
> > >   /*