Hi Mathieu,
On 9/17/2024 2:07 PM, Mathieu Poirier wrote:
On Mon, 16 Sept 2024 at 23:20, Kumar, Udit <u-kumar1@xxxxxx> wrote:
On 9/16/2024 8:50 PM, Mathieu Poirier wrote:
On Mon, 16 Sept 2024 at 02:31, Siddharth Vadapalli <s-vadapalli@xxxxxx> wrote:
Commit f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during
probe routine") introduced a check in the "k3_r5_rproc_mbox_callback()" and
"k3_r5_rproc_kick()" callbacks, causing them to exit if the remote core's
state is "RPROC_DETACHED". However, the "__rproc_attach()" function that is
responsible for attaching to a remote core, updates the state of the remote
core to "RPROC_ATTACHED" only after invoking "rproc_start_subdevices()".
The "rproc_start_subdevices()" function triggers the probe of the Virtio
RPMsg devices associated with the remote core, which require that the
"k3_r5_rproc_kick()" and "k3_r5_rproc_mbox_callback()" callbacks are
functional. Hence, drop the check in the callbacks.
Honestly, I am very tempted to just revert f3f11cfe8907 and ea1d6fb5b571.
Please don't :) , it will break rproc in general for k3 devices.
Why not - it is already broken anyway. Reverting the patches will
force TI to actually think about the feature in terms of design,
completeness and testing. The merge window opened on Sunday - I'm not
going to merge whack-a-mole patches and hope the right fix comes
along.
Now, I am not advocating here to revert or not.
But where we stand currently
1- Without this patch, IPC is broken in general.
2- With this patch, IPC is conditionally broken.
In either case, we need to fix it.
your call to revert or keep it.
Couple of solutions for this race around condition (in mine preference
order)
This is for the TI team to discuss _and_ test thoroughly. From hereon
and until I see things improve, all patches from TI will need to be
tagged with R-B and T-B tags (collected on the mailing lists) from two
different individuals before I look at them.
Sure we will take care of above
and fair ask on R-B and T-B tags
1) In
https://elixir.bootlin.com/linux/v6.11/source/drivers/remoteproc/ti_k3_r5_remoteproc.c#L190
have a check , if probe in is progress or not
2)
https://elixir.bootlin.com/linux/v6.11/source/drivers/remoteproc/ti_k3_r5_remoteproc.c#L1205
-- correct the state to ON or something else
3) Move condition
https://elixir.bootlin.com/linux/v6.11/source/drivers/remoteproc/remoteproc_core.c#L1360
before rproc_start_subdevices
<https://elixir.bootlin.com/linux/v6.11/C/ident/rproc_start_subdevices>
calling
Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle during probe routine")
Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
---
Hello,
Since the commit being fixed is not yet a part of Mainline Linux, this
patch is based on linux-next tagged next-20240913.
An alternative to this patch will be a change to the "__rproc_attach()"
function in the "remoteproc_core.c" driver with
rproc->state = RPROC_ATTACHED;
being set after "rproc_attach_device()" is invoked, but __before__
invoking "rproc_start_subdevices()". Since this change will be performed
in the common Remoteproc Core, it appeared to me that fixing it in the
TI remoteproc driver is the correct approach.
The equivalent of this patch for ti_k3_dsp_remoteproc.c might also be
required, which I shall post if the current patch is acceptable.
Kindly review and share your feedback on this patch.
Regards,
Siddharth.
drivers/remoteproc/ti_k3_r5_remoteproc.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 747ee467da88..4894461aa65f 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -194,10 +194,6 @@ static void k3_r5_rproc_mbox_callback(struct mbox_client *client, void *data)
const char *name = kproc->rproc->name;
u32 msg = omap_mbox_message(data);
- /* Do not forward message from a detached core */
- if (kproc->rproc->state == RPROC_DETACHED)
- return;
-
dev_dbg(dev, "mbox msg: 0x%x\n", msg);
switch (msg) {
@@ -233,10 +229,6 @@ static void k3_r5_rproc_kick(struct rproc *rproc, int vqid)
mbox_msg_t msg = (mbox_msg_t)vqid;
int ret;
- /* Do not forward message to a detached core */
- if (kproc->rproc->state == RPROC_DETACHED)
- return;
-
/* send the index of the triggered virtqueue in the mailbox payload */
ret = mbox_send_message(kproc->mbox, (void *)msg);
if (ret < 0)
--
2.40.1
