Hi Mathieu,

Subject: Re: [PATCH V6 6/7] remoteproc: imx_rproc: request mbox channel
later

On Thu, Sep 29, 2022 at 02:17:03PM +0800, Peng Fan (OSS) wrote:

quoted

From: Peng Fan <peng.fan@nxp.com>

It is possible that when remote processor crash, the communication
channel will be broken with garbage value in mailbox, such as when
Linux is issuing a message through mailbox, remote processor crashes,
we need free & rebuild the mailbox channels to make sure no garbage
value in mailbox channels.

So move the request/free to start/stop for managing remote procesosr
in Linux, move to attach/detach for remote processor is out of control
of Linux.

Previous, we just request mbox when attach for CM4 boot early before
Linux, but if mbox defer probe, remoteproc core will do resource
cleanup and corrupt resource table for later probe.

So move request mbox ealier and still keep mbox request when attach
for self recovery case, but keep a check when request/free mbox.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c

b/drivers/remoteproc/imx_rproc.c index 8175c4c9aa22..bece44b46719
100644

--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c

@@ -84,6 +84,8 @@ struct imx_rproc_mem {
 #define ATT_CORE_MASK   0xffff
 #define ATT_CORE(I)     BIT((I))

+static int imx_rproc_xtr_mbox_init(struct rproc *rproc); static void
+imx_rproc_free_mbox(struct rproc *rproc);
 static int imx_rproc_detach_pd(struct rproc *rproc);

 struct imx_rproc {

@@ -357,6 +359,10 @@ static int imx_rproc_start(struct rproc *rproc)
 	struct arm_smccc_res res;
 	int ret;

+	ret = imx_rproc_xtr_mbox_init(rproc);
+	if (ret)
+		return ret;
+
 	switch (dcfg->method) {
 	case IMX_RPROC_MMIO:
 		ret = regmap_update_bits(priv->regmap, dcfg->src_reg,

dcfg->src_mask, @@ -407,6 +413,8 @@ static int imx_rproc_stop(struct
rproc *rproc)

 	if (ret)
 		dev_err(dev, "Failed to stop remote core\n");
+	else
+		imx_rproc_free_mbox(rproc);

 	return ret;
 }

@@ -592,7 +600,7 @@ static void imx_rproc_kick(struct rproc *rproc,

int vqid)

 static int imx_rproc_attach(struct rproc *rproc)  {
-	return 0;
+	return imx_rproc_xtr_mbox_init(rproc);
 }

 static struct resource_table *imx_rproc_get_loaded_rsc_table(struct
rproc *rproc, size_t *table_sz) @@ -720,6 +728,9 @@ static int

imx_rproc_xtr_mbox_init(struct rproc *rproc)

quoted

 	struct device *dev = priv->dev;
 	struct mbox_client *cl;

+	if (priv->tx_ch && priv->rx_ch)
+		return 0;
+

Why is this needed?  The remoteproc core's state machine driven by rproc-

quoted

state guarantees that imx_rproc_start(), imx_rproc_stop() and

imx_rproc_attach are not called more than once.

And why is imx_rproc_xtr_mbox_init() in imx_rproc_probe() not removed?

I not remove it, code piece in probe function:

        ret = imx_rproc_xtr_mbox_init(rproc);
        if (ret)
                goto err_put_wkq;

        ret = imx_rproc_addr_init(priv, pdev);
        if (ret) {
                dev_err(dev, "failed on imx_rproc_addr_init\n");
                goto err_put_mbox;
        }

        ret = imx_rproc_detect_mode(priv);
        if (ret)
                goto err_put_mbox;

quoted

 	if (!of_get_property(dev->of_node, "mbox-names", NULL))
 		return 0;

@@ -749,8 +760,15 @@ static void imx_rproc_free_mbox(struct rproc

*rproc)  {
 	struct imx_rproc *priv = rproc->priv;

-	mbox_free_channel(priv->tx_ch);
-	mbox_free_channel(priv->rx_ch);
+	if (priv->tx_ch) {
+		mbox_free_channel(priv->tx_ch);
+		priv->tx_ch = NULL;
+	}
+
+	if (priv->rx_ch) {
+		mbox_free_channel(priv->rx_ch);
+		priv->rx_ch = NULL;
+	}
 }

 static void imx_rproc_put_scu(struct rproc *rproc) @@ -779,6 +797,8

@@ static int imx_rproc_partition_notify(struct notifier_block *nb,
 	if (!((event & BIT(priv->rproc_pt)) && (*(u8 *)group ==

IMX_SC_IRQ_GROUP_REBOOTED)))

quoted

 		return 0;

+	imx_rproc_free_mbox(priv->rproc);

Why putting this here when it will be called again in imx_rproc_stop()?  If
you need it for the attached scenario, create imx_rproc_detach() and add it
there.

This is when got notification from CM4, Linux free the mbox channel. 
The noficiation only work when CM4 runs in separated partition, linux not
able to start/stop CM4.
Patch 7/7 attach recovery will move this out. Here is just free mbox after CM4
crash, and linux not has permission to stop CM4.

Thanks,
Peng.

quoted

+
 	rproc_report_crash(priv->rproc, RPROC_WATCHDOG);

 	pr_info("Partition%d reset!\n", priv->rproc_pt);
--
2.37.1

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