Re: [PATCH] firmware: arm_scmi: Add lock for mailbox do_xfer

Re: [PATCH] firmware: arm_scmi: Add lock for mailbox do_xfer

[Sudeep Holla]

On Thu, Oct 19, 2023 at 02:05:10AM +0000, Xinglong Yang wrote:
>
> This email (including its attachments) is intended only for the person or
> entity to which it is addressed and may contain information that is
> privileged, confidential or otherwise protected from disclosure.
> Unauthorized use, dissemination, distribution or copying of this email or
> the information herein or taking any action in reliance on the contents of
> this email or the information herein, by anyone other than the intended
> recipient, or an employee or agent responsible for delivering the message to
> the intended recipient, is strictly prohibited. If you are not the intended
> recipient, please do not read, copy, use or disclose any part of this e-mail
> to others. Please notify the sender immediately and permanently delete this
> e-mail and any attachments if you received it in error. Internet
> communications cannot be guaranteed to be timely, secure, error-free or
> virus-free. The sender does not accept liability for any errors or
> omissions.

I will deleting the email as suggested. Please post without this disclaimer
to get any comments or review of the actual patch.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Add lock for mailbox do_xfer

[Cristian Marussi]

On Sun, Oct 22, 2023 at 08:15:17PM +0800, sean yang wrote:
> 
>    The function do_xfer may cause multithreading send message at almost
>    the same time.
> 
>    send_message will write the message in the shared memory, This causes
>    the second write
> 
>    to overwrite the first write, resulting in the loss of the first write.
> 
>    Add lock to avoid this situation. Release lock when the message has
>    responded.
> 
> 

Hi Xinglong ,

In the current SCMI stack, channels are protected from issues of
concurrency at the transport layer, not here in the core, because each
transport has different requirements in these regards.

As an example, VirtIO SCMI transport can indeed support multiple
outgoing messages in-flight if needed, so placing a global mutex here
around the send_message will kill this possibility. (and in this case
will serialize all because the muetx is not even per-channel but global)

Each SCMI transport has its own locking placed where it is deemed
needed, just look at smc or optee in send_message()/ mark_txdone().

Regarding mailboxes, which is probably the case you are referring to,
the shmem_tx_prepare accessors contain some spin_until_cond() to protect
the shared mem, but the core of the mutual exclusion around the channel
is done really by the mailbox framework, since we register the scmi mbox
client with 'knows_txdone = true' and then we use mbox_send_message() and
mbox_client_txdone() to enqueue messages to the mailbox framework: in a
nutshell, no message transmission will be attempted if the previous one
has not completed by calling mbox_client_txdone().

I've never seen any anomalies on this. Did you notice any issue with
mailboxes ? What is your setup ?

Thanks,
Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Add lock for mailbox do_xfer

[Sudeep Holla]

Hi Xinglong,

Cristian has already given reason why this change is wrong.

I wanted to let you know that you need to still fix your email.

On Sun, Oct 22, 2023 at 08:15:17PM +0800, sean yang wrote:
> The function do_xfer may cause multithreading send message at almost the
> same time.
>
> send_message will write the message in the shared memory, This causes the
> second write
>
> to overwrite the first write, resulting in the loss of the first write.
>
> Add lock to avoid this situation. Release lock when the message has
> responded.
>

This looks like either copy-pasted or some wrong format(html?).
Use git send-email if you are not already to send any patch on the list.
This is definitely wrong format and no one can apply this patch.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel