[PATCH] firmware: exynos-acpm: check saved RX before bailing out on empty RX queue

[PATCH] firmware: exynos-acpm: check saved RX before bailing out on empty RX queue

[Tudor Ambarus]

When we're polling for responses and get a response that corresponds to
another request, we save the RX data in order to drain the RX queue.

If the response for the current request is not found in the request's
iteration of the queue, or if the queue is empty, we must check whether
the RX data was saved by a previous request when it drained the RX queue.

We failed to check for already saved responses when the queue was empty,
and requests could time out. Check saved RX before bailing out on empty
RX queue.

Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
Reported-by: André Draszik <andre.draszik@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
on top of krzk/for-next
---
 drivers/firmware/samsung/exynos-acpm.c | 44 +++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
index a85b2dbdd9f0d7b1f327f54a0a283e4f32587a98..15e991b99f5a384a299c1baf6b279fc93244bcf2 100644
--- a/drivers/firmware/samsung/exynos-acpm.c
+++ b/drivers/firmware/samsung/exynos-acpm.c
@@ -184,6 +184,29 @@ struct acpm_match_data {
 #define client_to_acpm_chan(c) container_of(c, struct acpm_chan, cl)
 #define handle_to_acpm_info(h) container_of(h, struct acpm_info, handle)
 
+/**
+ * acpm_get_saved_rx() - get the response if it was already saved.
+ * @achan:	ACPM channel info.
+ * @xfer:	reference to the transfer to get response for.
+ * @tx_seqnum:	xfer TX sequence number.
+ */
+static void acpm_get_saved_rx(struct acpm_chan *achan,
+			      const struct acpm_xfer *xfer, u32 tx_seqnum)
+{
+	const struct acpm_rx_data *rx_data = &achan->rx_data[tx_seqnum - 1];
+	u32 rx_seqnum;
+
+	if (!rx_data->response)
+		return;
+
+	rx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, rx_data->cmd[0]);
+
+	if (rx_seqnum == tx_seqnum) {
+		memcpy(xfer->rxd, rx_data->cmd, xfer->rxlen);
+		clear_bit(rx_seqnum - 1, achan->bitmap_seqnum);
+	}
+}
+
 /**
  * acpm_get_rx() - get response from RX queue.
  * @achan:	ACPM channel info.
@@ -204,15 +227,16 @@ static int acpm_get_rx(struct acpm_chan *achan, const struct acpm_xfer *xfer)
 	rx_front = readl(achan->rx.front);
 	i = readl(achan->rx.rear);
 
-	/* Bail out if RX is empty. */
-	if (i == rx_front)
+	tx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, xfer->txd[0]);
+
+	if (i == rx_front) {
+		acpm_get_saved_rx(achan, xfer, tx_seqnum);
 		return 0;
+	}
 
 	base = achan->rx.base;
 	mlen = achan->mlen;
 
-	tx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, xfer->txd[0]);
-
 	/* Drain RX queue. */
 	do {
 		/* Read RX seqnum. */
@@ -259,16 +283,8 @@ static int acpm_get_rx(struct acpm_chan *achan, const struct acpm_xfer *xfer)
 	 * If the response was not in this iteration of the queue, check if the
 	 * RX data was previously saved.
 	 */
-	rx_data = &achan->rx_data[tx_seqnum - 1];
-	if (!rx_set && rx_data->response) {
-		rx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM,
-				      rx_data->cmd[0]);
-
-		if (rx_seqnum == tx_seqnum) {
-			memcpy(xfer->rxd, rx_data->cmd, xfer->rxlen);
-			clear_bit(rx_seqnum - 1, achan->bitmap_seqnum);
-		}
-	}
+	if (!rx_set)
+		acpm_get_saved_rx(achan, xfer, tx_seqnum);
 
 	return 0;
 }

---
base-commit: f0dbe0d40d08199109cb689849877694a8b91033
change-id: 20250324-acpm-drained-rx-queue-ec316d4cbcdf

Best regards,
-- 
Tudor Ambarus <tudor.ambarus@linaro.org>

Re: [PATCH] firmware: exynos-acpm: check saved RX before bailing out on empty RX queue

[André Draszik]

