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

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

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);
>  }