| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANLsYkw0YqSDC7-SmA4x4hB1Z1kO5MVR-dZb8PAbkw6RMW6PNA@mail.gmail.com>
Date: Wed, 3 Sep 2025 09:06:34 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Peng Fan <peng.fan@....nxp.com>
Cc: Peng Fan <peng.fan@....com>, Bjorn Andersson <andersson@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, Frank Li <frank.li@....com>,
Daniel Baluta <daniel.baluta@....com>, Iuliana Prodan <iuliana.prodan@....com>,
linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/3] remoteproc: imx_rproc: Add support for System
Manager API
On Tue, 2 Sept 2025 at 23:28, Peng Fan <peng.fan@....nxp.com> wrote:
>
> On Wed, Sep 03, 2025 at 12:56:11PM +0800, Peng Fan wrote:
> >On Tue, Sep 02, 2025 at 10:38:49AM -0600, Mathieu Poirier wrote:
> >>On Sat, Aug 30, 2025 at 08:52:09PM +0800, Peng Fan wrote:
> >>> On Fri, Aug 29, 2025 at 10:00:04AM -0600, Mathieu Poirier wrote:
> >>> >Good day,
> >>> >
> >>> >On Thu, Aug 21, 2025 at 05:05:05PM +0800, Peng Fan wrote:
> >>> >> i.MX95 features a Cortex-M33 core, six Cortex-A55 cores, and
> >>> >> one Cortex-M7 core. The System Control Management Interface(SCMI)
> >>> >> firmware runs on the M33 core. The i.MX95 SCMI firmware named System
> >>> >> Manager(SM) includes vendor extension protocols, Logical Machine
> >>> >> Management(LMM) protocol and CPU protocol and etc.
> >>> >>
> >>> >> There are three cases for M7:
> >>> >> (1) M7 in a separate Logical Machine(LM) that Linux can't control it.
> >>> >> (2) M7 in a separate Logical Machine that Linux can control it using
> >>> >> LMM protocol
> >>> >> (3) M7 runs in same Logical Machine as A55, so Linux can control it
> >>> >> using CPU protocol
> >>> >>
> >>> >> So extend the driver to using LMM and CPU protocol to manage the M7 core.
> >>> >> - Add IMX_RPROC_SM to indicate the remote core runs on a SoC that
> >>> >> has System Manager.
> >>> >> - Compare linux LM ID(got using scmi_imx_lmm_info) and M7 LM ID(the ID
> >>> >> is fixed as 1 in SM firmware if M7 is in a seprate LM),
> >>> >> if Linux LM ID equals M7 LM ID(linux and M7 in same LM), use CPU
> >>> >> protocol to start/stop. Otherwise, use LMM protocol to start/stop.
> >>> >> Whether using CPU or LMM protocol to start/stop, the M7 status
> >>> >> detection could use CPU protocol to detect started or not. So
> >>> >> in imx_rproc_detect_mode, use scmi_imx_cpu_started to check the
> >>> >> status of M7.
> >>> >> - For above case 1 and 2, Use SCMI_IMX_LMM_POWER_ON to detect whether
> >>> >> the M7 LM is under control of A55 LM.
> >>> >>
> >>> >> Current setup relies on pre-Linux software(U-Boot) to do
> >>> >> M7 TCM ECC initialization. In future, we could add the support in Linux
> >>> >> to decouple U-Boot and Linux.
> >>> >>
> >>> >> Reviewed-by: Daniel Baluta <daniel.baluta@....com>
> >>> >> Reviewed-by: Frank Li <Frank.Li@....com>
> >>> >> Signed-off-by: Peng Fan <peng.fan@....com>
> >>> >> ---
> >>> >> drivers/remoteproc/Kconfig | 2 +
> >>> >> drivers/remoteproc/imx_rproc.c | 123 ++++++++++++++++++++++++++++++++++++++++-
> >>> >> drivers/remoteproc/imx_rproc.h | 5 ++
> >>> >> 3 files changed, 127 insertions(+), 3 deletions(-)
> >>> >>
> >>> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >>> >> index 48a0d3a69ed08057716f1e7ea950899f60bbe0cf..ee54436fea5ad08a9c198ce74d44ce7a9aa206de 100644
> >>> >> --- a/drivers/remoteproc/Kconfig
> >>> >> +++ b/drivers/remoteproc/Kconfig
> >>> >> @@ -27,6 +27,8 @@ config IMX_REMOTEPROC
> >>> >> tristate "i.MX remoteproc support"
> >>> >> depends on ARCH_MXC
> >>> >> depends on HAVE_ARM_SMCCC
> >>> >> + depends on IMX_SCMI_CPU_DRV || !IMX_SCMI_CPU_DRV
> >>> >> + depends on IMX_SCMI_LMM_DRV || !IMX_SCMI_LMM_DRV
> >>> >> select MAILBOX
> >>> >> help
> >>> >> Say y here to support iMX's remote processors via the remote
> >>> >> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> >>> >> index a6eef0080ca9e46efe60dcb3878b9efdbdc0f08e..151b9ca34bac2dac9df0ed873f493791f2d1466e 100644
> >>> >> --- a/drivers/remoteproc/imx_rproc.c
> >>> >> +++ b/drivers/remoteproc/imx_rproc.c
> >>> >> @@ -8,6 +8,7 @@
> >>> >> #include <linux/clk.h>
> >>> >> #include <linux/err.h>
> >>> >> #include <linux/firmware/imx/sci.h>
> >>> >> +#include <linux/firmware/imx/sm.h>
> >>> >> #include <linux/interrupt.h>
> >>> >> #include <linux/kernel.h>
> >>> >> #include <linux/mailbox_client.h>
> >>> >> @@ -22,6 +23,7 @@
> >>> >> #include <linux/reboot.h>
> >>> >> #include <linux/regmap.h>
> >>> >> #include <linux/remoteproc.h>
> >>> >> +#include <linux/scmi_imx_protocol.h>
> >>> >> #include <linux/workqueue.h>
> >>> >>
> >>> >> #include "imx_rproc.h"
> >>> >> @@ -92,6 +94,11 @@ struct imx_rproc_mem {
> >>> >> #define ATT_CORE_MASK 0xffff
> >>> >> #define ATT_CORE(I) BIT((I))
> >>> >>
> >>> >> +/* Logical Machine Operation */
> >>> >> +#define IMX_RPROC_FLAGS_SM_LMM_OP BIT(0)
> >>> >> +/* Linux has permission to handle the Logical Machine of remote cores */
> >>> >> +#define IMX_RPROC_FLAGS_SM_LMM_AVAIL BIT(1)
> >>> >> +
> >>> >> static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block);
> >>> >> static void imx_rproc_free_mbox(struct rproc *rproc);
> >>> >>
> >>> >> @@ -116,6 +123,8 @@ struct imx_rproc {
> >>> >> u32 entry; /* cpu start address */
> >>> >> u32 core_index;
> >>> >> struct dev_pm_domain_list *pd_list;
> >>> >> + /* For i.MX System Manager based systems */
> >>> >> + u32 flags;
> >>> >> };
> >>> >>
> >>> >> static const struct imx_rproc_att imx_rproc_att_imx93[] = {
> >>> >> @@ -394,6 +403,30 @@ static int imx_rproc_start(struct rproc *rproc)
> >>> >> case IMX_RPROC_SCU_API:
> >>> >> ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc_id, true, priv->entry);
> >>> >> break;
> >>> >> + case IMX_RPROC_SM:
> >>> >> + if (priv->flags & IMX_RPROC_FLAGS_SM_LMM_OP) {
> >>> >> + if (!(priv->flags & IMX_RPROC_FLAGS_SM_LMM_AVAIL))
> >>> >> + return -EACCES;
> >>> >> +
> >>> >> + ret = scmi_imx_lmm_reset_vector_set(dcfg->lmid, dcfg->cpuid, 0, 0);
> >>> >> + if (ret) {
> >>> >> + dev_err(dev, "Failed to set reset vector lmid(%u), cpuid(%u): %d\n",
> >>> >> + dcfg->lmid, dcfg->cpuid, ret);
> >>> >> + }
> >>> >> +
> >>> >> + ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_BOOT, 0);
> >>> >> + if (ret)
> >>> >> + dev_err(dev, "Failed to boot lmm(%d): %d\n", ret, dcfg->lmid);
> >>> >> + } else {
> >>> >> + ret = scmi_imx_cpu_reset_vector_set(dcfg->cpuid, 0, true, false, false);
> >>> >> + if (ret) {
> >>> >> + dev_err(dev, "Failed to set reset vector cpuid(%u): %d\n",
> >>> >> + dcfg->cpuid, ret);
> >>> >> + }
> >>> >> +
> >>> >> + ret = scmi_imx_cpu_start(dcfg->cpuid, true);
> >>> >> + }
> >>> >> + break;
> >>> >> default:
> >>> >> return -EOPNOTSUPP;
> >>> >> }
> >>> >> @@ -436,6 +469,16 @@ static int imx_rproc_stop(struct rproc *rproc)
> >>> >> case IMX_RPROC_SCU_API:
> >>> >> ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc_id, false, priv->entry);
> >>> >> break;
> >>> >> + case IMX_RPROC_SM:
> >>> >> + if (priv->flags & IMX_RPROC_FLAGS_SM_LMM_OP) {
> >>> >> + if (priv->flags & IMX_RPROC_FLAGS_SM_LMM_AVAIL)
> >>> >> + ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_SHUTDOWN, 0);
> >>> >> + else
> >>> >> + ret = -EACCES;
> >>> >> + } else {
> >>> >> + ret = scmi_imx_cpu_start(dcfg->cpuid, false);
> >>> >> + }
> >>> >> + break;
> >>> >> default:
> >>> >> return -EOPNOTSUPP;
> >>> >> }
> >>> >> @@ -546,10 +589,48 @@ static int imx_rproc_mem_release(struct rproc *rproc,
> >>> >> return 0;
> >>> >> }
> >>> >>
> >>> >> +static int imx_rproc_sm_lmm_prepare(struct rproc *rproc)
> >>> >> +{
> >>> >> + struct imx_rproc *priv = rproc->priv;
> >>> >> + const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> >>> >> + int ret;
> >>> >> +
> >>> >> + if (!(priv->flags & IMX_RPROC_FLAGS_SM_LMM_OP))
> >>> >> + return 0;
> >>> >> +
> >>> >> + /*
> >>> >> + * Power on the Logical Machine to make sure TCM is available.
> >>> >> + * Also serve as permission check. If in different Logical
> >>> >> + * Machine, and linux has permission to handle the Logical
> >>> >> + * Machine, set IMX_RPROC_FLAGS_SM_LMM_AVAIL.
> >>> >> + */
> >>> >> + ret = scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_POWER_ON, 0);
> >>> >> + if (ret == 0) {
> >>> >> + dev_info(priv->dev, "lmm(%d) powered on\n", dcfg->lmid);
> >>> >> + priv->flags |= IMX_RPROC_FLAGS_SM_LMM_AVAIL;
> >>> >
> >>> >Why is setting this flag needed? The check that is done on it in
> >>> >imx_rproc_{start|stop} should be done here. If there is an error then we don't
> >>> >move forward.
> >>>
> >>> The flag is to indicate M7 LM could be controlled by Linux LM(to save SCMI
> >>> calls. without this flag, heavy SCMI calls will be runs into). The reason
> >>> on why set it here:
> >>> The prepare function will be invoked in two path: rproc_attach and rproc_fw_boot.
> >>> When M7 LM works in detached state and not under control of Linux LM, rproc_stop
> >>> could still be invoked, so we need to avoid Linux runs into scmi calls to
> >>> stop M7 LM(even if scmi firmware will return -EACCESS, but with a flag, we could
> >>> save a SCMI call), so there is a check in imx_rproc_stop and this is why
> >>> we need a flag there.
> >>>
> >>> The flag check in start might be redundant, but to keep safe, I still keep
> >>> it there.
> >>
> >>One of the (many) problem I see with this patch is that there is no
> >>IMX_RPROC_FLAGS_SM_CPU_OP to complement IMX_RPROC_FLAGS_SM_LMM_OP in
> >>imx_rproc_detect_mode(), leading to if/else statements that are hard to follow.
> >
> >There are only two methods as written in commit log.
> >one is LMM_OP, the other is CPU_OP
> >
> >>
> >>In imx_rproc_sm_lmm_prepare(), if scmi_imx_lmm_operation() is successful, return
> >>0 immediately. If -EACCESS the LMM method is unavailable in both normal and
> >>detached mode, so priv->flags &= ~IMX_RPROC_FLAGS_SM_LMM_OP.
> >
> >No. LMM in avaiable in normal and detach mode. I have written in commit log,
> >There are three cases for M7:
> > (1) M7 in a separate Logical Machine(LM) that Linux can't control it.
> > (2) M7 in a separate Logical Machine that Linux can control it using
> > LMM protocol
> > (3) M7 runs in same Logical Machine as A55, so Linux can control it
> > using CPU protocol
> >
> >>
> >>The main takeaway here is that the code introduced by this patch is difficult to
> >>understand and maintain. I suggest you find a way to make things simpler.
> >
> >You asked Daniel to help review this patchset. Daniel and Frank both help
> >reviewed this patchset and gave R-b.
> >
> >You also said this patchset "looks fine to you" Jul 21 [1].
> >
> >Now you overturn these and say "find a way to make things simpler.
> >What's the point to ask my colleague to review? What should I do,
> >redesign the patchset according to "make things simpler"?
> >
> >Please give detailed suggestions, but not a general comment.
>
> I realize my previous message may have come across as frustrated — I truly
> appreciate your time and feedback. I’m just trying to understand the
> direction you’d prefer for this patchset, especially since it had prior
> R-b’s and your earlier comment that it “looks fine.”
>
I simply started taking a closer look and quickly found problems.
> Could you please help clarify what specifically you’d like simplified?
> I’m happy to revise accordingly, but I’d really appreciate concrete
> suggestions so I can move forward effectively.
>
I did give suggestions. The core of this patch is 123 lines - even
after spending two full hours reviewing it I still don't have a clear
picture of what is happening. When that is the case, something is not
right. When I look at a patchset, I usually ask myself if I'll
remember what it does when looking at it again in 6 months. The
answer was a clear "no" with this one.
> @Daniel, @Frank — since you've reviewed and R-b'd this patchset, do you
> have thoughts on the latest feedback from Mathieu? Would you agree that
> further simplification is needed, or is the current structure acceptable?”
>
> Thanks again!
>
> Thanks,
> Peng
>
> >
> >[1] https://lore.kernel.org/all/CANLsYkwZz4xLOG25D6S-AEGFeUBWwyp1=ytmu2q90NyEpkoX9g@mail.gmail.com/
> >
> >Thanks,
> >Peng
> >>
> >
Powered by blists - more mailing lists