[PATCH v4 0/3] Defer TI's Remoteproc's Probe until Mailbox is Probed

[PATCH v4 0/3] Defer TI's Remoteproc's Probe until Mailbox is Probed

[Beleswar Padhi]

Hello All,

This series adds deferred probe functionality in the TI's Remoteproc
drivers. The remoteproc drivers are dependent on the omap-mailbox driver
for mbox functionalities. Sometimes, the remoteproc driver could be
probed before the mailbox driver leading to rproc boot failures. Thus,
defer the probe routine of remoteproc drivers until mailbox driver is
probed by checking the mbox_request_channel handle in probe. 

Also, use the acquired mbox handle in probe during rproc start/attach
routine instead of re-requesting. Do not free mbox handle during
stop/detach routine or error paths. This makes our k3_rproc_attach() &
k3_rproc_detach() functions NOP.

Also, use the devm_rproc_alloc() helper to automatically free created
rprocs incase of a probe defer.

v4: Changelog:
* Andrew
1) Removed "Fixes:" tag in PATCH 01 and PATCH 02 of series as these are
improvements.
2) Removed NULL assignment to core->rproc to avoid possible nullptr
dereference while checking core state at a later stage.

Link to v3:
https://lore.kernel.org/all/20240807062256.1721682-1-b-padhi@ti.com/

v3: Changelog:
1) Added a check in k3_mbox_kick() to prevent sending messages to a
detached core.
2) Added "Fixes:" tag in PATCH 01 and PATCH 02 of series.

Link to v2:
https://lore.kernel.org/all/20240604051722.3608750-1-b-padhi@ti.com/

v2: Changelog:
1) Added a check in k3_mbox_callback() to prevent forwarding messages
from a detached core.
2) Addressed Andrew's comments in v1 regarding some cleanup (Using
dev_err_probe, removing unused labels, adding matching mbox_free_channel
call during device removal).

Link to v1:
https://lore.kernel.org/all/20240530090737.655054-1-b-padhi@ti.com/

Beleswar Padhi (3):
  remoteproc: k3-r5: Use devm_rproc_alloc() helper
  remoteproc: k3-r5: Acquire mailbox handle during probe routine
  remoteproc: k3-dsp: Acquire mailbox handle during probe routine

 drivers/remoteproc/ti_k3_dsp_remoteproc.c | 80 ++++++++-----------
 drivers/remoteproc/ti_k3_r5_remoteproc.c  | 94 +++++++++--------------
 2 files changed, 65 insertions(+), 109 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] remoteproc: k3-r5: Use devm_rproc_alloc() helper

[Beleswar Padhi]

Use the device lifecycle managed allocation function. This helps prevent
mistakes like freeing out of order in cleanup functions and forgetting
to free on error paths.

Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 39a47540c590..57067308b3c0 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1259,8 +1259,8 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 			goto out;
 		}
 
-		rproc = rproc_alloc(cdev, dev_name(cdev), &k3_r5_rproc_ops,
-				    fw_name, sizeof(*kproc));
+		rproc = devm_rproc_alloc(cdev, dev_name(cdev), &k3_r5_rproc_ops,
+					 fw_name, sizeof(*kproc));
 		if (!rproc) {
 			ret = -ENOMEM;
 			goto out;
@@ -1280,7 +1280,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 
 		ret = k3_r5_rproc_configure_mode(kproc);
 		if (ret < 0)
-			goto err_config;
+			goto out;
 		if (ret)
 			goto init_rmem;
 
@@ -1288,7 +1288,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 		if (ret) {
 			dev_err(dev, "initial configure failed, ret = %d\n",
 				ret);
-			goto err_config;
+			goto out;
 		}
 
 init_rmem:
@@ -1298,7 +1298,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 		if (ret) {
 			dev_err(dev, "reserved memory init failed, ret = %d\n",
 				ret);
-			goto err_config;
+			goto out;
 		}
 
 		ret = rproc_add(rproc);
@@ -1351,9 +1351,6 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 	rproc_del(rproc);
 err_add:
 	k3_r5_reserved_mem_exit(kproc);
-err_config:
-	rproc_free(rproc);
-	core->rproc = NULL;
 out:
 	/* undo core0 upon any failures on core1 in split-mode */
 	if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1) {
@@ -1398,9 +1395,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
 		rproc_del(rproc);
 
 		k3_r5_reserved_mem_exit(kproc);
-
-		rproc_free(rproc);
-		core->rproc = NULL;
 	}
 }
 
-- 
2.34.1

[PATCH v4 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe routine

[Beleswar Padhi]

Acquire the mailbox handle during device probe and do not release handle
in stop/detach routine or error paths. This removes the redundant
requests for mbox handle later during rproc start/attach. This also
allows to defer remoteproc driver's probe if mailbox is not probed yet.

Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 78 +++++++++---------------
 1 file changed, 30 insertions(+), 48 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 57067308b3c0..8a63a9360c0f 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -194,6 +194,10 @@ 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) {
@@ -229,6 +233,10 @@ 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)
@@ -399,12 +407,9 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
 	client->knows_txdone = false;
 
 	kproc->mbox = mbox_request_channel(client, 0);
