[PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Justin Chen]

The mailbox layer has its own queue. However this confuses the per
message timeouts since the clock starts ticking the moment the messages
get queued up. So all messages in the queue have there timeout clocks
ticking instead of only the message inflight. To fix this, lets move the
queue back into the SCMI layer.

Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 .../firmware/arm_scmi/transports/mailbox.c    | 21 +++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
index 1a754dee24f7..30bc2865582f 100644
--- a/drivers/firmware/arm_scmi/transports/mailbox.c
+++ b/drivers/firmware/arm_scmi/transports/mailbox.c
@@ -33,6 +33,7 @@ struct scmi_mailbox {
 	struct mbox_chan *chan_platform_receiver;
 	struct scmi_chan_info *cinfo;
 	struct scmi_shared_mem __iomem *shmem;
+	struct mutex chan_lock;
 };
 
 #define client_to_scmi_mailbox(c) container_of(c, struct scmi_mailbox, cl)
@@ -205,6 +206,7 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	cl->rx_callback = rx_callback;
 	cl->tx_block = false;
 	cl->knows_txdone = tx;
+	mutex_init(&smbox->chan_lock);
 
 	smbox->chan = mbox_request_channel(cl, tx ? 0 : p2a_chan);
 	if (IS_ERR(smbox->chan)) {
@@ -267,11 +269,21 @@ static int mailbox_send_message(struct scmi_chan_info *cinfo,
 	struct scmi_mailbox *smbox = cinfo->transport_info;
 	int ret;
 
+	/*
+	 * The mailbox layer has it's own queue. However the mailbox queue confuses
+	 * the per message SCMI timeouts since the clock starts when the message is
+	 * submitted into the mailbox queue. So when multiple messages are queued up
+	 * the clock starts on all messages instead of only the one inflight.
+	 */
+	mutex_lock(&smbox->chan_lock);
+
 	ret = mbox_send_message(smbox->chan, xfer);
 
 	/* mbox_send_message returns non-negative value on success, so reset */
 	if (ret > 0)
 		ret = 0;
+	else
+		mutex_unlock(&smbox->chan_lock);
 
 	return ret;
 }
@@ -281,13 +293,10 @@ static void mailbox_mark_txdone(struct scmi_chan_info *cinfo, int ret,
 {
 	struct scmi_mailbox *smbox = cinfo->transport_info;
 
-	/*
-	 * NOTE: we might prefer not to need the mailbox ticker to manage the
-	 * transfer queueing since the protocol layer queues things by itself.
-	 * Unfortunately, we have to kick the mailbox framework after we have
-	 * received our message.
-	 */
 	mbox_client_txdone(smbox->chan, ret);
+
+	/* Release channel */
+	mutex_unlock(&smbox->chan_lock);
 }
 
 static void mailbox_fetch_response(struct scmi_chan_info *cinfo,
-- 
2.34.1

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Cristian Marussi]

On Fri, Oct 04, 2024 at 03:12:57PM -0700, Justin Chen wrote:
> The mailbox layer has its own queue. However this confuses the per
> message timeouts since the clock starts ticking the moment the messages
> get queued up. So all messages in the queue have there timeout clocks
> ticking instead of only the message inflight. To fix this, lets move the
> queue back into the SCMI layer.

Hi Justin,

thanks for reporting this, I'll try to understand better in the
following since we always avoided a lock in the mailbox driver (till
now) since it seemed not to be needed due to the interaction and inner
workings of the mailbox subsytem and its queues....so I just want to
understand better...not saying that this patch is wrong by itself for
now...

when you say:

"this confuses the per-message timeouts since the clock starts ticking
the moment the message get queued up."

...you are referring to the core mailbox subsystem queues and the code flow
that derives from mbox_send_message() right ?

So roughly the mailbox subsystem callchain involved would be this:

mbox_send_message()
	add_to_rbuf()
	msg_submit()

Am I right ? Is this the code flow you are referring to m?

In this case I am not sure what you mean with "the timer starts ticking
as soon as the message is queued"...which timer ? ... because:

 - the SCMI mailbox client is registered as "tx_block = false" so the
   sent messages are never waited for with wait_for_completion

and on the other side

 - once we have 1 message in-flight already, the next attempt to send a
   message will result in a call to add_to_rbuf() BUT the following
   msg_submit() will bailout immediately at line 63

	if (!chan->msg_count || chan->active_req)
		goto exit;

   after the first message is queued since there is an active_req
   inflight, and so no more messages will be queued till the next
   tx_tick(), which is invoked only by us from the SCMI stack when the
   reply to the inflight message has been read back. (this also means
   tx_prepare is NOT called until we kick from SCMI MBOX driver with
   mbox_client_txdone()

What I am missing, here ?

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Sudeep Holla]

On Fri, Oct 04, 2024 at 03:12:57PM -0700, Justin Chen wrote:
> The mailbox layer has its own queue. However this confuses the per
> message timeouts since the clock starts ticking the moment the messages
> get queued up. So all messages in the queue have there timeout clocks
> ticking instead of only the message inflight. To fix this, lets move the
> queue back into the SCMI layer.
>

I think this has come up in the past. We have avoided adding addition
locking here as the mailbox layer takes care of it. Has anything changed
recently ?

--
Regards,
Sudeep

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Cristian Marussi]

On Mon, Oct 07, 2024 at 02:04:10PM +0100, Sudeep Holla wrote:
> On Fri, Oct 04, 2024 at 03:12:57PM -0700, Justin Chen wrote:
> > The mailbox layer has its own queue. However this confuses the per
> > message timeouts since the clock starts ticking the moment the messages
> > get queued up. So all messages in the queue have there timeout clocks
> > ticking instead of only the message inflight. To fix this, lets move the
> > queue back into the SCMI layer.
> >
> 
> I think this has come up in the past. We have avoided adding addition
> locking here as the mailbox layer takes care of it. Has anything changed
> recently ?

I asked for an explanation in my reply (we crossed each other mails probably)
since it alredy came up in the past a few times and central locking seemed not
to be needed...here the difference is about the reason...Justin talks about
message timeouts related to the queueing process..so I asked to better
explain the detail (and the anbomaly observed) since it still does not
seem to me that even in this case the lock is needed....anyway I can
definitely be woring of course :D

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Justin Chen]

On 10/7/24 6:10 AM, Cristian Marussi wrote:
> On Mon, Oct 07, 2024 at 02:04:10PM +0100, Sudeep Holla wrote:
>> On Fri, Oct 04, 2024 at 03:12:57PM -0700, Justin Chen wrote:
>>> The mailbox layer has its own queue. However this confuses the per
>>> message timeouts since the clock starts ticking the moment the messages
>>> get queued up. So all messages in the queue have there timeout clocks
>>> ticking instead of only the message inflight. To fix this, lets move the
>>> queue back into the SCMI layer.
>>>
>>
>> I think this has come up in the past. We have avoided adding addition
>> locking here as the mailbox layer takes care of it. Has anything changed
>> recently ?
> 
> I asked for an explanation in my reply (we crossed each other mails probably)
> since it alredy came up in the past a few times and central locking seemed not
> to be needed...here the difference is about the reason...Justin talks about
> message timeouts related to the queueing process..so I asked to better
> explain the detail (and the anbomaly observed) since it still does not
> seem to me that even in this case the lock is needed....anyway I can
> definitely be woring of course :D
> 

Hello Cristian,

Thanks for the response. I'll try to elaborate.

When comparing SMC and mailbox transport, we noticed mailbox transport 
timesout much quicker when under load. Originally we thought this was 
the latency of the mailbox implementation, but after debugging we 
noticed a weird behavior. We saw SMCI transactions timing out before the 
mailbox even transmitted the message.

This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c 
do_xfer() function.

The fundamental issue is send_message() blocks for SMC transport, but 
doesn't block for mailbox transport. So if send_message() doesn't block 
we can have multiple messages waiting at scmi_wait_for_message_response().

SMC looks like this
CPU #0 SCMI message 0 -> calls send_message() then calls 
scmi_wait_for_message_response(), timesout after 30ms.
CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI 
message 0 to complete.

Mailbox looks like this
CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues up 
message, mailbox layer sees no message is outgoing and sends it. CPU 
waits at scmi_wait_for_message_response(), timesout after 30ms
CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues up 
message, mailbox layer sees message pending, hold message in queue. CPU 
waits at scmi_wait_for_message_response(), timesout after 30ms.

Lets say if transport takes 25ms. The first message would succeed, the 
second message would timeout after 5ms.

Hopefully this makes sense.

Justin

RE: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Peng Fan]

> Subject: Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for
> mailbox implementation
> 
> 
> 
> On 10/7/24 6:10 AM, Cristian Marussi wrote:
> > On Mon, Oct 07, 2024 at 02:04:10PM +0100, Sudeep Holla wrote:
> >> On Fri, Oct 04, 2024 at 03:12:57PM -0700, Justin Chen wrote:
> >>> The mailbox layer has its own queue. However this confuses the
> per
> >>> message timeouts since the clock starts ticking the moment the
> >>> messages get queued up. So all messages in the queue have there
> >>> timeout clocks ticking instead of only the message inflight. To fix
> >>> this, lets move the queue back into the SCMI layer.
> >>>
> >>
> >> I think this has come up in the past. We have avoided adding
> addition
> >> locking here as the mailbox layer takes care of it. Has anything
> >> changed recently ?
> >
> > I asked for an explanation in my reply (we crossed each other mails
> > probably) since it alredy came up in the past a few times and central
> > locking seemed not to be needed...here the difference is about the
> > reason...Justin talks about message timeouts related to the queueing
> > process..so I asked to better explain the detail (and the anbomaly
> > observed) since it still does not seem to me that even in this case
> > the lock is needed....anyway I can definitely be woring of course :D
> >
> 
> Hello Cristian,
> 
> Thanks for the response. I'll try to elaborate.
> 
> When comparing SMC and mailbox transport, we noticed mailbox
> transport timesout much quicker when under load. Originally we
> thought this was the latency of the mailbox implementation, but after
> debugging we noticed a weird behavior. We saw SMCI transactions
> timing out before the mailbox even transmitted the message.
> 
> This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
> do_xfer() function.
> 
> The fundamental issue is send_message() blocks for SMC transport, but
> doesn't block for mailbox transport. So if send_message() doesn't block
> we can have multiple messages waiting at
> scmi_wait_for_message_response().
> 
> SMC looks like this
> CPU #0 SCMI message 0 -> calls send_message() then calls
> scmi_wait_for_message_response(), timesout after 30ms.
> CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI
> message 0 to complete.
> 
> Mailbox looks like this
> CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues
> up message, mailbox layer sees no message is outgoing and sends it.
> CPU waits at scmi_wait_for_message_response(), timesout after 30ms
> CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues
> up message, mailbox layer sees message pending, hold message in
> queue. CPU waits at scmi_wait_for_message_response(), timesout after
> 30ms.
> 
> Lets say if transport takes 25ms. The first message would succeed, the
> second message would timeout after 5ms.

Each xfer has its own completion, how the 2nd impacts the 1st?

Regards,
Peng.

> 
> Hopefully this makes sense.
> 
> Justin
> 

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Justin Chen]

On 10/7/2024 7:43 PM, Peng Fan wrote:
>> Subject: Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for
>> mailbox implementation
>>
>>
>>
>> On 10/7/24 6:10 AM, Cristian Marussi wrote:
>>> On Mon, Oct 07, 2024 at 02:04:10PM +0100, Sudeep Holla wrote:
>>>> On Fri, Oct 04, 2024 at 03:12:57PM -0700, Justin Chen wrote:
>>>>> The mailbox layer has its own queue. However this confuses the
>> per
>>>>> message timeouts since the clock starts ticking the moment the
>>>>> messages get queued up. So all messages in the queue have there
>>>>> timeout clocks ticking instead of only the message inflight. To fix
>>>>> this, lets move the queue back into the SCMI layer.
>>>>>
>>>>
>>>> I think this has come up in the past. We have avoided adding
>> addition
>>>> locking here as the mailbox layer takes care of it. Has anything
>>>> changed recently ?
>>>
>>> I asked for an explanation in my reply (we crossed each other mails
>>> probably) since it alredy came up in the past a few times and central
>>> locking seemed not to be needed...here the difference is about the
>>> reason...Justin talks about message timeouts related to the queueing
>>> process..so I asked to better explain the detail (and the anbomaly
>>> observed) since it still does not seem to me that even in this case
>>> the lock is needed....anyway I can definitely be woring of course :D
>>>
>>
>> Hello Cristian,
>>
>> Thanks for the response. I'll try to elaborate.
>>
>> When comparing SMC and mailbox transport, we noticed mailbox
>> transport timesout much quicker when under load. Originally we
>> thought this was the latency of the mailbox implementation, but after
>> debugging we noticed a weird behavior. We saw SMCI transactions
>> timing out before the mailbox even transmitted the message.
>>
>> This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
>> do_xfer() function.
>>
>> The fundamental issue is send_message() blocks for SMC transport, but
>> doesn't block for mailbox transport. So if send_message() doesn't block
>> we can have multiple messages waiting at
>> scmi_wait_for_message_response().
>>
>> SMC looks like this
>> CPU #0 SCMI message 0 -> calls send_message() then calls
>> scmi_wait_for_message_response(), timesout after 30ms.
>> CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI
>> message 0 to complete.
>>
>> Mailbox looks like this
>> CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues
>> up message, mailbox layer sees no message is outgoing and sends it.
>> CPU waits at scmi_wait_for_message_response(), timesout after 30ms
>> CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues
>> up message, mailbox layer sees message pending, hold message in
>> queue. CPU waits at scmi_wait_for_message_response(), timesout after
>> 30ms.
>>
>> Lets say if transport takes 25ms. The first message would succeed, the
>> second message would timeout after 5ms.
> 
> Each xfer has its own completion, how the 2nd impacts the 1st?
> 
> Regards,
> Peng.
> 

Hello Peng,

The mailbox layer queues messages and doesn't block when send_message() 
is called. So we can have both xfer waiting for completion at the same time.

Lets assume a message takes 25ms to complete, with a 30ms timeout.

0ms Message #0 is queued in mailbox layer and sent out, then sits at 
scmi_wait_for_message_response() with a timeout of 30ms
1ms Message #1 is queued in mailbox layer but not sent out yet. Since 
send_message() doesn't block, it also sits at 
scmi_wait_for_message_response() with a timeout of 30ms
...
25ms Message #0 is completed, txdone is called and Message #1 is sent out
31ms Message #1 times out since the count started at 1ms. Even though it 
has only been inflight for 6ms.

Thanks,
Justin

>>
>> Hopefully this makes sense.
>>
>> Justin
>>
> 

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Cristian Marussi]

On Mon, Oct 07, 2024 at 10:58:47AM -0700, Justin Chen wrote:
> 
> 
> On 10/7/24 6:10 AM, Cristian Marussi wrote:
> > On Mon, Oct 07, 2024 at 02:04:10PM +0100, Sudeep Holla wrote:
> > > On Fri, Oct 04, 2024 at 03:12:57PM -0700, Justin Chen wrote:
> > > > The mailbox layer has its own queue. However this confuses the per
> > > > message timeouts since the clock starts ticking the moment the messages
> > > > get queued up. So all messages in the queue have there timeout clocks
> > > > ticking instead of only the message inflight. To fix this, lets move the
> > > > queue back into the SCMI layer.
> > > > 
> > > 
> > > I think this has come up in the past. We have avoided adding addition
> > > locking here as the mailbox layer takes care of it. Has anything changed
> > > recently ?
> > 
> > I asked for an explanation in my reply (we crossed each other mails probably)
> > since it alredy came up in the past a few times and central locking seemed not
> > to be needed...here the difference is about the reason...Justin talks about
> > message timeouts related to the queueing process..so I asked to better
> > explain the detail (and the anbomaly observed) since it still does not
> > seem to me that even in this case the lock is needed....anyway I can
> > definitely be woring of course :D
> > 
> 
> Hello Cristian,
> 

Hi Justin,

> Thanks for the response. I'll try to elaborate.
> 
> When comparing SMC and mailbox transport, we noticed mailbox transport
> timesout much quicker when under load. Originally we thought this was the
> latency of the mailbox implementation, but after debugging we noticed a
> weird behavior. We saw SMCI transactions timing out before the mailbox even
> transmitted the message.
> 
> This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
> do_xfer() function.
> 
> The fundamental issue is send_message() blocks for SMC transport, but
> doesn't block for mailbox transport. So if send_message() doesn't block we
> can have multiple messages waiting at scmi_wait_for_message_response().
> 

oh...yes...now I can see it...tx_prepare is really never called given
how the mailbox subsystem de-queues messages once at time...so we end up
waiting for a reply to some message that is still to be sent...so the
message inflight is really NOT corrupted because the next remain pending
until the reply in the shmem is read back , BUT the timeout will drift away
if you multiple inflights are pending to be sent...

> SMC looks like this
> CPU #0 SCMI message 0 -> calls send_message() then calls
> scmi_wait_for_message_response(), timesout after 30ms.
> CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI message 0
> to complete.
> 
> Mailbox looks like this
> CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues up
> message, mailbox layer sees no message is outgoing and sends it. CPU waits
> at scmi_wait_for_message_response(), timesout after 30ms
> CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues up
> message, mailbox layer sees message pending, hold message in queue. CPU
> waits at scmi_wait_for_message_response(), timesout after 30ms.
> 
> Lets say if transport takes 25ms. The first message would succeed, the
> second message would timeout after 5ms.
> 
> Hopefully this makes sense.

Yes, of course, thanks, for reporting this, and taking time to
explain...

...in general the patch LGTM...I think your patch is good also because it
could be easily backported as a fix....can you add a Fixes tag in your
next version ?

Also can you explain in more detail the issue and the solution in the commit
message....that will help having it merged as a Fix in stables...

...for the future (definitely NOT in this series) we could probably think to
get rid of the sleeping mutex in favour of some other non-sleeping form of
mutual exclusion around the channnel (like in SMC transport) and enable
(optionally) Atomic transmission support AND also review if the shmem
layer busy-waiting in txprepare is anymore needed at all...

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Sudeep Holla]

On Tue, Oct 08, 2024 at 01:10:39PM +0100, Cristian Marussi wrote:
> On Mon, Oct 07, 2024 at 10:58:47AM -0700, Justin Chen wrote:
> > Thanks for the response. I'll try to elaborate.
> > 
> > When comparing SMC and mailbox transport, we noticed mailbox transport
> > timesout much quicker when under load. Originally we thought this was the
> > latency of the mailbox implementation, but after debugging we noticed a
> > weird behavior. We saw SMCI transactions timing out before the mailbox even
> > transmitted the message.
> > 
> > This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
> > do_xfer() function.
> > 
> > The fundamental issue is send_message() blocks for SMC transport, but
> > doesn't block for mailbox transport. So if send_message() doesn't block we
> > can have multiple messages waiting at scmi_wait_for_message_response().
> > 
> 
> oh...yes...now I can see it...tx_prepare is really never called given
> how the mailbox subsystem de-queues messages once at time...so we end up
> waiting for a reply to some message that is still to be sent...so the
> message inflight is really NOT corrupted because the next remain pending
> until the reply in the shmem is read back , BUT the timeout will drift away
> if you multiple inflights are pending to be sent...
>

