| 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: <d6vx4owncfgnpnujfepavtxeg76pt6rzlxf4fhhd6uwwtrfl3v@6rnc2llvoo26>
Date: Mon, 3 Nov 2025 20:01:46 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Deepak Kumar Singh <deepak.singh@....qualcomm.com>
Cc: chris.lew@....qualcomm.com, konradybcio@...nel.org,
jingyi.wang@....qualcomm.com, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH V2 2/2] soc: qcom: smp2p: Add support for smp2p v2
On Mon, Nov 03, 2025 at 08:59:29PM +0530, Deepak Kumar Singh wrote:
> From: Chris Lew <chris.lew@....qualcomm.com>
>
> Some remoteproc need smp2p v2 support, mirror the version written by the
> remote if the remote supports v2.
I don't think they _need_ v2 support. The subsystem might implement v2
and only support v2...
> This is needed if the remote does not have backwards compatibility
> with v1.
I guess this retroactively amends the previous sentence to make it
valid? Please rewrite these two sentences.
> And reset entry last value on SSR for smp2p v2.
The first two sentences described a problem and a "solution" to that
problem, here you're just throwing in a fact.
Please document what version 2 actually is and make it clear why
resetting "entry last value on SSR".
>
> Signed-off-by: Chris Lew <chris.lew@....qualcomm.com>
> Signed-off-by: Deepak Kumar Singh <deepak.singh@....qualcomm.com>
> ---
> drivers/soc/qcom/smp2p.c | 41 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index 39628df36745..c35ca7535c14 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -36,6 +36,10 @@
> * The driver uses the Linux GPIO and interrupt framework to expose a virtual
> * GPIO for each outbound entry and a virtual interrupt controller for each
> * inbound entry.
> + *
> + * Driver supports two versions:
> + * V1 - For processor that start after local host
> + * V2 - For processor that start in early boot sequence
> */
>
> #define SMP2P_MAX_ENTRY 16
> @@ -50,11 +54,12 @@
>
> #define ONE 1
> #define TWO 2
> +#define MAX_VERSION TWO
>
> /**
> * struct smp2p_smem_item - in memory communication structure
> * @magic: magic number
> - * @version: version - must be 1
> + * @version: version
> * @features: features flag - currently unused
> * @local_pid: processor id of sending end
> * @remote_pid: processor id of receiving end
> @@ -183,14 +188,23 @@ static void qcom_smp2p_kick(struct qcom_smp2p *smp2p)
> static bool qcom_smp2p_check_ssr(struct qcom_smp2p *smp2p)
> {
> struct smp2p_smem_item *in = smp2p->in;
> + struct smp2p_entry *entry;
> bool restart;
>
> if (!smp2p->ssr_ack_enabled)
> return false;
>
> restart = in->flags & BIT(SMP2P_FLAGS_RESTART_DONE_BIT);
> + restart = restart != smp2p->ssr_ack;
This is hard to read, please try to avoid the immediate reassignment.
> + if (restart && in->version > ONE) {
> + list_for_each_entry(entry, &smp2p->inbound, node) {
> + if (!entry->value)
> + continue;
> + entry->last_value = 0;
Why do we only do this for version 2+?
> + }
> + }
>
> - return restart != smp2p->ssr_ack;
> + return restart;
> }
>
> static void qcom_smp2p_do_ssr_ack(struct qcom_smp2p *smp2p)
> @@ -225,6 +239,20 @@ static void qcom_smp2p_negotiate(struct qcom_smp2p *smp2p)
> }
> }
>
> +static int qcom_smp2p_in_version(struct qcom_smp2p *smp2p)
> +{
> + unsigned int smem_id = smp2p->smem_items[SMP2P_INBOUND];
> + unsigned int pid = smp2p->remote_pid;
> + struct smp2p_smem_item *in;
> + size_t size;
> +
> + in = qcom_smem_get(pid, smem_id, &size);
> + if (IS_ERR(in))
> + return 0;
> +
> + return in->version;
> +}
> +
> static void qcom_smp2p_start_in(struct qcom_smp2p *smp2p)
> {
> unsigned int smem_id = smp2p->smem_items[SMP2P_INBOUND];
> @@ -522,6 +550,7 @@ static int qcom_smp2p_alloc_outbound_item(struct qcom_smp2p *smp2p)
> struct smp2p_smem_item *out;
> unsigned smem_id = smp2p->smem_items[SMP2P_OUTBOUND];
> unsigned pid = smp2p->remote_pid;
> + u8 in_version;
> int ret;
>
> ret = qcom_smem_alloc(pid, smem_id, sizeof(*out));
> @@ -543,12 +572,18 @@ static int qcom_smp2p_alloc_outbound_item(struct qcom_smp2p *smp2p)
> out->valid_entries = 0;
> out->features = SMP2P_ALL_FEATURES;
>
> + in_version = qcom_smp2p_in_version(smp2p);
This is a bit obfuscated in my view, and doesn't seem complete.
We're calling qcom_smp2p_alloc_outbound_item() during probe(), at which
time any non-early booted subsystem will yet to have been launched and
hence they haven't allocated their SMP2P SMEM item.
So in_version will be 0, which is less than 2, so therefor we're running
version 1.
If the subsystem is then brought out of reset and it implements version
2 (we intended it to be launched by bootloader, but it wasn't - this
shouldn't be a problem), we will have a version "mismatch".
Upon first interrupt from the remote we will determine in
qcom_smp2p_negotiate() that we're version 1 and they are version 2, so
we will not complete the negotiation - and thereby not deliver
interrupts.
> + if (in_version > MAX_VERSION) {
> + dev_err(smp2p->dev, "Unsupported smp2p version\n");
I think we can afford ourself to add "...: %d\n", in_version); here. It
would make the error print directly actionable the day we hit it (or
make it obvious if we hit this error due to a bogus in_version).
Regards,
Bjorn
> + return -EINVAL;
> + }
> +
> /*
> * Make sure the rest of the header is written before we validate the
> * item by writing a valid version number.
> */
> wmb();
> - out->version = 1;
> + out->version = (in_version) ? in_version : 1;
>
> qcom_smp2p_kick(smp2p);
>
> --
> 2.34.1
>
Powered by blists - more mailing lists