Re: [PATCH v2 1/5] firmware: arm_scmi: Account for SHMEM memory overhead

[Cristian Marussi <cristian.marussi@arm.com>]

On Mon, Oct 21, 2024 at 10:11:44AM -0700, Florian Fainelli wrote:
> On 10/21/24 10:07, Cristian Marussi wrote:
> > Transports using shared memory have to consider the overhead due to the
> > layout area when determining the area effectively available for messages.
> > 

Hi Florian,

thanks for having a look.

> > Till now, such definitions were ambiguos across the SCMI stack and the
> > overhead layout area was not considered at all.
> > 
> > Add proper checks in the shmem layer to validate the provided max_msg_size
> > against the effectively available memory area, less the layout.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > Note that as a consequence of this fix the default max_msg_size is reduced
> > to 104 bytes for shmem-based transports, in order to fit into the most
> > common implementations where the whole shmem area is sized at 128,
> > including the 24 bytes of standard layout area.
> > 
> > This should have NO bad side effects, since the current maximum payload
> > size of any messages across any protocol (including all the known vendor
> > ones) is 76 bytes.
> 
> This looks good to me, just a small nit/suggestion:
> 
> [snip]
> 
> >   	size = resource_size(res);
> > +	if (cinfo->max_msg_size + SCMI_SHMEM_LAYOUT_OVERHEAD > size) {
> > +		dev_err(dev, "misconfigured SCMI shared memory\n");
> > +		return IOMEM_ERR_PTR(-ENOSPC);
> > +	}
> > +
> >   	addr = devm_ioremap(dev, res->start, size);
> >   	if (!addr) {
> >   		dev_err(dev, "failed to ioremap SCMI %s shared memory\n", desc);
> > diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
> > index e7efa3376aae..4e0396250ad0 100644
> > --- a/drivers/firmware/arm_scmi/transports/mailbox.c
> > +++ b/drivers/firmware/arm_scmi/transports/mailbox.c
> > @@ -16,6 +16,8 @@
> >   #include "../common.h"
> > +#define SCMI_MAILBOX_MAX_MSG_SIZE	104
> 
> This IMHO, could be named SCMI_SHMEM_MAX_PAYLOAD_SIZE and used across all 3
> transports that are loosely SHMEM-based?

Yes indeed, just I was not so sure we want to stick to the same default
across different transports that are based on SHMEM....even though they
are in fact the same as of now, and anyway modifiable via DT if his
series goes in...I'll see what Sudeep prefers in these regards.

Thanks,
Cristian