Indeed.

> > SMC looks like this
> > CPU #0 SCMI message 0 -> calls send_message() then calls
> > scmi_wait_for_message_response(), timesout after 30ms.
> > CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI message 0
> > to complete.
> > 
> > Mailbox looks like this
> > CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues up
> > message, mailbox layer sees no message is outgoing and sends it. CPU waits
> > at scmi_wait_for_message_response(), timesout after 30ms
> > CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues up
> > message, mailbox layer sees message pending, hold message in queue. CPU
> > waits at scmi_wait_for_message_response(), timesout after 30ms.
> > 
> > Lets say if transport takes 25ms. The first message would succeed, the
> > second message would timeout after 5ms.
> > 
> > Hopefully this makes sense.
> 
> Yes, of course, thanks, for reporting this, and taking time to
> explain...
> 
> ...in general the patch LGTM...I think your patch is good also because it
> could be easily backported as a fix....can you add a Fixes tag in your
> next version ?
>

Are you seeing this issue a lot ? IOW, do we need this to be backported ?

> Also can you explain in more detail the issue and the solution in the commit
> message....that will help having it merged as a Fix in stables...
>
> ...for the future (definitely NOT in this series) we could probably think to
> get rid of the sleeping mutex in favour of some other non-sleeping form of
> mutual exclusion around the channnel (like in SMC transport) and enable
> (optionally) Atomic transmission support AND also review if the shmem
> layer busy-waiting in txprepare is anymore needed at all...
>

Agreed, if we are locking the channel in SCMI, we can drop the busy-waiting
in tx_prepare and the associated details in the comment as this locking
voids that. It is better have both the changes in the same patch to indicate
the relation between them.

--
Regards,
Sudeep

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Sudeep Holla]

On Tue, Oct 08, 2024 at 02:23:00PM +0100, Sudeep Holla wrote:
> On Tue, Oct 08, 2024 at 01:10:39PM +0100, Cristian Marussi wrote:
> > On Mon, Oct 07, 2024 at 10:58:47AM -0700, Justin Chen wrote:
> > > Thanks for the response. I'll try to elaborate.
> > > 
> > > When comparing SMC and mailbox transport, we noticed mailbox transport
> > > timesout much quicker when under load. Originally we thought this was the
> > > latency of the mailbox implementation, but after debugging we noticed a
> > > weird behavior. We saw SMCI transactions timing out before the mailbox even
> > > transmitted the message.
> > > 
> > > This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
> > > do_xfer() function.
> > > 
> > > The fundamental issue is send_message() blocks for SMC transport, but
> > > doesn't block for mailbox transport. So if send_message() doesn't block we
> > > can have multiple messages waiting at scmi_wait_for_message_response().
> > > 
> > 
> > oh...yes...now I can see it...tx_prepare is really never called given
> > how the mailbox subsystem de-queues messages once at time...so we end up
> > waiting for a reply to some message that is still to be sent...so the
> > message inflight is really NOT corrupted because the next remain pending
> > until the reply in the shmem is read back , BUT the timeout will drift away
> > if you multiple inflights are pending to be sent...
> >
> 
> Indeed.
> 
> > > SMC looks like this
> > > CPU #0 SCMI message 0 -> calls send_message() then calls
> > > scmi_wait_for_message_response(), timesout after 30ms.
> > > CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI message 0
> > > to complete.
> > > 
> > > Mailbox looks like this
> > > CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues up
> > > message, mailbox layer sees no message is outgoing and sends it. CPU waits
> > > at scmi_wait_for_message_response(), timesout after 30ms
> > > CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues up
> > > message, mailbox layer sees message pending, hold message in queue. CPU
> > > waits at scmi_wait_for_message_response(), timesout after 30ms.
> > > 
> > > Lets say if transport takes 25ms. The first message would succeed, the
> > > second message would timeout after 5ms.
> > > 
> > > Hopefully this makes sense.
> > 
> > Yes, of course, thanks, for reporting this, and taking time to
> > explain...
> > 
> > ...in general the patch LGTM...I think your patch is good also because it
> > could be easily backported as a fix....can you add a Fixes tag in your
> > next version ?
> >
> 
> Are you seeing this issue a lot ? IOW, do we need this to be backported ?
> 
> > Also can you explain in more detail the issue and the solution in the commit
> > message....that will help having it merged as a Fix in stables...
> >
> > ...for the future (definitely NOT in this series) we could probably think to
> > get rid of the sleeping mutex in favour of some other non-sleeping form of
> > mutual exclusion around the channnel (like in SMC transport) and enable
> > (optionally) Atomic transmission support AND also review if the shmem
> > layer busy-waiting in txprepare is anymore needed at all...
> >
> 
> Agreed, if we are locking the channel in SCMI, we can drop the busy-waiting
> in tx_prepare and the associated details in the comment as this locking
> voids that. It is better have both the changes in the same patch to indicate
> the relation between them.

Actually scratch that last point. The waiting in tx_prepare until the platform
marks it free for agent to use is still needed. One usecase is when agent/OS
times out but platform continues to process and eventually releases the shmem.
Sorry I completely forgot about that.

-- 
Regards,
Sudeep

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Cristian Marussi]

