Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 24 Jun 2022 10:22:58 +0100
From:   Cristian Marussi <cristian.marussi@arm.com>
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org,
        etienne.carriere@linaro.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] scmi/optee: fix response size warning
Message-ID: <YrWCctNSyKcqSHE9@e120937-lin>
References: <20220624074549.3298-1-vincent.guittot@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220624074549.3298-1-vincent.guittot@linaro.org>
Precedence: bulk

On Fri, Jun 24, 2022 at 09:45:49AM +0200, Vincent Guittot wrote:
> Some protocols check the response size with the expected value but optee
> shared memory doesn't return such size whereas it is available in the
> optee output buffer.
> 
> As an example, the base protocol compares the response size with the
> expected result when requesting the list of protocol which triggers a
> warning with optee shared memory:
> 
> [    1.260306] arm-scmi firmware:scmi0: Malformed reply - real_sz:116  calc_sz:4  (loop_num_ret:4)
> 
> Save the output buffer length and use it when fetching the answer.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

Hi Vincent,

> ---
> 
> Tested on sudeep's for-next/scmi branch
> 
>  drivers/firmware/arm_scmi/optee.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> index b503c22cfd32..8abace56b958 100644
> --- a/drivers/firmware/arm_scmi/optee.c
> +++ b/drivers/firmware/arm_scmi/optee.c
> @@ -117,6 +117,7 @@ struct scmi_optee_channel {
>  	u32 channel_id;
>  	u32 tee_session;
>  	u32 caps;
> +	u32 rx_len;
>  	struct mutex mu;
>  	struct scmi_chan_info *cinfo;
>  	union {
> @@ -302,6 +303,9 @@ static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t
>  		return -EIO;
>  	}
>  
> +	/* Save response size */
> +	channel->rx_len = param[2].u.memref.size;
> +
>  	return 0;
>  }
>  
> @@ -353,6 +357,7 @@ static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *ch
>  	shbuf = tee_shm_get_va(channel->tee_shm, 0);
>  	memset(shbuf, 0, msg_size);
>  	channel->req.msg = shbuf;
> +	channel->rx_len = msg_size;
>  
>  	return 0;
>  }
> @@ -508,7 +513,7 @@ static void scmi_optee_fetch_response(struct scmi_chan_info *cinfo,
>  	struct scmi_optee_channel *channel = cinfo->transport_info;
>  
>  	if (channel->tee_shm)
> -		msg_fetch_response(channel->req.msg, SCMI_OPTEE_MAX_MSG_SIZE, xfer);
> +		msg_fetch_response(channel->req.msg, channel->rx_len, xfer);
>  	else
>  		shmem_fetch_response(channel->req.shmem, xfer);
>  }

Looks good to me.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian