[PATCH] HID: Intel-ish-hid: Ishtp: Fix sensor reads after ACPI S3 suspend

[PATCH] HID: Intel-ish-hid: Ishtp: Fix sensor reads after ACPI S3 suspend

[Even Xu]

After legacy suspend/resume via ACPI S3, sensor read operation fails
with timeout. Also, it will cause delay in resume operation as there
will be retries on failure.

This is caused by commit f645a90e8ff7 ("HID: intel-ish-hid:
ishtp-hid-client: use helper functions for connection"), which used
helper functions to simplify connect, reset and disconnect process.
Also avoid freeing and allocating client buffers again during reconnect
process.

But there is a case, when ISH firmware resets after ACPI S3 suspend,
ishtp bus driver frees client buffers. Since there is no realloc again
during reconnect, there are no client buffers available to send connection
requests to the firmware. Without successful connection to the firmware,
subsequent sensor reads will timeout.

To address this issue, ishtp bus driver does not free client buffers on
warm reset after S3 resume. Simply add the buffers from the read list
to free list of buffers.

Fixes: f645a90e8ff7 ("HID: intel-ish-hid: ishtp-hid-client: use helper functions for connection")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218442
Signed-off-by: Even Xu <even.xu@intel.com>
---
 drivers/hid/intel-ish-hid/ishtp/bus.c    | 2 ++
 drivers/hid/intel-ish-hid/ishtp/client.c | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index aa6cb033bb06..03d5601ce807 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -722,6 +722,8 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev,
 	spin_lock_irqsave(&ishtp_dev->cl_list_lock, flags);
 	list_for_each_entry(cl, &ishtp_dev->cl_list, link) {
 		cl->state = ISHTP_CL_DISCONNECTED;
+		if (warm_reset && cl->device->reference_count)
+			continue;
 
 		/*
 		 * Wake any pending process. The waiter would check dev->state
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index 82c907f01bd3..8a7f2f6a4f86 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -49,7 +49,9 @@ static void ishtp_read_list_flush(struct ishtp_cl *cl)
 	list_for_each_entry_safe(rb, next, &cl->dev->read_list.list, list)
 		if (rb->cl && ishtp_cl_cmp_id(cl, rb->cl)) {
 			list_del(&rb->list);
-			ishtp_io_rb_free(rb);
+			spin_lock(&cl->free_list_spinlock);
+			list_add_tail(&rb->list, &cl->free_rb_list.list);
+			spin_unlock(&cl->free_list_spinlock);
 		}
 	spin_unlock_irqrestore(&cl->dev->read_list_spinlock, flags);
 }
-- 
2.43.0

Re: [PATCH] HID: Intel-ish-hid: Ishtp: Fix sensor reads after ACPI S3 suspend

[srinivas pandruvada]

On Fri, 2024-02-09 at 14:52 +0800, Even Xu wrote:
> After legacy suspend/resume via ACPI S3, sensor read operation fails
> with timeout. Also, it will cause delay in resume operation as there
> will be retries on failure.
> 
> This is caused by commit f645a90e8ff7 ("HID: intel-ish-hid:
> ishtp-hid-client: use helper functions for connection"), which used
> helper functions to simplify connect, reset and disconnect process.
> Also avoid freeing and allocating client buffers again during
> reconnect
> process.
> 
> But there is a case, when ISH firmware resets after ACPI S3 suspend,
> ishtp bus driver frees client buffers. Since there is no realloc
> again
> during reconnect, there are no client buffers available to send
> connection
> requests to the firmware. Without successful connection to the
> firmware,
> subsequent sensor reads will timeout.
> 
> To address this issue, ishtp bus driver does not free client buffers
> on
> warm reset after S3 resume. Simply add the buffers from the read list
> to free list of buffers.
> 
> Fixes: f645a90e8ff7 ("HID: intel-ish-hid: ishtp-hid-client: use
> helper functions for connection")
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218442
> Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Hi Jiri,

This regression is introduced with 6.8-rc1, so need a pull request for
this rc cycle.

Thanks,
Srinivas

> ---
>  drivers/hid/intel-ish-hid/ishtp/bus.c    | 2 ++
>  drivers/hid/intel-ish-hid/ishtp/client.c | 4 +++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c
> b/drivers/hid/intel-ish-hid/ishtp/bus.c
> index aa6cb033bb06..03d5601ce807 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/bus.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
> @@ -722,6 +722,8 @@ void ishtp_bus_remove_all_clients(struct
> ishtp_device *ishtp_dev,
>         spin_lock_irqsave(&ishtp_dev->cl_list_lock, flags);
>         list_for_each_entry(cl, &ishtp_dev->cl_list, link) {
>                 cl->state = ISHTP_CL_DISCONNECTED;
> +               if (warm_reset && cl->device->reference_count)
> +                       continue;
>  
>                 /*
>                  * Wake any pending process. The waiter would check
> dev->state
> diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c
> b/drivers/hid/intel-ish-hid/ishtp/client.c
> index 82c907f01bd3..8a7f2f6a4f86 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/client.c
> @@ -49,7 +49,9 @@ static void ishtp_read_list_flush(struct ishtp_cl
> *cl)
>         list_for_each_entry_safe(rb, next, &cl->dev->read_list.list,
> list)
>                 if (rb->cl && ishtp_cl_cmp_id(cl, rb->cl)) {
>                         list_del(&rb->list);
> -                       ishtp_io_rb_free(rb);
> +                       spin_lock(&cl->free_list_spinlock);
> +                       list_add_tail(&rb->list, &cl-
> >free_rb_list.list);
> +                       spin_unlock(&cl->free_list_spinlock);
>                 }
>         spin_unlock_irqrestore(&cl->dev->read_list_spinlock, flags);
>  }

Re: [PATCH] HID: Intel-ish-hid: Ishtp: Fix sensor reads after ACPI S3 suspend

[Jiri Kosina]

On Fri, 9 Feb 2024, srinivas pandruvada wrote:

> > Fixes: f645a90e8ff7 ("HID: intel-ish-hid: ishtp-hid-client: use
> > helper functions for connection")
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218442
> > Signed-off-by: Even Xu <even.xu@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> Hi Jiri,
> 
> This regression is introduced with 6.8-rc1, so need a pull request for
> this rc cycle.

Right; now queued in hid.git#for-6.8/upstream-fixes. Thanks,

-- 
Jiri Kosina
SUSE Labs

RE: [PATCH] HID: Intel-ish-hid: Ishtp: Fix sensor reads after ACPI S3 suspend

[Xu, Even]

Thanks Jiri and Srinivas!

Best Regards,
Even Xu

-----Original Message-----
From: Jiri Kosina <jikos@kernel.org> 
Sent: Tuesday, February 13, 2024 6:32 PM
To: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Xu, Even <even.xu@intel.com>; benjamin.tissoires@redhat.com; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] HID: Intel-ish-hid: Ishtp: Fix sensor reads after ACPI S3 suspend

On Fri, 9 Feb 2024, srinivas pandruvada wrote:

> > Fixes: f645a90e8ff7 ("HID: intel-ish-hid: ishtp-hid-client: use 
> > helper functions for connection")
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218442
> > Signed-off-by: Even Xu <even.xu@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> Hi Jiri,
> 
> This regression is introduced with 6.8-rc1, so need a pull request for 
> this rc cycle.

Right; now queued in hid.git#for-6.8/upstream-fixes. Thanks,

--
Jiri Kosina
SUSE Labs