On Mon, 2025-03-24 at 12:35 +0000, Tudor Ambarus wrote:
> When we're polling for responses and get a response that corresponds to
> another request, we save the RX data in order to drain the RX queue.
> 
> If the response for the current request is not found in the request's
> iteration of the queue, or if the queue is empty, we must check whether
> the RX data was saved by a previous request when it drained the RX queue.
> 
> We failed to check for already saved responses when the queue was empty,
> and requests could time out. Check saved RX before bailing out on empty
> RX queue.
> 
> Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
> Reported-by: André Draszik <andre.draszik@linaro.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
> on top of krzk/for-next
> ---
>  drivers/firmware/samsung/exynos-acpm.c | 44 +++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
> index a85b2dbdd9f0d7b1f327f54a0a283e4f32587a98..15e991b99f5a384a299c1baf6b279fc93244bcf2 100644
> --- a/drivers/firmware/samsung/exynos-acpm.c
> +++ b/drivers/firmware/samsung/exynos-acpm.c
> @@ -184,6 +184,29 @@ struct acpm_match_data {
>  #define client_to_acpm_chan(c) container_of(c, struct acpm_chan, cl)
>  #define handle_to_acpm_info(h) container_of(h, struct acpm_info, handle)
>  
> +/**
> + * acpm_get_saved_rx() - get the response if it was already saved.
> + * @achan:	ACPM channel info.
> + * @xfer:	reference to the transfer to get response for.
> + * @tx_seqnum:	xfer TX sequence number.
> + */
> +static void acpm_get_saved_rx(struct acpm_chan *achan,
> +			      const struct acpm_xfer *xfer, u32 tx_seqnum)
> +{
> +	const struct acpm_rx_data *rx_data = &achan->rx_data[tx_seqnum - 1];
> +	u32 rx_seqnum;
> +
> +	if (!rx_data->response)
> +		return;
> +
> +	rx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, rx_data->cmd[0]);
> +
> +	if (rx_seqnum == tx_seqnum) {

To help the casual reader, maybe add a comment to say that this condition is true
if/when the response was received, and before reception rx_seqnum will be == 0,
because acpm_prepare_xfer() clears it - i.e. it is not ever supposed to be any
arbitrary number. It's kinda implied, but a comment like that would make this
more explicit. If I'm getting this all right.

Other that that;

Reviewed-by: André Draszik <andre.draszik@linaro.org>
Tested-by: André Draszik <andre.draszik@linaro.org>

> +		memcpy(xfer->rxd, rx_data->cmd, xfer->rxlen);
> +		clear_bit(rx_seqnum - 1, achan->bitmap_seqnum);
> +	}
> +}
> +
>  /**
>   * acpm_get_rx() - get response from RX queue.
>   * @achan:	ACPM channel info.
> @@ -204,15 +227,16 @@ static int acpm_get_rx(struct acpm_chan *achan, const struct acpm_xfer *xfer)
>  	rx_front = readl(achan->rx.front);
>  	i = readl(achan->rx.rear);
>  
> -	/* Bail out if RX is empty. */
> -	if (i == rx_front)
> +	tx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, xfer->txd[0]);
> +
> +	if (i == rx_front) {
> +		acpm_get_saved_rx(achan, xfer, tx_seqnum);
>  		return 0;
> +	}
>  
>  	base = achan->rx.base;
>  	mlen = achan->mlen;
>  
> -	tx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, xfer->txd[0]);
> -
>  	/* Drain RX queue. */
>  	do {
>  		/* Read RX seqnum. */
> @@ -259,16 +283,8 @@ static int acpm_get_rx(struct acpm_chan *achan, const struct acpm_xfer *xfer)
>  	 * If the response was not in this iteration of the queue, check if the
>  	 * RX data was previously saved.
>  	 */
> -	rx_data = &achan->rx_data[tx_seqnum - 1];
> -	if (!rx_set && rx_data->response) {
> -		rx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM,
> -				      rx_data->cmd[0]);
> -
> -		if (rx_seqnum == tx_seqnum) {
> -			memcpy(xfer->rxd, rx_data->cmd, xfer->rxlen);
> -			clear_bit(rx_seqnum - 1, achan->bitmap_seqnum);
> -		}
> -	}
> +	if (!rx_set)
> +		acpm_get_saved_rx(achan, xfer, tx_seqnum);
>  
>  	return 0;
>  }
> 
> ---
> base-commit: f0dbe0d40d08199109cb689849877694a8b91033
> change-id: 20250324-acpm-drained-rx-queue-ec316d4cbcdf
> 
> Best regards,

Re: [PATCH] firmware: exynos-acpm: check saved RX before bailing out on empty RX queue

[Krzysztof Kozlowski]

On Mon, 24 Mar 2025 12:35:41 +0000, Tudor Ambarus wrote:
> When we're polling for responses and get a response that corresponds to
> another request, we save the RX data in order to drain the RX queue.
> 
> If the response for the current request is not found in the request's
> iteration of the queue, or if the queue is empty, we must check whether
> the RX data was saved by a previous request when it drained the RX queue.
> 
> [...]

Applied, thanks!

[1/1] firmware: exynos-acpm: check saved RX before bailing out on empty RX queue
      https://git.kernel.org/krzk/linux/c/2908ffa53f8ed487ab4285635c79d4911cced93c

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>