From: Beleswar Prasad Padhi <b-padhi@ti.com>
To: Andrew Davis <afd@ti.com>, <andersson@kernel.org>,
<mathieu.poirier@linaro.org>
Cc: <hnagalla@ti.com>, <u-kumar1@ti.com>, <s-vadapalli@ti.com>,
<srk@ti.com>, <jan.kiszka@siemens.com>,
<christophe.jaillet@wanadoo.fr>, <jkangas@redhat.com>,
<eballetbo@redhat.com>, <linux-remoteproc@vger.kernel.org>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick}
Date: Thu, 23 Jan 2025 10:13:12 +0530 [thread overview]
Message-ID: <8cbd9dde-e6ac-4f24-a060-9ba8abbff8d7@ti.com> (raw)
In-Reply-To: <545c990c-63f8-4d31-ae80-3454736b1329@ti.com>
On 22/01/25 00:17, Andrew Davis wrote:
> On 12/24/24 3:14 AM, Beleswar Padhi 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 to exit if the remote core's state
>> was "RPROC_DETACHED". However, this caused issues in IPC-only mode, as
>> the default state of the core is set to "RPROC_DETACHED", and the
>> transition to "RPROC_ATTACHED" happens only after the "__rproc_attach()"
>> function has invoked "rproc_start_subdevices()".
>>
>
> Sounds like the core issue was making assumptions about the state of a
> variable that is usually only used internally by the rproc core
> (rproc->state).
Yes that is right, so we now combined internal rproc state
(is_attach_ongoing) with core framework's rproc->state.
>
> Instead, what would be the harm in just dropping the state check?
> Messages coming from a detached core should be processed the same.
Taking a look at k3_r5/dsp_rproc_mbox_callback(), today we don't really
do much other than just log a print to console when we receive
non-virtio mailbox messages. If we get a virtio mailbox message from a
detached core, that will anyways be dropped in the further layers. So, I
am okay with removing these checks entirely. These additional checks
would be needed when we decide to do something more than just logging
prints, a problem for another day though.
I will respin the series with these checks dropped.
Thanks,
Beleswar
>
> Andrew
>
>> The "rproc_start_subdevices()" function triggers the probe of Virtio
>> RPMsg subdevices, which require the mailbox callbacks to be functional.
>> To resolve this, a new variable, "is_attach_ongoing", is introduced to
>> distinguish between core states: when a core is actually detached and
>> when it is in the process of being attached. The callbacks are updated
>> to return early only if the core is actually detached and not during an
>> ongoing attach operation in IPC-only mode.
>>
>> Reported-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> Closes:
>> https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/
>> Fixes: f3f11cfe8907 ("remoteproc: k3-r5: Acquire mailbox handle
>> during probe routine")
>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
>> ---
>> Link to RFC version:
>> https://lore.kernel.org/all/20240916083131.2801755-1-s-vadapalli@ti.com/
>>
>> Improvements in v1:
>> 1. Ensured these mbox callbacks are functional when the core is in
>> the proccess
>> of getting attached in IPC-Only mode.
>> 2. Ensured these mbox callbacks are _not_ functional when the core
>> state is
>> actually detached.
>>
>> drivers/remoteproc/ti_k3_r5_remoteproc.c | 53 +++++++++++++++++-------
>> 1 file changed, 39 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> index dbc513c5569c..e218a803fdb5 100644
>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> @@ -131,6 +131,7 @@ struct k3_r5_cluster {
>> * @btcm_enable: flag to control BTCM enablement
>> * @loczrama: flag to dictate which TCM is at device address 0x0
>> * @released_from_reset: flag to signal when core is out of reset
>> + * @is_attach_ongoing: flag to indicate if IPC-only "attach()" is in
>> progress
>> */
>> struct k3_r5_core {
>> struct list_head elem;
>> @@ -148,6 +149,7 @@ struct k3_r5_core {
>> u32 btcm_enable;
>> u32 loczrama;
>> bool released_from_reset;
>> + bool is_attach_ongoing;
>> };
>> /**
>> @@ -194,8 +196,11 @@ 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)
>> + /*
>> + * Do not forward messages from a detached core, except when the
>> core
>> + * is in the process of being attached in IPC-only mode.
>> + */
>> + if (!kproc->core->is_attach_ongoing && kproc->rproc->state ==
>> RPROC_DETACHED)
>> return;
>> dev_dbg(dev, "mbox msg: 0x%x\n", msg);
>> @@ -233,8 +238,11 @@ 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)
>> + /*
>> + * Do not forward messages to a detached core, except when the
>> core is
>> + * in the process of being attached in IPC-only mode.
>> + */
>> + if (!kproc->core->is_attach_ongoing && kproc->rproc->state ==
>> RPROC_DETACHED)
>> return;
>> /* send the index of the triggered virtqueue in the mailbox
>> payload */
>> @@ -671,22 +679,39 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
>> /*
>> * Attach to a running R5F remote processor (IPC-only mode)
>> *
>> - * The R5F attach callback is a NOP. The remote processor is already
>> booted, and
>> - * all required resources have been acquired during probe routine,
>> so there is
>> - * no need to issue any TI-SCI commands to boot the R5F cores in
>> IPC-only mode.
>> - * This callback is invoked only in IPC-only mode and exists because
>> - * rproc_validate() checks for its existence.
>> + * The R5F attach callback only needs to set the "is_attach_ongoing"
>> flag to
>> + * notify k3_r5_rproc_{kick/mbox_callback} functions that the core
>> is in the
>> + * process of getting attached in IPC-only mode. The remote
>> processor is
>> + * already booted, and all required resources have been acquired
>> during probe
>> + * routine, so there is no need to issue any TI-SCI commands to boot
>> the R5F
>> + * cores in IPC-only mode. This callback is invoked only in IPC-only
>> mode.
>> */
>> -static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; }
>> +static int k3_r5_rproc_attach(struct rproc *rproc)
>> +{
>> + struct k3_r5_rproc *kproc = rproc->priv;
>> +
>> + kproc->core->is_attach_ongoing = true;
>> +
>> + return 0;
>> +}
>> /*
>> * Detach from a running R5F remote processor (IPC-only mode)
>> *
>> - * The R5F detach callback is a NOP. The R5F cores are not stopped
>> and will be
>> - * left in booted state in IPC-only mode. This callback is invoked
>> only in
>> - * IPC-only mode and exists for sanity sake.
>> + * The R5F detach callback performs the opposite operation to attach
>> callback
>> + * and only needs to clear the "is_attach_ongoing" flag to ensure no
>> mailbox
>> + * messages are sent to or received from a detached core. The R5F
>> cores are
>> + * not stopped and will be left in booted state in IPC-only mode. This
>> + * callback is invoked only in IPC-only mode.
>> */
>> -static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; }
>> +static int k3_r5_rproc_detach(struct rproc *rproc)
>> +{
>> + struct k3_r5_rproc *kproc = rproc->priv;
>> +
>> + kproc->core->is_attach_ongoing = false;
>> +
>> + return 0;
>> +}
>> /*
>> * This function implements the .get_loaded_rsc_table() callback
>> and is used
next prev parent reply other threads:[~2025-01-23 4:43 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-24 9:14 [PATCH 0/3] Rework TI K3 R5F remoteproc driver Beleswar Padhi
2024-12-24 9:14 ` [PATCH 1/3] remoteproc: k3-r5: Fix checks in k3_r5_rproc_{mbox_callback/kick} Beleswar Padhi
2024-12-27 14:38 ` Hari Nagalla
2024-12-30 4:06 ` Beleswar Prasad Padhi
2025-01-03 6:05 ` Siddharth Vadapalli
2025-01-03 10:48 ` Kumar, Udit
2025-01-03 10:57 ` Beleswar Prasad Padhi
2025-01-03 13:04 ` Kumar, Udit
2025-01-21 18:47 ` Andrew Davis
2025-01-23 4:43 ` Beleswar Prasad Padhi [this message]
2024-12-24 9:14 ` [PATCH 2/3] remoteproc: k3-dsp: Fix checks in k3_dsp_rproc_{mbox_callback/kick} Beleswar Padhi
2025-01-03 6:06 ` Siddharth Vadapalli
2024-12-24 9:14 ` [PATCH v2 3/3] remoteproc: k3-r5: Refactor sequential core power up/down operations Beleswar Padhi
2025-01-03 10:48 ` Kumar, Udit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8cbd9dde-e6ac-4f24-a060-9ba8abbff8d7@ti.com \
--to=b-padhi@ti.com \
--cc=afd@ti.com \
--cc=andersson@kernel.org \
--cc=christophe.jaillet@wanadoo.fr \
--cc=eballetbo@redhat.com \
--cc=hnagalla@ti.com \
--cc=jan.kiszka@siemens.com \
--cc=jkangas@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=s-vadapalli@ti.com \
--cc=srk@ti.com \
--cc=u-kumar1@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).