On Tue, Oct 08, 2024 at 02:34:59PM +0100, Sudeep Holla wrote:
> On Tue, Oct 08, 2024 at 02:23:00PM +0100, Sudeep Holla wrote:
> > On Tue, Oct 08, 2024 at 01:10:39PM +0100, Cristian Marussi wrote:
> > > On Mon, Oct 07, 2024 at 10:58:47AM -0700, Justin Chen wrote:
> > > > Thanks for the response. I'll try to elaborate.
> > > > 
> > > > When comparing SMC and mailbox transport, we noticed mailbox transport
> > > > timesout much quicker when under load. Originally we thought this was the
> > > > latency of the mailbox implementation, but after debugging we noticed a
> > > > weird behavior. We saw SMCI transactions timing out before the mailbox even
> > > > transmitted the message.
> > > > 
> > > > This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
> > > > do_xfer() function.
> > > > 
> > > > The fundamental issue is send_message() blocks for SMC transport, but
> > > > doesn't block for mailbox transport. So if send_message() doesn't block we
> > > > can have multiple messages waiting at scmi_wait_for_message_response().
> > > > 
> > > 
> > > oh...yes...now I can see it...tx_prepare is really never called given
> > > how the mailbox subsystem de-queues messages once at time...so we end up
> > > waiting for a reply to some message that is still to be sent...so the
> > > message inflight is really NOT corrupted because the next remain pending
> > > until the reply in the shmem is read back , BUT the timeout will drift away
> > > if you multiple inflights are pending to be sent...
> > >
> > 
> > Indeed.
> > 
> > > > SMC looks like this
> > > > CPU #0 SCMI message 0 -> calls send_message() then calls
> > > > scmi_wait_for_message_response(), timesout after 30ms.
> > > > CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI message 0
> > > > to complete.
> > > > 
> > > > Mailbox looks like this
> > > > CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues up
> > > > message, mailbox layer sees no message is outgoing and sends it. CPU waits
> > > > at scmi_wait_for_message_response(), timesout after 30ms
> > > > CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues up
> > > > message, mailbox layer sees message pending, hold message in queue. CPU
> > > > waits at scmi_wait_for_message_response(), timesout after 30ms.
> > > > 
> > > > Lets say if transport takes 25ms. The first message would succeed, the
> > > > second message would timeout after 5ms.
> > > > 
> > > > Hopefully this makes sense.
> > > 
> > > Yes, of course, thanks, for reporting this, and taking time to
> > > explain...
> > > 
> > > ...in general the patch LGTM...I think your patch is good also because it
> > > could be easily backported as a fix....can you add a Fixes tag in your
> > > next version ?
> > >
> > 
> > Are you seeing this issue a lot ? IOW, do we need this to be backported ?
> > 
> > > Also can you explain in more detail the issue and the solution in the commit
> > > message....that will help having it merged as a Fix in stables...
> > >
> > > ...for the future (definitely NOT in this series) we could probably think to
> > > get rid of the sleeping mutex in favour of some other non-sleeping form of
> > > mutual exclusion around the channnel (like in SMC transport) and enable
> > > (optionally) Atomic transmission support AND also review if the shmem
> > > layer busy-waiting in txprepare is anymore needed at all...
> > >
> > 
> > Agreed, if we are locking the channel in SCMI, we can drop the busy-waiting
> > in tx_prepare and the associated details in the comment as this locking
> > voids that. It is better have both the changes in the same patch to indicate
> > the relation between them.
> 
> Actually scratch that last point. The waiting in tx_prepare until the platform
> marks it free for agent to use is still needed. One usecase is when agent/OS
> times out but platform continues to process and eventually releases the shmem.
> Sorry I completely forgot about that.
> 

Yes indeed it is the mechanism that we avoid to reclaim forcibly anyway the shmem
if the transmission times out...and we should keep that to avoid
corruption of newer messages by late replies from the earlier ones that
have timed out.

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Justin Chen]

On 10/8/24 6:37 AM, Cristian Marussi wrote:
> On Tue, Oct 08, 2024 at 02:34:59PM +0100, Sudeep Holla wrote:
>> On Tue, Oct 08, 2024 at 02:23:00PM +0100, Sudeep Holla wrote:
>>> On Tue, Oct 08, 2024 at 01:10:39PM +0100, Cristian Marussi wrote:
>>>> On Mon, Oct 07, 2024 at 10:58:47AM -0700, Justin Chen wrote:
>>>>> Thanks for the response. I'll try to elaborate.
>>>>>
>>>>> When comparing SMC and mailbox transport, we noticed mailbox transport
>>>>> timesout much quicker when under load. Originally we thought this was the
>>>>> latency of the mailbox implementation, but after debugging we noticed a
>>>>> weird behavior. We saw SMCI transactions timing out before the mailbox even
>>>>> transmitted the message.
>>>>>
>>>>> This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
>>>>> do_xfer() function.
>>>>>
>>>>> The fundamental issue is send_message() blocks for SMC transport, but
>>>>> doesn't block for mailbox transport. So if send_message() doesn't block we
>>>>> can have multiple messages waiting at scmi_wait_for_message_response().
>>>>>
>>>>
>>>> oh...yes...now I can see it...tx_prepare is really never called given
>>>> how the mailbox subsystem de-queues messages once at time...so we end up
>>>> waiting for a reply to some message that is still to be sent...so the
>>>> message inflight is really NOT corrupted because the next remain pending
>>>> until the reply in the shmem is read back , BUT the timeout will drift away
>>>> if you multiple inflights are pending to be sent...
>>>>
>>>
>>> Indeed.
>>>
>>>>> SMC looks like this
>>>>> CPU #0 SCMI message 0 -> calls send_message() then calls
>>>>> scmi_wait_for_message_response(), timesout after 30ms.
>>>>> CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI message 0
>>>>> to complete.
>>>>>
>>>>> Mailbox looks like this
>>>>> CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues up
>>>>> message, mailbox layer sees no message is outgoing and sends it. CPU waits
>>>>> at scmi_wait_for_message_response(), timesout after 30ms
>>>>> CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues up
>>>>> message, mailbox layer sees message pending, hold message in queue. CPU
>>>>> waits at scmi_wait_for_message_response(), timesout after 30ms.
>>>>>
>>>>> Lets say if transport takes 25ms. The first message would succeed, the
>>>>> second message would timeout after 5ms.
>>>>>
>>>>> Hopefully this makes sense.
>>>>
>>>> Yes, of course, thanks, for reporting this, and taking time to
>>>> explain...
>>>>
>>>> ...in general the patch LGTM...I think your patch is good also because it
>>>> could be easily backported as a fix....can you add a Fixes tag in your
>>>> next version ?
>>>>
>>>
>>> Are you seeing this issue a lot ? IOW, do we need this to be backported ?
>>>

I wouldn't say a lot. But we are seeing it with standard use of our 
devices running over an extended amount of time. Yes we would like this 
backported.

>>>> Also can you explain in more detail the issue and the solution in the commit
>>>> message....that will help having it merged as a Fix in stables...
>>>>
>>>> ...for the future (definitely NOT in this series) we could probably think to
>>>> get rid of the sleeping mutex in favour of some other non-sleeping form of
>>>> mutual exclusion around the channnel (like in SMC transport) and enable
>>>> (optionally) Atomic transmission support AND also review if the shmem
>>>> layer busy-waiting in txprepare is anymore needed at all...
>>>>
>>>
>>> Agreed, if we are locking the channel in SCMI, we can drop the busy-waiting
>>> in tx_prepare and the associated details in the comment as this locking
>>> voids that. It is better have both the changes in the same patch to indicate
>>> the relation between them.
>>
>> Actually scratch that last point. The waiting in tx_prepare until the platform
>> marks it free for agent to use is still needed. One usecase is when agent/OS
>> times out but platform continues to process and eventually releases the shmem.
>> Sorry I completely forgot about that.
>>
> 
> Yes indeed it is the mechanism that we avoid to reclaim forcibly anyway the shmem
> if the transmission times out...and we should keep that to avoid
> corruption of newer messages by late replies from the earlier ones that
> have timed out.
> 

