[PATCH] firmware: samsung: fix stale response flag in acpm_prepare_xfer()

[PATCH] firmware: samsung: fix stale response flag in acpm_prepare_xfer()

[Titouan Ameline de Cadeville]

acpm_prepare_xfer() only ever set rx_data->response to true, never
false. A reused sequence number slot could therefore inherit a stale
true from a previous transfer that expected a response, causing
acpm_get_rx() to enter the response-copy path for a fire-and-forget
transfer whose rxd is NULL.

Unconditionally assign the correct boolean value so the slot is fully
reset on every reuse.

Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
Signed-off-by: Titouan Ameline de Cadeville <titouan.ameline@gmail.com>
---
 drivers/firmware/samsung/exynos-acpm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
index 16c46ed60837..2fee6bb60efc 100644
--- a/drivers/firmware/samsung/exynos-acpm.c
+++ b/drivers/firmware/samsung/exynos-acpm.c
@@ -380,8 +380,7 @@ static void acpm_prepare_xfer(struct acpm_chan *achan,
 	/* Clear data for upcoming responses */
 	rx_data = &achan->rx_data[achan->seqnum - 1];
 	memset(rx_data->cmd, 0, sizeof(*rx_data->cmd) * rx_data->n_cmd);
-	if (xfer->rxd)
-		rx_data->response = true;
+	rx_data->response = !!xfer->rxd;
 
 	/* Flag the index based on seqnum. (seqnum: 1~63, bitmap: 0~62) */
 	set_bit(achan->seqnum - 1, achan->bitmap_seqnum);
-- 
2.44.2

Re: [PATCH] firmware: samsung: fix stale response flag in acpm_prepare_xfer()

[Tudor Ambarus]

Hi,

Thanks for the patch!

On 4/27/26 12:02 AM, Titouan Ameline de Cadeville wrote:
> acpm_prepare_xfer() only ever set rx_data->response to true, never
> false. A reused sequence number slot could therefore inherit a stale
> true from a previous transfer that expected a response, causing
> acpm_get_rx() to enter the response-copy path for a fire-and-forget
> transfer whose rxd is NULL.
> 
> Unconditionally assign the correct boolean value so the slot is fully
> reset on every reuse.
> 

How did you find this?

Sashiko identified this too when reviewing the ACPM thermal patches.
I sent some fixes last week, where this bug is squashed as well:

https://lore.kernel.org/linux-samsung-soc/20260423-acpm-fixes-sashiko-reports-v1-0-2217b790925e@linaro.org/T/#m1c32aa4c84ea7e3909bc8fe7599585b71e95d8b5


Thanks!
ta

> Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
> Signed-off-by: Titouan Ameline de Cadeville <titouan.ameline@gmail.com>
> ---
>  drivers/firmware/samsung/exynos-acpm.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
> index 16c46ed60837..2fee6bb60efc 100644
> --- a/drivers/firmware/samsung/exynos-acpm.c
> +++ b/drivers/firmware/samsung/exynos-acpm.c
> @@ -380,8 +380,7 @@ static void acpm_prepare_xfer(struct acpm_chan *achan,
>  	/* Clear data for upcoming responses */
>  	rx_data = &achan->rx_data[achan->seqnum - 1];
>  	memset(rx_data->cmd, 0, sizeof(*rx_data->cmd) * rx_data->n_cmd);
> -	if (xfer->rxd)
> -		rx_data->response = true;
> +	rx_data->response = !!xfer->rxd;
>  
>  	/* Flag the index based on seqnum. (seqnum: 1~63, bitmap: 0~62) */
>  	set_bit(achan->seqnum - 1, achan->bitmap_seqnum);

Re: [PATCH] firmware: samsung: fix stale response flag in acpm_prepare_xfer()

[Titouan Ameline]

I was reading through the driver code and noticed that
acpm_prepare_xfer() only enters the

if (xfer->rxd) branch to set response = true
with no corresponding else to reset it to false.

Since seqnum slots are recycled, I traced what happens when a slot
previously used by a response-expecting transfer gets reused by a
fire-and-forget one

-> the stale true remains and the wrong branch is taken in acpm_get_rx().

Le lun. 27 avr. 2026 à 10:48, Tudor Ambarus <tudor.ambarus@linaro.org> a écrit :
>
> Hi,
>
> Thanks for the patch!
>
> On 4/27/26 12:02 AM, Titouan Ameline de Cadeville wrote:
> > acpm_prepare_xfer() only ever set rx_data->response to true, never
> > false. A reused sequence number slot could therefore inherit a stale
> > true from a previous transfer that expected a response, causing
> > acpm_get_rx() to enter the response-copy path for a fire-and-forget
> > transfer whose rxd is NULL.
> >
> > Unconditionally assign the correct boolean value so the slot is fully
> > reset on every reuse.
> >
>
> How did you find this?
>
> Sashiko identified this too when reviewing the ACPM thermal patches.
> I sent some fixes last week, where this bug is squashed as well:
>
> https://lore.kernel.org/linux-samsung-soc/20260423-acpm-fixes-sashiko-reports-v1-0-2217b790925e@linaro.org/T/#m1c32aa4c84ea7e3909bc8fe7599585b71e95d8b5
>
>
> Thanks!
> ta
>
> > Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
> > Signed-off-by: Titouan Ameline de Cadeville <titouan.ameline@gmail.com>
> > ---
> >  drivers/firmware/samsung/exynos-acpm.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
> > index 16c46ed60837..2fee6bb60efc 100644
> > --- a/drivers/firmware/samsung/exynos-acpm.c
> > +++ b/drivers/firmware/samsung/exynos-acpm.c
> > @@ -380,8 +380,7 @@ static void acpm_prepare_xfer(struct acpm_chan *achan,
> >       /* Clear data for upcoming responses */
> >       rx_data = &achan->rx_data[achan->seqnum - 1];
> >       memset(rx_data->cmd, 0, sizeof(*rx_data->cmd) * rx_data->n_cmd);
> > -     if (xfer->rxd)
> > -             rx_data->response = true;
> > +     rx_data->response = !!xfer->rxd;
> >
> >       /* Flag the index based on seqnum. (seqnum: 1~63, bitmap: 0~62) */
> >       set_bit(achan->seqnum - 1, achan->bitmap_seqnum);
>

Re: [PATCH] firmware: samsung: fix stale response flag in acpm_prepare_xfer()

[Krzysztof Kozlowski]

On 27/04/2026 23:37, Titouan Ameline wrote:
> I was reading through the driver code and noticed that
> acpm_prepare_xfer() only enters the
> 
> if (xfer->rxd) branch to set response = true
> with no corresponding else to reset it to false.
> 
> Since seqnum slots are recycled, I traced what happens when a slot
> previously used by a response-expecting transfer gets reused by a
> fire-and-forget one
> 
> -> the stale true remains and the wrong branch is taken in acpm_get_rx().
> 
> Le lun. 27 avr. 2026 à 10:48, Tudor Ambarus <tudor.ambarus@linaro.org> a écrit :
>>

This seams to duplicate Tudor's earlier patches and his solution looks
more complete, so I will take his work with Reported-by credits.

Best regards,
Krzysztof