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@xxxxxxxxxx>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
Tested-by: André Draszik <andre.draszik@xxxxxxxxxx>

> +		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,