Yup. I saw an interesting interaction from this. Since modifying shmem 
and ringing the doorbell are often two different task. The modification 
of shmem can race with processing of timed out messages from the 
platform. This usually leads to an early ACK and spurious interrupt. 
Mostly harmless. We did see lockups when multiple timeouts occur, but it 
was unclear if this was an issue with the SCMI transport layer or our 
driver/platform.

Thanks,
Justin

> Thanks,
> Cristian

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Justin Chen]

On 10/8/24 12:23 PM, Justin Chen wrote:
> 
> 
> On 10/8/24 6:37 AM, Cristian Marussi wrote:
>> On Tue, Oct 08, 2024 at 02:34:59PM +0100, Sudeep Holla wrote:
>>> On Tue, Oct 08, 2024 at 02:23:00PM +0100, Sudeep Holla wrote:
>>>> On Tue, Oct 08, 2024 at 01:10:39PM +0100, Cristian Marussi wrote:
>>>>> On Mon, Oct 07, 2024 at 10:58:47AM -0700, Justin Chen wrote:
>>>>>> Thanks for the response. I'll try to elaborate.
>>>>>>
>>>>>> When comparing SMC and mailbox transport, we noticed mailbox 
>>>>>> transport
>>>>>> timesout much quicker when under load. Originally we thought this 
>>>>>> was the
>>>>>> latency of the mailbox implementation, but after debugging we 
>>>>>> noticed a
>>>>>> weird behavior. We saw SMCI transactions timing out before the 
>>>>>> mailbox even
>>>>>> transmitted the message.
>>>>>>
>>>>>> This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
>>>>>> do_xfer() function.
>>>>>>
>>>>>> The fundamental issue is send_message() blocks for SMC transport, but
>>>>>> doesn't block for mailbox transport. So if send_message() doesn't 
>>>>>> block we
>>>>>> can have multiple messages waiting at 
>>>>>> scmi_wait_for_message_response().
>>>>>>
>>>>>
>>>>> oh...yes...now I can see it...tx_prepare is really never called given
>>>>> how the mailbox subsystem de-queues messages once at time...so we 
>>>>> end up
>>>>> waiting for a reply to some message that is still to be sent...so the
>>>>> message inflight is really NOT corrupted because the next remain 
>>>>> pending
>>>>> until the reply in the shmem is read back , BUT the timeout will 
>>>>> drift away
>>>>> if you multiple inflights are pending to be sent...
>>>>>
>>>>
>>>> Indeed.
>>>>
>>>>>> SMC looks like this
>>>>>> CPU #0 SCMI message 0 -> calls send_message() then calls
>>>>>> scmi_wait_for_message_response(), timesout after 30ms.
>>>>>> CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI 
>>>>>> message 0
>>>>>> to complete.
>>>>>>
>>>>>> Mailbox looks like this
>>>>>> CPU #0 SCMI message 0 -> calls send_message(), mailbox layer 
>>>>>> queues up
>>>>>> message, mailbox layer sees no message is outgoing and sends it. 
>>>>>> CPU waits
>>>>>> at scmi_wait_for_message_response(), timesout after 30ms
>>>>>> CPU #1 SCMI message 1 -> calls send_message(), mailbox layer 
>>>>>> queues up
>>>>>> message, mailbox layer sees message pending, hold message in 
>>>>>> queue. CPU
>>>>>> waits at scmi_wait_for_message_response(), timesout after 30ms.
>>>>>>
>>>>>> Lets say if transport takes 25ms. The first message would succeed, 
>>>>>> the
>>>>>> second message would timeout after 5ms.
>>>>>>
>>>>>> Hopefully this makes sense.
>>>>>
>>>>> Yes, of course, thanks, for reporting this, and taking time to
>>>>> explain...
>>>>>
>>>>> ...in general the patch LGTM...I think your patch is good also 
>>>>> because it
>>>>> could be easily backported as a fix....can you add a Fixes tag in your
>>>>> next version ?
>>>>>
>>>>
>>>> Are you seeing this issue a lot ? IOW, do we need this to be 
>>>> backported ?
>>>>
> 
> I wouldn't say a lot. But we are seeing it with standard use of our 
> devices running over an extended amount of time. Yes we would like this 
> backported.
> 

Any advice on what Fixes tag to add here? Since the driver was moved to 
the transport folder, I assume this is going to be a manual backport. 
Not sure if the Fixes tag should be when the SCMI MBOX transport was 
moved to a standalone driver in the transport folder or the very first 
SCMI commits since the MBOX transport was added then.

Thanks,
Justin

<snip>

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Cristian Marussi]

