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@xxxxxxxxx>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
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);
> }
