Re: [PATCH v3] firmware: arm_scmi: Free mailbox channels if probe fails

On Mon, Nov 01, 2021 at 09:35:42AM -0700, rishabhb@codeaurora.org wrote:

On 2021-09-01 02:35, Cristian Marussi wrote:

quoted

On Tue, Aug 31, 2021 at 06:48:35AM +0100, Cristian Marussi wrote:

quoted

On Mon, Aug 30, 2021 at 02:09:37PM -0700, rishabhb@codeaurora.org
wrote:

quoted

Hi Christian

Hi Rishabh,

thanks for looking into this kind of bad interactions.

quoted

There seems to be another issue here. The response from agent can be delayed
causing a timeout during base protocol acquire,
which leads to the probe failure. What I have observed is sometimes the
failure of probe and rx_callback (due to a delayed message)
happens at the same time on different cpus.
Because of this race, the device memory may be cleared while the
interrupt(rx_callback) is executing on another cpu.

You are right that concurrency was not handled properly in this kind
of
context and moreover, if you think about it, even the case of out of
order reception of responses and delayed_responses (type2 SCMI
messages)
for asynchronous SCMI commands was not handled properly.

quoted

How do you propose we solve this? Do you think it is better to take the
setting up of base and other protocols out of probe and
in some delayed work? That would imply the device memory is not released
until remove is called. Or should we add locking to
the interrupt handler(scmi_rx_callback) and the cleanup in probe to avoid
the race?

These issues were more easily exposed by SCMI Virtio transport, so in
the series where I introduced scmi-virtio:

https://lore.kernel.org/linux-arm-kernel/162848483974.232214.9506203742448269364.b4-ty@arm.com/ (local)

(which is now queued for v5.15 ...  now on -next I think...finger
crossed)

I took the chance to rectify a couple of other things in the SCMI core
in the initial commits.
As an example, in the above series

 [PATCH v7 05/15] firmware: arm_scmi: Handle concurrent and
out-of-order messages

cares to add a refcount to xfers and some locking on xfers between TX
and RX path to avoid that a timed out xfer can vanish while the rx
path
is concurrently working on it (as you said); moreover I handle the
condition (rare if not unplausible anyway) in which a transport
delivers
out of order responses and delayed responses.

I tested this scenarios on some fake emulated SCMI Virtio transport
where I could play any sort of mess and tricks to stress this limit
conditions, but you're more than welcome to verify if the race you are
seeing on Base protocol time out is solved (as I would hope :D) by
this
series of mine.

Let me know, any feedback is welcome.

Btw, in the series above there are also other minor changes, but there
is also another more radical change needed to ensure correctness and
protection against stale old messages which maybe could interest you
in general if you are looking into SCMI:

[PATCH v7 04/15] firmware: arm_scmi: Introduce monotonically
increasing tokens

Let me know if yo have other concerns.

Hi Rishabhb,

just a quick remark, thinking again about your fail @probe scenario
above
I realized that while the concurrency patch I mentioned above could help
on
races against vanishing xfers when late timed-out responses are
delivered,
here we really are then also shutting down everything on failure, so
there
could be further issues between a very late invokation of
scmi_rx_callback
and the core devm_ helpers freeing the underlying xfer/cinfo/etc..
structs
used by scmi-rx-callback itself (maybe this was already what you meant
and
I didn't get it,...sorry)

On the other side, I don't feel that delaying Base init to a deferred
worker is a viable solution since we need Base protocol init to be
initialized and we need to just give up if we cannot communicate with
the SCMI platform fw in such early stages. (Base protocol is really the
only mandatory proto is I remember correctly the spec)

Currenly I'm off and only glancing at mails but I'll have a thought
about
these issues once back in a few weeks time.

Thanks,
Cristian

Hi Cristian
I hope you enjoyed your vacation. Did you get a chance to look at the issue
stated above and have some idea as to how to solve this?

Do you still see the issue with v5.15 ? Can you please check if haven't
already done that ?

Also 30ms delay we have is huge IMO and we typically expect the communication
with remote processor or any entity that implements SCMI to happen in terms
of one or few ms tops.

If there is a race, we need to fix that but I am interested in knowing
why the default time of 30ms not sufficient ? Did increasing that helps
and is this timeout happening only for the initial commands(guessing the
SCMI firmware is not yet ready) or does it happen even during run-time ?

-- 
Regards,
Sudeep

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