On Tue, Oct 08, 2024 at 12:23:28PM -0700, Justin Chen wrote:
> 
> 
> On 10/8/24 6:37 AM, Cristian Marussi wrote:
> > On Tue, Oct 08, 2024 at 02:34:59PM +0100, Sudeep Holla wrote:
> > > On Tue, Oct 08, 2024 at 02:23:00PM +0100, Sudeep Holla wrote:
> > > > On Tue, Oct 08, 2024 at 01:10:39PM +0100, Cristian Marussi wrote:
> > > > > On Mon, Oct 07, 2024 at 10:58:47AM -0700, Justin Chen wrote:
> > > > > > Thanks for the response. I'll try to elaborate.
> > > > > > 
> > > > > > When comparing SMC and mailbox transport, we noticed mailbox transport
> > > > > > timesout much quicker when under load. Originally we thought this was the
> > > > > > latency of the mailbox implementation, but after debugging we noticed a
> > > > > > weird behavior. We saw SMCI transactions timing out before the mailbox even
> > > > > > transmitted the message.
> > > > > > 
> > > > > > This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
> > > > > > do_xfer() function.
> > > > > > 
> > > > > > The fundamental issue is send_message() blocks for SMC transport, but
> > > > > > doesn't block for mailbox transport. So if send_message() doesn't block we
> > > > > > can have multiple messages waiting at scmi_wait_for_message_response().
> > > > > > 
> > > > > 
> > > > > oh...yes...now I can see it...tx_prepare is really never called given
> > > > > how the mailbox subsystem de-queues messages once at time...so we end up
> > > > > waiting for a reply to some message that is still to be sent...so the
> > > > > message inflight is really NOT corrupted because the next remain pending
> > > > > until the reply in the shmem is read back , BUT the timeout will drift away
> > > > > if you multiple inflights are pending to be sent...
> > > > > 
> > > > 
> > > > Indeed.
> > > > 
> > > > > > SMC looks like this
> > > > > > CPU #0 SCMI message 0 -> calls send_message() then calls
> > > > > > scmi_wait_for_message_response(), timesout after 30ms.
> > > > > > CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI message 0
> > > > > > to complete.
> > > > > > 
> > > > > > Mailbox looks like this
> > > > > > CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues up
> > > > > > message, mailbox layer sees no message is outgoing and sends it. CPU waits
> > > > > > at scmi_wait_for_message_response(), timesout after 30ms
> > > > > > CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues up
> > > > > > message, mailbox layer sees message pending, hold message in queue. CPU
> > > > > > waits at scmi_wait_for_message_response(), timesout after 30ms.
> > > > > > 
> > > > > > Lets say if transport takes 25ms. The first message would succeed, the
> > > > > > second message would timeout after 5ms.
> > > > > > 
> > > > > > Hopefully this makes sense.
> > > > > 
> > > > > Yes, of course, thanks, for reporting this, and taking time to
> > > > > explain...
> > > > > 
> > > > > ...in general the patch LGTM...I think your patch is good also because it
> > > > > could be easily backported as a fix....can you add a Fixes tag in your
> > > > > next version ?
> > > > > 
> > > > 
> > > > Are you seeing this issue a lot ? IOW, do we need this to be backported ?
> > > > 
> 
> I wouldn't say a lot. But we are seeing it with standard use of our devices
> running over an extended amount of time. Yes we would like this backported.
> 
> > > > > Also can you explain in more detail the issue and the solution in the commit
> > > > > message....that will help having it merged as a Fix in stables...
> > > > > 
> > > > > ...for the future (definitely NOT in this series) we could probably think to
> > > > > get rid of the sleeping mutex in favour of some other non-sleeping form of
> > > > > mutual exclusion around the channnel (like in SMC transport) and enable
> > > > > (optionally) Atomic transmission support AND also review if the shmem
> > > > > layer busy-waiting in txprepare is anymore needed at all...
> > > > > 
> > > > 
> > > > Agreed, if we are locking the channel in SCMI, we can drop the busy-waiting
> > > > in tx_prepare and the associated details in the comment as this locking
> > > > voids that. It is better have both the changes in the same patch to indicate
> > > > the relation between them.
> > > 
> > > Actually scratch that last point. The waiting in tx_prepare until the platform
> > > marks it free for agent to use is still needed. One usecase is when agent/OS
> > > times out but platform continues to process and eventually releases the shmem.
> > > Sorry I completely forgot about that.
> > > 
> > 
> > Yes indeed it is the mechanism that we avoid to reclaim forcibly anyway the shmem
> > if the transmission times out...and we should keep that to avoid
> > corruption of newer messages by late replies from the earlier ones that
> > have timed out.
> > 
> 
> Yup. I saw an interesting interaction from this. Since modifying shmem and
> ringing the doorbell are often two different task. The modification of shmem
> can race with processing of timed out messages from the platform. This
> usually leads to an early ACK and spurious interrupt. Mostly harmless. We
> did see lockups when multiple timeouts occur, but it was unclear if this was
> an issue with the SCMI transport layer or our driver/platform.
> 

Are you talking about something similar to this:

https://lore.kernel.org/all/20231220172112.763539-1-cristian.marussi@arm.com/

... reported as a side effect of a spurious IRQ on late timed-out
replies, it should have been fixed with the above commit in v6.8.

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Justin Chen]