-	if (IS_ERR(kproc->mbox)) {
-		ret = -EBUSY;
-		dev_err(dev, "mbox_request_channel failed: %ld\n",
-			PTR_ERR(kproc->mbox));
-		return ret;
-	}
+	if (IS_ERR(kproc->mbox))
+		return dev_err_probe(dev, PTR_ERR(kproc->mbox),
+				     "mbox_request_channel failed\n");
 
 	/*
 	 * Ping the remote processor, this is only for sanity-sake for now;
@@ -552,10 +557,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
 	u32 boot_addr;
 	int ret;
 
-	ret = k3_r5_rproc_request_mbox(rproc);
-	if (ret)
-		return ret;
-
 	boot_addr = rproc->bootaddr;
 	/* TODO: add boot_addr sanity checking */
 	dev_dbg(dev, "booting R5F core using boot addr = 0x%x\n", boot_addr);
@@ -564,7 +565,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
 	core = kproc->core;
 	ret = ti_sci_proc_set_config(core->tsp, boot_addr, 0, 0);
 	if (ret)
-		goto put_mbox;
+		return ret;
 
 	/* unhalt/run all applicable cores */
 	if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
@@ -580,13 +581,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
 		if (core != core0 && core0->rproc->state == RPROC_OFFLINE) {
 			dev_err(dev, "%s: can not start core 1 before core 0\n",
 				__func__);
-			ret = -EPERM;
-			goto put_mbox;
+			return -EPERM;
 		}
 
 		ret = k3_r5_core_run(core);
 		if (ret)
-			goto put_mbox;
+			return ret;
 	}
 
 	return 0;
@@ -596,8 +596,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
 		if (k3_r5_core_halt(core))
 			dev_warn(core->dev, "core halt back failed\n");
 	}
-put_mbox:
-	mbox_free_channel(kproc->mbox);
 	return ret;
 }
 
@@ -658,8 +656,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
 			goto out;
 	}
 
-	mbox_free_channel(kproc->mbox);
-
 	return 0;
 
 unroll_core_halt:
@@ -674,42 +670,22 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
 /*
  * Attach to a running R5F remote processor (IPC-only mode)
  *
- * The R5F attach callback only needs to request the mailbox, the remote
- * processor is already booted, 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.
+ * 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.
  */
-static int k3_r5_rproc_attach(struct rproc *rproc)
-{
-	struct k3_r5_rproc *kproc = rproc->priv;
-	struct device *dev = kproc->dev;
-	int ret;
-
-	ret = k3_r5_rproc_request_mbox(rproc);
-	if (ret)
-		return ret;
-
-	dev_info(dev, "R5F core initialized in IPC-only mode\n");
-	return 0;
-}
+static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; }
 
 /*
  * Detach from a running R5F remote processor (IPC-only mode)
  *
- * The R5F detach callback performs the opposite operation to attach callback
- * and only needs to release the mailbox, 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.
+ * 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.
  */
