| Message ID | 20250324-acpm-drained-rx-queue-v1-1-577774335151@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: exynos-acpm: check saved RX before bailing out on empty RX queue | expand |
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,
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; }
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(-) --- base-commit: f0dbe0d40d08199109cb689849877694a8b91033 change-id: 20250324-acpm-drained-rx-queue-ec316d4cbcdf Best regards,