On 10/9/24 1:32 AM, Cristian Marussi wrote:
> On Tue, Oct 08, 2024 at 12:23:28PM -0700, Justin Chen wrote:
>>
>>
>> On 10/8/24 6:37 AM, Cristian Marussi wrote:
>>> On Tue, Oct 08, 2024 at 02:34:59PM +0100, Sudeep Holla wrote:
>>>> On Tue, Oct 08, 2024 at 02:23:00PM +0100, Sudeep Holla wrote:
>>>>> On Tue, Oct 08, 2024 at 01:10:39PM +0100, Cristian Marussi wrote:
>>>>>> On Mon, Oct 07, 2024 at 10:58:47AM -0700, Justin Chen wrote:
>>>>>>> Thanks for the response. I'll try to elaborate.
>>>>>>>
>>>>>>> When comparing SMC and mailbox transport, we noticed mailbox transport
>>>>>>> timesout much quicker when under load. Originally we thought this was the
>>>>>>> latency of the mailbox implementation, but after debugging we noticed a
>>>>>>> weird behavior. We saw SMCI transactions timing out before the mailbox even
>>>>>>> transmitted the message.
>>>>>>>
>>>>>>> This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
>>>>>>> do_xfer() function.
>>>>>>>
>>>>>>> The fundamental issue is send_message() blocks for SMC transport, but
>>>>>>> doesn't block for mailbox transport. So if send_message() doesn't block we
>>>>>>> can have multiple messages waiting at scmi_wait_for_message_response().
>>>>>>>
>>>>>>
>>>>>> oh...yes...now I can see it...tx_prepare is really never called given
>>>>>> how the mailbox subsystem de-queues messages once at time...so we end up
>>>>>> waiting for a reply to some message that is still to be sent...so the
>>>>>> message inflight is really NOT corrupted because the next remain pending
>>>>>> until the reply in the shmem is read back , BUT the timeout will drift away
>>>>>> if you multiple inflights are pending to be sent...
>>>>>>
>>>>>
>>>>> Indeed.
>>>>>
>>>>>>> SMC looks like this
>>>>>>> CPU #0 SCMI message 0 -> calls send_message() then calls
>>>>>>> scmi_wait_for_message_response(), timesout after 30ms.
>>>>>>> CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI message 0
>>>>>>> to complete.
>>>>>>>
>>>>>>> Mailbox looks like this
>>>>>>> CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues up
>>>>>>> message, mailbox layer sees no message is outgoing and sends it. CPU waits
>>>>>>> at scmi_wait_for_message_response(), timesout after 30ms
>>>>>>> CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues up
>>>>>>> message, mailbox layer sees message pending, hold message in queue. CPU
>>>>>>> waits at scmi_wait_for_message_response(), timesout after 30ms.
>>>>>>>
>>>>>>> Lets say if transport takes 25ms. The first message would succeed, the
>>>>>>> second message would timeout after 5ms.
>>>>>>>
>>>>>>> Hopefully this makes sense.
>>>>>>
>>>>>> Yes, of course, thanks, for reporting this, and taking time to
>>>>>> explain...
>>>>>>
>>>>>> ...in general the patch LGTM...I think your patch is good also because it
>>>>>> could be easily backported as a fix....can you add a Fixes tag in your
>>>>>> next version ?
>>>>>>
>>>>>
>>>>> Are you seeing this issue a lot ? IOW, do we need this to be backported ?
>>>>>
>>
>> I wouldn't say a lot. But we are seeing it with standard use of our devices
>> running over an extended amount of time. Yes we would like this backported.
>>
>>>>>> Also can you explain in more detail the issue and the solution in the commit
>>>>>> message....that will help having it merged as a Fix in stables...
>>>>>>
>>>>>> ...for the future (definitely NOT in this series) we could probably think to
>>>>>> get rid of the sleeping mutex in favour of some other non-sleeping form of
>>>>>> mutual exclusion around the channnel (like in SMC transport) and enable
>>>>>> (optionally) Atomic transmission support AND also review if the shmem
>>>>>> layer busy-waiting in txprepare is anymore needed at all...
>>>>>>
>>>>>
>>>>> Agreed, if we are locking the channel in SCMI, we can drop the busy-waiting
>>>>> in tx_prepare and the associated details in the comment as this locking
>>>>> voids that. It is better have both the changes in the same patch to indicate
>>>>> the relation between them.
>>>>
>>>> Actually scratch that last point. The waiting in tx_prepare until the platform
>>>> marks it free for agent to use is still needed. One usecase is when agent/OS
>>>> times out but platform continues to process and eventually releases the shmem.
>>>> Sorry I completely forgot about that.
>>>>
>>>
>>> Yes indeed it is the mechanism that we avoid to reclaim forcibly anyway the shmem
>>> if the transmission times out...and we should keep that to avoid
>>> corruption of newer messages by late replies from the earlier ones that
>>> have timed out.
>>>
>>
>> Yup. I saw an interesting interaction from this. Since modifying shmem and
>> ringing the doorbell are often two different task. The modification of shmem
>> can race with processing of timed out messages from the platform. This
>> usually leads to an early ACK and spurious interrupt. Mostly harmless. We
>> did see lockups when multiple timeouts occur, but it was unclear if this was
>> an issue with the SCMI transport layer or our driver/platform.
>>
> 
> Are you talking about something similar to this:
> 
> https://lore.kernel.org/all/20231220172112.763539-1-cristian.marussi@arm.com/
> 
> ... reported as a side effect of a spurious IRQ on late timed-out
> replies, it should have been fixed with the above commit in v6.8.
> 

I had these fixes and saw these warning message. With these locking 
changes, I no longer see the lock up. Guess they were related somehow. 
Either way, Thanks for the review. v2 incoming.

Justin

> Thanks,
> Cristian

Re: [PATCH] firmware: arm_scmi: Queue in scmi layer for mailbox implementation

[Sudeep Holla]

On Mon, Oct 07, 2024 at 10:58:47AM -0700, Justin Chen wrote:
> 
> 
> On 10/7/24 6:10 AM, Cristian Marussi wrote:
> > On Mon, Oct 07, 2024 at 02:04:10PM +0100, Sudeep Holla wrote:
> > > On Fri, Oct 04, 2024 at 03:12:57PM -0700, Justin Chen wrote:
> > > > The mailbox layer has its own queue. However this confuses the per
> > > > message timeouts since the clock starts ticking the moment the messages
> > > > get queued up. So all messages in the queue have there timeout clocks
> > > > ticking instead of only the message inflight. To fix this, lets move the
> > > > queue back into the SCMI layer.
> > > > 
> > > 
> > > I think this has come up in the past. We have avoided adding addition
> > > locking here as the mailbox layer takes care of it. Has anything changed
> > > recently ?
> > 
> > I asked for an explanation in my reply (we crossed each other mails probably)
> > since it alredy came up in the past a few times and central locking seemed not
> > to be needed...here the difference is about the reason...Justin talks about
> > message timeouts related to the queueing process..so I asked to better
> > explain the detail (and the anbomaly observed) since it still does not
> > seem to me that even in this case the lock is needed....anyway I can
> > definitely be woring of course :D
> > 
> 
> Hello Cristian,
> 
> Thanks for the response. I'll try to elaborate.
> 
> When comparing SMC and mailbox transport, we noticed mailbox transport
> timesout much quicker when under load. Originally we thought this was the
> latency of the mailbox implementation, but after debugging we noticed a
> weird behavior. We saw SMCI transactions timing out before the mailbox even
> transmitted the message.
> 
> This issue lies in the SCMI layer. drivers/firmware/arm_scmi/driver.c
> do_xfer() function.
> 
> The fundamental issue is send_message() blocks for SMC transport, but
> doesn't block for mailbox transport. So if send_message() doesn't block we
> can have multiple messages waiting at scmi_wait_for_message_response().
> 
> SMC looks like this
> CPU #0 SCMI message 0 -> calls send_message() then calls
> scmi_wait_for_message_response(), timesout after 30ms.
> CPU #1 SCMI message 1 -> blocks at send_message() waiting for SCMI message 0
> to complete.
> 
> Mailbox looks like this
> CPU #0 SCMI message 0 -> calls send_message(), mailbox layer queues up
> message, mailbox layer sees no message is outgoing and sends it. CPU waits
> at scmi_wait_for_message_response(), timesout after 30ms
> CPU #1 SCMI message 1 -> calls send_message(), mailbox layer queues up
> message, mailbox layer sees message pending, hold message in queue. CPU
> waits at scmi_wait_for_message_response(), timesout after 30ms.
> 
> Lets say if transport takes 25ms. The first message would succeed, the
> second message would timeout after 5ms.
>

I understand this issue and was aware of this. Just had assumed it won't
have such a impact as I was assuming transport to take couple of ms for
any synchronous commands.

Typically I would expect transport to take much less that 25ms(1-4ms IMO)
to complete any synchronous commands. If it takes 25ms for sync command,
then it could be some serious issue.

Anyways I am not against the idea. More details in response to Cristian.

-- 
Regards,
Sudeep