Received-SPF: pass (google.com: domain of linux-kernel+bounces-59486-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <64ee580b9969335d60966e23e9bd859e8f075953.camel@linux.intel.com>
Subject: Re: [PATCH] HID: Intel-ish-hid: Ishtp: Fix sensor reads after ACPI
 S3 suspend
From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
To: Even Xu <even.xu@intel.com>, jikos@kernel.org, 
	benjamin.tissoires@redhat.com, linux-input@vger.kernel.org
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Date: Fri, 09 Feb 2024 06:41:53 -0800
In-Reply-To: <20240209065232.15486-1-even.xu@intel.com>
References: <20240209065232.15486-1-even.xu@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-3.fc36) 
Precedence: bulk
MIME-Version: 1.0

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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> Fixes: f645a90e8ff7 ("HID: intel-ish-hid: ishtp-hid-client: use
> helper functions for connection")
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=3D218442
> 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

> ---
> =C2=A0drivers/hid/intel-ish-hid/ishtp/bus.c=C2=A0=C2=A0=C2=A0 | 2 ++
> =C2=A0drivers/hid/intel-ish-hid/ishtp/client.c | 4 +++-
> =C2=A02 files changed, 5 insertions(+), 1 deletion(-)
>=20
> 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,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_lock_irqsave(&ishtp_=
dev->cl_list_lock, flags);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0list_for_each_entry(cl, &=
ishtp_dev->cl_list, link) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0cl->state =3D ISHTP_CL_DISCONNECTED;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0if (warm_reset && cl->device->reference_count)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0continue;
> =C2=A0
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0/*
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 * 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)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0list_for_each_entry_safe(=
rb, next, &cl->dev->read_list.list,
> list)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0if (rb->cl && ishtp_cl_cmp_id(cl, rb->cl)) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0lis=
t_del(&rb->list);
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ishtp_io_=
rb_free(rb);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_lock=
(&cl->free_list_spinlock);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0list_add_=
tail(&rb->list, &cl-
> >free_rb_list.list);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_unlo=
ck(&cl->free_list_spinlock);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0}
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irqrestore(&c=
l->dev->read_list_spinlock, flags);
> =C2=A0}