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: <xfzzf3vebav3vhwe6n2vd6j2s3nhu2fgnoqkcalhrnhmi25wxw@zghvkcopsz54>
Date: Wed, 22 Oct 2025 17:13:04 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Deepak Kumar Singh <quic_deesin@...cinc.com>
Cc: 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 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?

Regards,
Bjorn