-static int k3_r5_rproc_detach(struct rproc *rproc)
-{
-	struct k3_r5_rproc *kproc = rproc->priv;
-	struct device *dev = kproc->dev;
-
-	mbox_free_channel(kproc->mbox);
-	dev_info(dev, "R5F core deinitialized in IPC-only mode\n");
-	return 0;
-}
+static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; }
 
 /*
  * This function implements the .get_loaded_rsc_table() callback and is used
@@ -1278,6 +1254,10 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 		kproc->rproc = rproc;
 		core->rproc = rproc;
 
+		ret = k3_r5_rproc_request_mbox(rproc);
+		if (ret)
+			return ret;
+
 		ret = k3_r5_rproc_configure_mode(kproc);
 		if (ret < 0)
 			goto out;
@@ -1392,6 +1372,8 @@ static void k3_r5_cluster_rproc_exit(void *data)
 			}
 		}
 
+		mbox_free_channel(kproc->mbox);
+
 		rproc_del(rproc);
 
 		k3_r5_reserved_mem_exit(kproc);
-- 
2.34.1

[PATCH v4 3/3] remoteproc: k3-dsp: Acquire mailbox handle during probe routine

[Beleswar Padhi]

Acquire the mailbox handle during device probe and do not release handle
in stop/detach routine or error paths. This removes the redundant
requests for mbox handle later during rproc start/attach. This also
allows to defer remoteproc driver's probe if mailbox is not probed yet.

Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
Acked-by: Andrew Davis <afd@ti.com>
---
 drivers/remoteproc/ti_k3_dsp_remoteproc.c | 80 +++++++++--------------
 1 file changed, 30 insertions(+), 50 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
index a22d41689a7d..9009367e2891 100644
--- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
@@ -115,6 +115,10 @@ static void k3_dsp_rproc_mbox_callback(struct mbox_client *client, void *data)
 	const char *name = kproc->rproc->name;
 	u32 msg = omap_mbox_message(data);
 
+	/* Do not forward messages from a detached core */
+	if (kproc->rproc->state == RPROC_DETACHED)
+		return;
+
 	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
 
 	switch (msg) {
@@ -155,6 +159,10 @@ static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
 	mbox_msg_t msg = (mbox_msg_t)vqid;
 	int ret;
 
+	/* Do not forward messages 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)
@@ -230,12 +238,9 @@ static int k3_dsp_rproc_request_mbox(struct rproc *rproc)
 	client->knows_txdone = false;
 
 	kproc->mbox = mbox_request_channel(client, 0);
-	if (IS_ERR(kproc->mbox)) {
-		ret = -EBUSY;
-		dev_err(dev, "mbox_request_channel failed: %ld\n",
-			PTR_ERR(kproc->mbox));
-		return ret;
-	}
+	if (IS_ERR(kproc->mbox))
+		return dev_err_probe(dev, PTR_ERR(kproc->mbox),
+				     "mbox_request_channel failed\n");
 
 	/*
 	 * Ping the remote processor, this is only for sanity-sake for now;
@@ -315,32 +320,23 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
 	u32 boot_addr;
 	int ret;
 
-	ret = k3_dsp_rproc_request_mbox(rproc);
-	if (ret)
-		return ret;
-
 	boot_addr = rproc->bootaddr;
 	if (boot_addr & (kproc->data->boot_align_addr - 1)) {
 		dev_err(dev, "invalid boot address 0x%x, must be aligned on a 0x%x boundary\n",
 			boot_addr, kproc->data->boot_align_addr);
-		ret = -EINVAL;
-		goto put_mbox;
+		return -EINVAL;
 	}
 
 	dev_dbg(dev, "booting DSP core using boot addr = 0x%x\n", boot_addr);
 	ret = ti_sci_proc_set_config(kproc->tsp, boot_addr, 0, 0);
 	if (ret)
-		goto put_mbox;
+		return ret;
 
 	ret = k3_dsp_rproc_release(kproc);
 	if (ret)
-		goto put_mbox;
+		return ret;
 
 	return 0;
-
-put_mbox:
-	mbox_free_channel(kproc->mbox);
-	return ret;
 }
 
 /*
@@ -353,8 +349,6 @@ static int k3_dsp_rproc_stop(struct rproc *rproc)
 {
 	struct k3_dsp_rproc *kproc = rproc->priv;
 
-	mbox_free_channel(kproc->mbox);
-
 	k3_dsp_rproc_reset(kproc);
 
 	return 0;
@@ -363,42 +357,22 @@ static int k3_dsp_rproc_stop(struct rproc *rproc)
 /*
  * Attach to a running DSP remote processor (IPC-only mode)
  *
- * This rproc attach callback only needs to request the mailbox, the remote
- * processor is already booted, so there is no need to issue any TI-SCI
- * commands to boot the DSP core. This callback is invoked only in IPC-only
- * mode.
+ * This rproc 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 DSP core. This callback
+ * is invoked only in IPC-only mode and exists because rproc_validate() checks
+ * for its existence.
  */
-static int k3_dsp_rproc_attach(struct rproc *rproc)
-{
-	struct k3_dsp_rproc *kproc = rproc->priv;
-	struct device *dev = kproc->dev;
-	int ret;
-
-	ret = k3_dsp_rproc_request_mbox(rproc);
-	if (ret)
-		return ret;
-
-	dev_info(dev, "DSP initialized in IPC-only mode\n");
-	return 0;
-}
+static int k3_dsp_rproc_attach(struct rproc *rproc) { return 0; }
 
 /*
  * Detach from a running DSP remote processor (IPC-only mode)
  *
- * This rproc detach callback performs the opposite operation to attach callback
- * and only needs to release the mailbox, the DSP core is not stopped and will
- * be left to continue to run its booted firmware. This callback is invoked only
- * in IPC-only mode.
+ * This rproc detach callback is a NOP. The DSP core is not stopped and will be
+ * left to continue to run its booted firmware. This callback is invoked only in
+ * IPC-only mode and exists for sanity sake.
  */
-static int k3_dsp_rproc_detach(struct rproc *rproc)
-{
-	struct k3_dsp_rproc *kproc = rproc->priv;
-	struct device *dev = kproc->dev;
-
-	mbox_free_channel(kproc->mbox);
-	dev_info(dev, "DSP deinitialized in IPC-only mode\n");
-	return 0;
-}
+static int k3_dsp_rproc_detach(struct rproc *rproc) { return 0; }
 
 /*
  * This function implements the .get_loaded_rsc_table() callback and is used
@@ -697,6 +671,10 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
 	kproc->dev = dev;
 	kproc->data = data;
 
+	ret = k3_dsp_rproc_request_mbox(rproc);
+	if (ret)
+		return ret;
+
 	kproc->ti_sci = devm_ti_sci_get_by_phandle(dev, "ti,sci");
 	if (IS_ERR(kproc->ti_sci))
 		return dev_err_probe(dev, PTR_ERR(kproc->ti_sci),
@@ -789,6 +767,8 @@ static void k3_dsp_rproc_remove(struct platform_device *pdev)
 		if (ret)
 			dev_err(dev, "failed to detach proc (%pe)\n", ERR_PTR(ret));
 	}
+
+	mbox_free_channel(kproc->mbox);
 }
 
 static const struct k3_dsp_mem_data c66_mems[] = {
-- 
2.34.1

Re: [PATCH v4 1/3] remoteproc: k3-r5: Use devm_rproc_alloc() helper

[Andrew Davis]

On 8/8/24 2:41 AM, Beleswar Padhi wrote:
> Use the device lifecycle managed allocation function. This helps prevent
> mistakes like freeing out of order in cleanup functions and forgetting
> to free on error paths.
> 
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---

LGTM

Reviewed-by: Andrew Davis <afd@ti.com>

>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 39a47540c590..57067308b3c0 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -1259,8 +1259,8 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   			goto out;
>   		}
>   
> -		rproc = rproc_alloc(cdev, dev_name(cdev), &k3_r5_rproc_ops,
> -				    fw_name, sizeof(*kproc));
> +		rproc = devm_rproc_alloc(cdev, dev_name(cdev), &k3_r5_rproc_ops,
> +					 fw_name, sizeof(*kproc));
>   		if (!rproc) {
>   			ret = -ENOMEM;
>   			goto out;
> @@ -1280,7 +1280,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   
>   		ret = k3_r5_rproc_configure_mode(kproc);
>   		if (ret < 0)
> -			goto err_config;
> +			goto out;
>   		if (ret)
>   			goto init_rmem;
>   
> @@ -1288,7 +1288,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   		if (ret) {
>   			dev_err(dev, "initial configure failed, ret = %d\n",
>   				ret);
> -			goto err_config;
> +			goto out;
>   		}
>   
>   init_rmem:
> @@ -1298,7 +1298,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   		if (ret) {
>   			dev_err(dev, "reserved memory init failed, ret = %d\n",
>   				ret);
> -			goto err_config;
> +			goto out;
>   		}
>   
>   		ret = rproc_add(rproc);
> @@ -1351,9 +1351,6 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   	rproc_del(rproc);
>   err_add:
>   	k3_r5_reserved_mem_exit(kproc);
> -err_config:
> -	rproc_free(rproc);
> -	core->rproc = NULL;
>   out:
>   	/* undo core0 upon any failures on core1 in split-mode */
>   	if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1) {
> @@ -1398,9 +1395,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
>   		rproc_del(rproc);
>   
>   		k3_r5_reserved_mem_exit(kproc);
> -
> -		rproc_free(rproc);
> -		core->rproc = NULL;
>   	}
>   }
>   

Re: [PATCH v4 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe routine

[Mathieu Poirier]

Hi Beleswar,

On Thu, Aug 08, 2024 at 01:11:26PM +0530, Beleswar Padhi wrote:
> Acquire the mailbox handle during device probe and do not release handle
> in stop/detach routine or error paths. This removes the redundant
> requests for mbox handle later during rproc start/attach. This also
> allows to defer remoteproc driver's probe if mailbox is not probed yet.
> 
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---
>  drivers/remoteproc/ti_k3_r5_remoteproc.c | 78 +++++++++---------------
>  1 file changed, 30 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 57067308b3c0..8a63a9360c0f 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -194,6 +194,10 @@ 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) {
> @@ -229,6 +233,10 @@ 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)
> @@ -399,12 +407,9 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
>  	client->knows_txdone = false;
>  
>  	kproc->mbox = mbox_request_channel(client, 0);
> -	if (IS_ERR(kproc->mbox)) {
> -		ret = -EBUSY;
> -		dev_err(dev, "mbox_request_channel failed: %ld\n",
> -			PTR_ERR(kproc->mbox));
> -		return ret;
> -	}
> +	if (IS_ERR(kproc->mbox))
> +		return dev_err_probe(dev, PTR_ERR(kproc->mbox),
> +				     "mbox_request_channel failed\n");
>  
>  	/*
>  	 * Ping the remote processor, this is only for sanity-sake for now;
> @@ -552,10 +557,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>  	u32 boot_addr;
>  	int ret;
>  
> -	ret = k3_r5_rproc_request_mbox(rproc);
> -	if (ret)
> -		return ret;
> -
>  	boot_addr = rproc->bootaddr;
>  	/* TODO: add boot_addr sanity checking */
>  	dev_dbg(dev, "booting R5F core using boot addr = 0x%x\n", boot_addr);
> @@ -564,7 +565,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>  	core = kproc->core;
>  	ret = ti_sci_proc_set_config(core->tsp, boot_addr, 0, 0);
>  	if (ret)
> -		goto put_mbox;
> +		return ret;
>  
>  	/* unhalt/run all applicable cores */
>  	if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> @@ -580,13 +581,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>  		if (core != core0 && core0->rproc->state == RPROC_OFFLINE) {
>  			dev_err(dev, "%s: can not start core 1 before core 0\n",
>  				__func__);
> -			ret = -EPERM;
> -			goto put_mbox;
> +			return -EPERM;
>  		}
>  
>  		ret = k3_r5_core_run(core);
>  		if (ret)
> -			goto put_mbox;
> +			return ret;
>  	}
>  
>  	return 0;
> @@ -596,8 +596,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
>  		if (k3_r5_core_halt(core))
>  			dev_warn(core->dev, "core halt back failed\n");
>  	}
> -put_mbox:
> -	mbox_free_channel(kproc->mbox);
>  	return ret;
>  }
>  
> @@ -658,8 +656,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
>  			goto out;
>  	}
>  
> -	mbox_free_channel(kproc->mbox);
> -
>  	return 0;
>  
>  unroll_core_halt:
> @@ -674,42 +670,22 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
>  /*
>   * Attach to a running R5F remote processor (IPC-only mode)
>   *
> - * The R5F attach callback only needs to request the mailbox, the remote
> - * processor is already booted, 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.
> + * 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.

Excellent documentation.

>   */
> -static int k3_r5_rproc_attach(struct rproc *rproc)
> -{
> -	struct k3_r5_rproc *kproc = rproc->priv;
> -	struct device *dev = kproc->dev;
> -	int ret;
> -
> -	ret = k3_r5_rproc_request_mbox(rproc);
> -	if (ret)
> -		return ret;
> -
> -	dev_info(dev, "R5F core initialized in IPC-only mode\n");
> -	return 0;
> -}
> +static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; }
>  
>  /*
>   * Detach from a running R5F remote processor (IPC-only mode)
>   *
> - * The R5F detach callback performs the opposite operation to attach callback
> - * and only needs to release the mailbox, 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.
> + * 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.

I would add the part about detach() being a NOP to attach() above...

>   */
> -static int k3_r5_rproc_detach(struct rproc *rproc)
> -{
> -	struct k3_r5_rproc *kproc = rproc->priv;
> -	struct device *dev = kproc->dev;
> -
> -	mbox_free_channel(kproc->mbox);
> -	dev_info(dev, "R5F core deinitialized in IPC-only mode\n");
> -	return 0;
> -}
> +static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; }

