[PATCH v2 1/2] firmware: arm_scpi: zero RX buffer before requesting data from the mbox

[tixy@linaro.org (Jon Medhurst (Tixy))]

On Fri, 2016-11-25 at 01:54 +0100, Martin Blumenstingl wrote:
> The original code was relying on the fact that the SCPI firmware
> responds with the same number of bytes (or more, all extra data would be
> ignored in that case) as requested.
> However, we have some pre-v1.0 SCPI firmwares which are responding with
> less data for some commands (sensor_value.hi_val did not exist in the
> old implementation). This means that some data from the previous
> command's RX buffer was leaked into the current command (as the RX
> buffer is re-used for all commands on the same channel). Clearing the
> RX buffer before (re-) using it ensures we get a consistent result, even
> if the SCPI firmware returns less bytes than requested.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/firmware/arm_scpi.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 70e1323..8c183d8 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -259,6 +259,7 @@ struct scpi_chan {
>  	struct mbox_chan *chan;
>  	void __iomem *tx_payload;
>  	void __iomem *rx_payload;
> +	resource_size_t max_payload_len;

Ah, here's max_payload_len, sorry, I reviewed the patches in the wrong
order. And reflecting on things, having the runtime

>  	struct list_head rx_pending;
>  	struct list_head xfers_list;
>  	struct scpi_xfer *xfers;
> @@ -470,6 +471,20 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>  	if (t->rx_buf) {
>  		if (!(++ch->token))
>  			++ch->token;
> +
> +		/* clear the RX buffer as it is shared across all commands on
> +		 * the same channel (to make sure we're not leaking data from
> +		 * the previous response into the current command if the SCPI
> +		 * firmware writes less data than requested).
> +		 * This is especially important for pre-v1.0 SCPI firmwares
> +		 * where some fields in the responses do not exist (while they
> +		 * exist but are optional in newer versions). One example for
> +		 * this problem is sensor_value.hi_val, which would contain
> +		 * ("leak") the second 4 bytes of the RX buffer from the
> +		 * previous command.
> +		 */
> +		memset_io(ch->rx_payload, 0, ch->max_payload_len);
> +

Isn't the payload size specified in the header? In which case the bug
you describe is due to the implementation writing 4 bytes and setting
the length to 8. Anyway, this seems almost like a quirk of a specific
implementation and perhaps should be handled as such, rather that doing
this for all commands on all boards using SCPI.

>  		ADD_SCPI_TOKEN(t->cmd, ch->token);
>  		spin_lock_irqsave(&ch->rx_lock, flags);
>  		list_add_tail(&t->node, &ch->rx_pending);
> @@ -921,7 +936,9 @@ static int scpi_probe(struct platform_device *pdev)
>  			ret = -EADDRNOTAVAIL;
>  			goto err;
>  		}
> -		pchan->tx_payload = pchan->rx_payload + (size >> 1);
> +
> +		pchan->max_payload_len = size / 2;
> +		pchan->tx_payload = pchan->rx_payload + pchan->max_payload_len;
>  
>  		cl->dev = dev;
>  		cl->rx_callback = scpi_handle_remote_msg;

-- 
Tixy