linux-kernel - Re: [PATCH 1/2] soc: qcom: smp2p: Add irqchip state support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOdFzcigkaMF0vC6mGoJ4PKzPuJS2K=C8ghSjbfALAj+GvSTaA@mail.gmail.com>
Date: Wed, 22 Oct 2025 18:05:11 -0700
From: Christopher Lew <christopher.lew@....qualcomm.com>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Deepak Kumar Singh <quic_deesin@...cinc.com>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        Jingyi Wang <jingyi.wang@....qualcomm.com>,
        Konrad Dybcio <konradybcio@...nel.org>, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Chris Lew <chris.lew@....qualcomm.com>
Subject: Re: [PATCH 1/2] soc: qcom: smp2p: Add irqchip state support

On Wed, Oct 22, 2025 at 3:10 PM Bjorn Andersson <andersson@...nel.org> wrote:
>
> On Wed, Oct 22, 2025 at 04:27:28PM +0530, Deepak Kumar Singh wrote:
> >
> >
> > On 10/21/2025 3:05 PM, Konrad Dybcio wrote:
> > > On 10/21/25 10:12 AM, Deepak Kumar Singh wrote:
> > > >
> > > >
> > > > On 9/24/2025 8:20 PM, Konrad Dybcio wrote:
> > > > > On 9/24/25 6:18 AM, Jingyi Wang wrote:
> > > > > > From: Chris Lew <chris.lew@....qualcomm.com>
> > > > > >
> > > > > > A remoteproc booted during earlier boot stages such as UEFI or the
> > > > > > bootloader, may need to be attached to without restarting the remoteproc
> > > > > > hardware. To do this the remoteproc will need to check the ready and
> > > > > > handover states in smp2p without an interrupt notification.
> > > > > >
> > > > > > Add support for the .irq_get_irqchip_state callback so remoteproc can
> > > > > > read the current state of the fatal, ready and handover bits.
> > > > > >
> > > > > > Signed-off-by: Chris Lew <chris.lew@....qualcomm.com>
> > > > > > Co-developed-by: Jingyi Wang <jingyi.wang@....qualcomm.com>
> > > > > > Signed-off-by: Jingyi Wang <jingyi.wang@....qualcomm.com>
> > > > > > ---
> > > > > >    drivers/soc/qcom/smp2p.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >    1 file changed, 55 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> > > > > > index cb515c2340c1..e2cfd9ec8875 100644
> > > > > > --- a/drivers/soc/qcom/smp2p.c
> > > > > > +++ b/drivers/soc/qcom/smp2p.c
> > > > > > @@ -222,6 +222,39 @@ static void qcom_smp2p_negotiate(struct qcom_smp2p *smp2p)
> > > > > >        }
> > > > > >    }
> > > > > >    +static void qcom_smp2p_start_in(struct qcom_smp2p *smp2p)
> > > > > > +{
> > > > > > +    unsigned int smem_id = smp2p->smem_items[SMP2P_INBOUND];
> > > > > > +    unsigned int pid = smp2p->remote_pid;
> > > > > > +    char buf[SMP2P_MAX_ENTRY_NAME];
> > > > > > +    struct smp2p_smem_item *in;
> > > > > > +    struct smp2p_entry *entry;
> > > > > > +    size_t size;
> > > > > > +    int i;
> > > > > > +
> > > > > > +    in = qcom_smem_get(pid, smem_id, &size);
> > > > > > +    if (IS_ERR(in))
> > > > > > +        return;
> > > > > > +
> > > > > > +    smp2p->in = in;
> > > > > > +
> > > > > > +    /* Check if version is initialized and set to v2 */
> > > > > > +    if (in->version == 0)
> > > > > > +        return;
> > > > >
> > > > > This doesn't seem to be fully in line with the comment
> > > > >
> > > > > Konrad
> > > > >
> > > > Hi Konard,
> > > >
> > > > Can you please elaborate more on this?
> > > > in->version == 0 means remote has not initialized the version yet, so no need of enumerating entries. For other case i.e in->version == 1 or 2, in entries added by early booted remote has to be enumerated.
> > >
> > > It's not at all obvious that 0 is supposed to mean "uninitialized"
> > >
> > > Please #define it
> > >
> > > Konrad
> > I think that can be added or instead we can replace (in->version == 0 )with
> > (in->version != SMP2P_VERSION_2).
> >
>
> I agree with Konrad regarding the discrepancy between comment and code,
> "Initialized and set to 2" means specifically version 2, while checking
> against 0 means "any of the remaining 255 possible values.
>
> I don't think we need a define for the version number 2.
>
>
> But we most definitely need a comment about why the remainder shouldn't
> be executed for all other (initialized) versions. Today, specifically,
> why isn't this code valid for version 1?
>

I think I had made an assumption that the processors still supporting
V1 were remoteproc managed processors, for those processors this check
would always return early because those remoteprocs boot after smp2p
probes.
If this code somehow executed on a v1 edge, then we would most likely
miss some notifications. That's abnormal behavior so let's change the
check to explicitly look for 2.

Thanks,
Chris

> Regards,
> Bjorn