... and just remove this.

Otherwise this patch looks good.

>  
>  /*
>   * This function implements the .get_loaded_rsc_table() callback and is used
> @@ -1278,6 +1254,10 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>  		kproc->rproc = rproc;
>  		core->rproc = rproc;
>  
> +		ret = k3_r5_rproc_request_mbox(rproc);
> +		if (ret)
> +			return ret;
> +
>  		ret = k3_r5_rproc_configure_mode(kproc);
>  		if (ret < 0)
>  			goto out;
> @@ -1392,6 +1372,8 @@ static void k3_r5_cluster_rproc_exit(void *data)
>  			}
>  		}
>  
> +		mbox_free_channel(kproc->mbox);
> +
>  		rproc_del(rproc);
>  
>  		k3_r5_reserved_mem_exit(kproc);
> -- 
> 2.34.1
> 

Re: [PATCH v4 3/3] remoteproc: k3-dsp: Acquire mailbox handle during probe routine

[Mathieu Poirier]

On Thu, Aug 08, 2024 at 01:11:27PM +0530, Beleswar Padhi wrote:
> Acquire the mailbox handle during device probe and do not release handle
> in stop/detach routine or error paths. This removes the redundant
> requests for mbox handle later during rproc start/attach. This also
> allows to defer remoteproc driver's probe if mailbox is not probed yet.
> 
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> Acked-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/remoteproc/ti_k3_dsp_remoteproc.c | 80 +++++++++--------------
>  1 file changed, 30 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> index a22d41689a7d..9009367e2891 100644
> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> @@ -115,6 +115,10 @@ static void k3_dsp_rproc_mbox_callback(struct mbox_client *client, void *data)
>  	const char *name = kproc->rproc->name;
>  	u32 msg = omap_mbox_message(data);
>  
> +	/* Do not forward messages from a detached core */
> +	if (kproc->rproc->state == RPROC_DETACHED)
> +		return;
> +
>  	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
>  
>  	switch (msg) {
> @@ -155,6 +159,10 @@ static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
>  	mbox_msg_t msg = (mbox_msg_t)vqid;
>  	int ret;
>  
> +	/* Do not forward messages 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)
> @@ -230,12 +238,9 @@ static int k3_dsp_rproc_request_mbox(struct rproc *rproc)
>  	client->knows_txdone = false;
>  
>  	kproc->mbox = mbox_request_channel(client, 0);
> -	if (IS_ERR(kproc->mbox)) {
> -		ret = -EBUSY;
> -		dev_err(dev, "mbox_request_channel failed: %ld\n",
> -			PTR_ERR(kproc->mbox));
> -		return ret;
> -	}
> +	if (IS_ERR(kproc->mbox))
> +		return dev_err_probe(dev, PTR_ERR(kproc->mbox),
> +				     "mbox_request_channel failed\n");
>  
>  	/*
>  	 * Ping the remote processor, this is only for sanity-sake for now;
> @@ -315,32 +320,23 @@ static int k3_dsp_rproc_start(struct rproc *rproc)
>  	u32 boot_addr;
>  	int ret;
>  
> -	ret = k3_dsp_rproc_request_mbox(rproc);
> -	if (ret)
> -		return ret;
> -
>  	boot_addr = rproc->bootaddr;
>  	if (boot_addr & (kproc->data->boot_align_addr - 1)) {
>  		dev_err(dev, "invalid boot address 0x%x, must be aligned on a 0x%x boundary\n",
>  			boot_addr, kproc->data->boot_align_addr);
> -		ret = -EINVAL;
> -		goto put_mbox;
> +		return -EINVAL;
>  	}
>  
>  	dev_dbg(dev, "booting DSP core using boot addr = 0x%x\n", boot_addr);
>  	ret = ti_sci_proc_set_config(kproc->tsp, boot_addr, 0, 0);
>  	if (ret)
> -		goto put_mbox;
> +		return ret;
>  
>  	ret = k3_dsp_rproc_release(kproc);
>  	if (ret)
> -		goto put_mbox;
> +		return ret;
>  
>  	return 0;
> -
> -put_mbox:
> -	mbox_free_channel(kproc->mbox);
> -	return ret;
>  }
>  
>  /*
> @@ -353,8 +349,6 @@ static int k3_dsp_rproc_stop(struct rproc *rproc)
>  {
>  	struct k3_dsp_rproc *kproc = rproc->priv;
>  
> -	mbox_free_channel(kproc->mbox);
> -
>  	k3_dsp_rproc_reset(kproc);
>  
>  	return 0;
> @@ -363,42 +357,22 @@ static int k3_dsp_rproc_stop(struct rproc *rproc)
>  /*
>   * Attach to a running DSP remote processor (IPC-only mode)
>   *
> - * This rproc attach callback only needs to request the mailbox, the remote
> - * processor is already booted, so there is no need to issue any TI-SCI
> - * commands to boot the DSP core. This callback is invoked only in IPC-only
> - * mode.
> + * This rproc 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 DSP core. This callback
> + * is invoked only in IPC-only mode and exists because rproc_validate() checks
> + * for its existence.
>   */
> -static int k3_dsp_rproc_attach(struct rproc *rproc)
> -{
> -	struct k3_dsp_rproc *kproc = rproc->priv;
> -	struct device *dev = kproc->dev;
> -	int ret;
> -
> -	ret = k3_dsp_rproc_request_mbox(rproc);
> -	if (ret)
> -		return ret;
> -
> -	dev_info(dev, "DSP initialized in IPC-only mode\n");
> -	return 0;
> -}
> +static int k3_dsp_rproc_attach(struct rproc *rproc) { return 0; }
>  
>  /*
>   * Detach from a running DSP remote processor (IPC-only mode)
>   *
> - * This rproc detach callback performs the opposite operation to attach callback
> - * and only needs to release the mailbox, the DSP core is not stopped and will
> - * be left to continue to run its booted firmware. This callback is invoked only
> - * in IPC-only mode.
> + * This rproc detach callback is a NOP. The DSP core is not stopped and will be
> + * left to continue to run its booted firmware. This callback is invoked only in
> + * IPC-only mode and exists for sanity sake.
>   */
> -static int k3_dsp_rproc_detach(struct rproc *rproc)
> -{
> -	struct k3_dsp_rproc *kproc = rproc->priv;
> -	struct device *dev = kproc->dev;
> -
> -	mbox_free_channel(kproc->mbox);
> -	dev_info(dev, "DSP deinitialized in IPC-only mode\n");
> -	return 0;
> -}
> +static int k3_dsp_rproc_detach(struct rproc *rproc) { return 0; }

Same comment as the previous patch.

Thanks,
Mathieu

>  
>  /*
>   * This function implements the .get_loaded_rsc_table() callback and is used
> @@ -697,6 +671,10 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
>  	kproc->dev = dev;
>  	kproc->data = data;
>  
> +	ret = k3_dsp_rproc_request_mbox(rproc);
> +	if (ret)
> +		return ret;
> +
>  	kproc->ti_sci = devm_ti_sci_get_by_phandle(dev, "ti,sci");
>  	if (IS_ERR(kproc->ti_sci))
>  		return dev_err_probe(dev, PTR_ERR(kproc->ti_sci),
> @@ -789,6 +767,8 @@ static void k3_dsp_rproc_remove(struct platform_device *pdev)
>  		if (ret)
>  			dev_err(dev, "failed to detach proc (%pe)\n", ERR_PTR(ret));
>  	}
> +
> +	mbox_free_channel(kproc->mbox);
>  }
>  
>  static const struct k3_dsp_mem_data c66_mems[] = {
> -- 
> 2.34.1
> 

Re: [PATCH v4 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe routine

[Beleswar Prasad Padhi]

Hi Mathieu,

On 14-08-2024 21:22, Mathieu Poirier wrote:
> Hi Beleswar, On Thu, Aug 08, 2024 at 01: 11: 26PM +0530, Beleswar 
> Padhi wrote: > Acquire the mailbox handle during device probe and do 
> not release handle > in stop/detach routine or error paths. This 
> removes the redundant > requests for
> ZjQcmQRYFpfptBannerStart
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!vldnVV7DH2eSIoaksHjpMPogloWUPfAcp2-dEVbMoE1YA3kGFXdJXGAJUKJm$> 
>
> ZjQcmQRYFpfptBannerEnd
> Hi Beleswar,
>
> On Thu, Aug 08, 2024 at 01:11:26PM +0530, Beleswar Padhi wrote:
> > Acquire the mailbox handle during device probe and do not release handle
> > in stop/detach routine or error paths. This removes the redundant
> > requests for mbox handle later during rproc start/attach. This also
> > allows to defer remoteproc driver's probe if mailbox is not probed yet.
> > 
> > Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> > ---
> >  drivers/remoteproc/ti_k3_r5_remoteproc.c | 78 +++++++++---------------
> >  1 file changed, 30 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > index 57067308b3c0..8a63a9360c0f 100644
> > --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > @@ -194,6 +194,10 @@ 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) {
> > @@ -229,6 +233,10 @@ 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)
> > @@ -399,12 +407,9 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
> >  	client->knows_txdone = false;
> >  
> >  	kproc->mbox = mbox_request_channel(client, 0);
> > -	if (IS_ERR(kproc->mbox)) {
> > -		ret = -EBUSY;
> > -		dev_err(dev, "mbox_request_channel failed: %ld\n",
> > -			PTR_ERR(kproc->mbox));
> > -		return ret;
> > -	}
> > +	if (IS_ERR(kproc->mbox))
> > +		return dev_err_probe(dev, PTR_ERR(kproc->mbox),
> > +				     "mbox_request_channel failed\n");
> >  
> >  	/*
> >  	 * Ping the remote processor, this is only for sanity-sake for now;
> > @@ -552,10 +557,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> >  	u32 boot_addr;
> >  	int ret;
> >  
> > -	ret = k3_r5_rproc_request_mbox(rproc);
> > -	if (ret)
> > -		return ret;
> > -
> >  	boot_addr = rproc->bootaddr;
> >  	/* TODO: add boot_addr sanity checking */
> >  	dev_dbg(dev, "booting R5F core using boot addr = 0x%x\n", boot_addr);
> > @@ -564,7 +565,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> >  	core = kproc->core;
> >  	ret = ti_sci_proc_set_config(core->tsp, boot_addr, 0, 0);
> >  	if (ret)
> > -		goto put_mbox;
> > +		return ret;
> >  
> >  	/* unhalt/run all applicable cores */
> >  	if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> > @@ -580,13 +581,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> >  		if (core != core0 && core0->rproc->state == RPROC_OFFLINE) {
> >  			dev_err(dev, "%s: can not start core 1 before core 0\n",
> >  				__func__);
> > -			ret = -EPERM;
> > -			goto put_mbox;
> > +			return -EPERM;
> >  		}
> >  
> >  		ret = k3_r5_core_run(core);
> >  		if (ret)
> > -			goto put_mbox;
> > +			return ret;
> >  	}
> >  
> >  	return 0;
> > @@ -596,8 +596,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> >  		if (k3_r5_core_halt(core))
> >  			dev_warn(core->dev, "core halt back failed\n");
> >  	}
> > -put_mbox:
> > -	mbox_free_channel(kproc->mbox);
> >  	return ret;
> >  }
> >  
> > @@ -658,8 +656,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
> >  			goto out;
> >  	}
> >  
> > -	mbox_free_channel(kproc->mbox);
> > -
> >  	return 0;
> >  
> >  unroll_core_halt:
> > @@ -674,42 +670,22 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
> >  /*
> >   * Attach to a running R5F remote processor (IPC-only mode)
> >   *
> > - * The R5F attach callback only needs to request the mailbox, the remote
> > - * processor is already booted, 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.
> > + * 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.
>
> Excellent documentation.


Thanks!

>
> >   */
> > -static int k3_r5_rproc_attach(struct rproc *rproc)
> > -{
> > -	struct k3_r5_rproc *kproc = rproc->priv;
> > -	struct device *dev = kproc->dev;
> > -	int ret;
> > -
> > -	ret = k3_r5_rproc_request_mbox(rproc);
> > -	if (ret)
> > -		return ret;
> > -
> > -	dev_info(dev, "R5F core initialized in IPC-only mode\n");
> > -	return 0;
> > -}
> > +static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; }
> >  
> >  /*
> >   * Detach from a running R5F remote processor (IPC-only mode)
> >   *
> > - * The R5F detach callback performs the opposite operation to attach callback
> > - * and only needs to release the mailbox, 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.
> > + * 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.
>
> I would add the part about detach() being a NOP to attach() above...
>
> >   */
> > -static int k3_r5_rproc_detach(struct rproc *rproc)
> > -{
> > -	struct k3_r5_rproc *kproc = rproc->priv;
> > -	struct device *dev = kproc->dev;
> > -
> > -	mbox_free_channel(kproc->mbox);
> > -	dev_info(dev, "R5F core deinitialized in IPC-only mode\n");
> > -	return 0;
> > -}
> > +static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; }
>
> ... and just remove this.


Thanks for the comments. But dropping k3_r5_rproc_detach() would mean we 
would get -EINVAL[1] while trying to detach the core from sysfs[0]. Is 
it expected?

[0]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/remoteproc_sysfs.c#n202
[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/remoteproc_core.c#n1752

>
> Otherwise this patch looks good.
>
> >  
> >  /*
> >   * This function implements the .get_loaded_rsc_table() callback and is used
> > @@ -1278,6 +1254,10 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
> >  		kproc->rproc = rproc;
> >  		core->rproc = rproc;
> >  
> > +		ret = k3_r5_rproc_request_mbox(rproc);
> > +		if (ret)
> > +			return ret;
> > +
> >  		ret = k3_r5_rproc_configure_mode(kproc);
> >  		if (ret < 0)
> >  			goto out;
> > @@ -1392,6 +1372,8 @@ static void k3_r5_cluster_rproc_exit(void *data)
> >  			}
> >  		}
> >  
> > +		mbox_free_channel(kproc->mbox);
> > +
> >  		rproc_del(rproc);
> >  
> >  		k3_r5_reserved_mem_exit(kproc);
> > -- 
> > 2.34.1
> > 

Re: [PATCH v4 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe routine

[Mathieu Poirier]

On Fri, Aug 16, 2024 at 01:23:59PM +0530, Beleswar Prasad Padhi wrote:
> Hi Mathieu,
> 
> On 14-08-2024 21:22, Mathieu Poirier wrote:
> > Hi Beleswar, On Thu, Aug 08, 2024 at 01: 11: 26PM +0530, Beleswar Padhi
> > wrote: > Acquire the mailbox handle during device probe and do not
> > release handle > in stop/detach routine or error paths. This removes the
> > redundant > requests for
> > ZjQcmQRYFpfptBannerStart
> > Report Suspicious
> > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!vldnVV7DH2eSIoaksHjpMPogloWUPfAcp2-dEVbMoE1YA3kGFXdJXGAJUKJm$>
> > 
> > ZjQcmQRYFpfptBannerEnd
> > Hi Beleswar,
> > 
> > On Thu, Aug 08, 2024 at 01:11:26PM +0530, Beleswar Padhi wrote:
> > > Acquire the mailbox handle during device probe and do not release handle
> > > in stop/detach routine or error paths. This removes the redundant
> > > requests for mbox handle later during rproc start/attach. This also
> > > allows to defer remoteproc driver's probe if mailbox is not probed yet.
> > > > Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> > > ---
> > >  drivers/remoteproc/ti_k3_r5_remoteproc.c | 78 +++++++++---------------
> > >  1 file changed, 30 insertions(+), 48 deletions(-)
> > > > diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > > index 57067308b3c0..8a63a9360c0f 100644
> > > --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > > +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > > @@ -194,6 +194,10 @@ 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) {
> > > @@ -229,6 +233,10 @@ 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)
> > > @@ -399,12 +407,9 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
> > >  	client->knows_txdone = false;
> > >  >  	kproc->mbox = mbox_request_channel(client, 0);
> > > -	if (IS_ERR(kproc->mbox)) {
> > > -		ret = -EBUSY;
> > > -		dev_err(dev, "mbox_request_channel failed: %ld\n",
> > > -			PTR_ERR(kproc->mbox));
> > > -		return ret;
> > > -	}
> > > +	if (IS_ERR(kproc->mbox))
> > > +		return dev_err_probe(dev, PTR_ERR(kproc->mbox),
> > > +				     "mbox_request_channel failed\n");
> > >  >  	/*
> > >  	 * Ping the remote processor, this is only for sanity-sake for now;
> > > @@ -552,10 +557,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> > >  	u32 boot_addr;
> > >  	int ret;
> > >  > -	ret = k3_r5_rproc_request_mbox(rproc);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	boot_addr = rproc->bootaddr;
> > >  	/* TODO: add boot_addr sanity checking */
> > >  	dev_dbg(dev, "booting R5F core using boot addr = 0x%x\n", boot_addr);
> > > @@ -564,7 +565,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> > >  	core = kproc->core;
> > >  	ret = ti_sci_proc_set_config(core->tsp, boot_addr, 0, 0);
> > >  	if (ret)
> > > -		goto put_mbox;
> > > +		return ret;
> > >  >  	/* unhalt/run all applicable cores */
> > >  	if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> > > @@ -580,13 +581,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> > >  		if (core != core0 && core0->rproc->state == RPROC_OFFLINE) {
> > >  			dev_err(dev, "%s: can not start core 1 before core 0\n",
> > >  				__func__);
> > > -			ret = -EPERM;
> > > -			goto put_mbox;
> > > +			return -EPERM;
> > >  		}
> > >  >  		ret = k3_r5_core_run(core);
> > >  		if (ret)
> > > -			goto put_mbox;
> > > +			return ret;
> > >  	}
> > >  >  	return 0;
> > > @@ -596,8 +596,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
> > >  		if (k3_r5_core_halt(core))
> > >  			dev_warn(core->dev, "core halt back failed\n");
> > >  	}
> > > -put_mbox:
> > > -	mbox_free_channel(kproc->mbox);
> > >  	return ret;
> > >  }
> > >  > @@ -658,8 +656,6 @@ static int k3_r5_rproc_stop(struct rproc
> > *rproc)
> > >  			goto out;
> > >  	}
> > >  > -	mbox_free_channel(kproc->mbox);
> > > -
> > >  	return 0;
> > >  >  unroll_core_halt:
> > > @@ -674,42 +670,22 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
> > >  /*
> > >   * Attach to a running R5F remote processor (IPC-only mode)
> > >   *
> > > - * The R5F attach callback only needs to request the mailbox, the remote
> > > - * processor is already booted, 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.
> > > + * 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.
> > 
> > Excellent documentation.
> 
> 
> Thanks!
> 
> > 
> > >   */
> > > -static int k3_r5_rproc_attach(struct rproc *rproc)
> > > -{
> > > -	struct k3_r5_rproc *kproc = rproc->priv;
> > > -	struct device *dev = kproc->dev;
> > > -	int ret;
> > > -
> > > -	ret = k3_r5_rproc_request_mbox(rproc);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	dev_info(dev, "R5F core initialized in IPC-only mode\n");
> > > -	return 0;
> > > -}
> > > +static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; }
> > >  >  /*
> > >   * Detach from a running R5F remote processor (IPC-only mode)
> > >   *
> > > - * The R5F detach callback performs the opposite operation to attach callback
> > > - * and only needs to release the mailbox, 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.
> > > + * 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.
> > 
> > I would add the part about detach() being a NOP to attach() above...
> > 
> > >   */
> > > -static int k3_r5_rproc_detach(struct rproc *rproc)
> > > -{
> > > -	struct k3_r5_rproc *kproc = rproc->priv;
> > > -	struct device *dev = kproc->dev;
> > > -
> > > -	mbox_free_channel(kproc->mbox);
> > > -	dev_info(dev, "R5F core deinitialized in IPC-only mode\n");
> > > -	return 0;
> > > -}
> > > +static int k3_r5_rproc_detach(struct rproc *rproc) { return 0; }
> > 
> > ... and just remove this.
> 
> 
> Thanks for the comments. But dropping k3_r5_rproc_detach() would mean we
> would get -EINVAL[1] while trying to detach the core from sysfs[0]. Is it
> expected?
>

You are correct.  I have applied your patch.

> [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/remoteproc_sysfs.c#n202
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/remoteproc_core.c#n1752
> 
> > 
> > Otherwise this patch looks good.
> > 
> > >  >  /*
> > >   * This function implements the .get_loaded_rsc_table() callback and is used
> > > @@ -1278,6 +1254,10 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
> > >  		kproc->rproc = rproc;
> > >  		core->rproc = rproc;
> > >  > +		ret = k3_r5_rproc_request_mbox(rproc);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > >  		ret = k3_r5_rproc_configure_mode(kproc);
> > >  		if (ret < 0)
> > >  			goto out;
> > > @@ -1392,6 +1372,8 @@ static void k3_r5_cluster_rproc_exit(void *data)
> > >  			}
> > >  		}
> > >  > +		mbox_free_channel(kproc->mbox);
> > > +
> > >  		rproc_del(rproc);
> > >  >  		k3_r5_reserved_mem_exit(kproc);
> > > -- > 2.34.1
> > >