Re: [PATCH v6 5/6] remoteproc: qcom: pas: Add late attach support for subsystems

[Mukesh Ojha]

On Wed, Jun 10, 2026 at 11:11:59AM +0800, Jingyi Wang wrote:
> 
> 
> On 5/21/2026 7:22 PM, Mukesh Ojha wrote:
> > On Thu, May 21, 2026 at 11:42:49AM +0800, Jingyi Wang wrote:
> > > 
> > > 
> > > On 5/20/2026 4:27 PM, Mukesh Ojha wrote:
> > > > On Tue, May 19, 2026 at 12:24:23AM -0700, Jingyi Wang wrote:
> > > > > Subsystems can be brought out of reset by entities such as bootloaders.
> > > > > As the irq enablement could be later than subsystem bring up, the state
> > > > > of subsystem should be checked by reading SMP2P bits.
> > > > > 
> > > > > A new qcom_pas_attach() function is introduced. if a crash state is
> > > > > detected for the subsystem, rproc_report_crash() is called. If the ready
> > > > > state is detected, it will be marked as "attached", otherwise it could
> > > > > be the early boot feature is not supported by other entities. In this
> > > > > case, the state will be marked as RPROC_OFFLINE so that the PAS driver
> > > > > can load the firmware and start the remoteproc.
> > > > > 
> > > > > Co-developed-by: Gokul Krishna Krishnakumar <gokul.krishnakumar@xxxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Gokul Krishna Krishnakumar <gokul.krishnakumar@xxxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Jingyi Wang <jingyi.wang@xxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >    drivers/remoteproc/qcom_q6v5_pas.c | 58 ++++++++++++++++++++++++++++++++++++++
> > > > >    1 file changed, 58 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > index da27d1d3c9da..ac2a00aacd2e 100644
> > > > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > @@ -60,6 +60,7 @@ struct qcom_pas_data {
> > > > >    	int region_assign_count;
> > > > >    	bool region_assign_shared;
> > > > >    	int region_assign_vmid;
> > > > > +	bool early_boot;
> > > > >    };
> > > > >    struct qcom_pas {
> > > > > @@ -510,6 +511,57 @@ static unsigned long qcom_pas_panic(struct rproc *rproc)
> > > > >    	return qcom_q6v5_panic(&pas->q6v5);
> > > > >    }
> > > > > +static int qcom_pas_attach(struct rproc *rproc)
> > > > > +{
> > > > > +	int ret;
> > > > > +	struct qcom_pas *pas = rproc->priv;
> > > > > +	bool ready_state;
> > > > > +	bool crash_state;
> > > > > +
> > > > > +	pas->q6v5.handover_issued = true;
> > > > > +	enable_irq(pas->q6v5.handover_irq);
> > > > > +
> > > > > +	pas->q6v5.running = true;
> > > > > +	ret = irq_get_irqchip_state(pas->q6v5.fatal_irq,
> > > > > +				    IRQCHIP_STATE_LINE_LEVEL, &crash_state);
> > > > > +
> > > > > +	if (ret)
> > > > > +		goto disable_running;
> > > > > +
> > > > > +	if (crash_state) {
> > > > > +		dev_err(pas->dev, "Subsystem has crashed before driver probe\n");
> > > > > +		rproc_report_crash(rproc, RPROC_FATAL_ERROR);
> > > > 
> > > > I am not sure if this is already discussed, but what if it is the first
> > > > crash with recovery and coredump enabled? What would be in the dump,
> > > > nothing? As there is no segment, is it expected since Linux did not load
> > > > this?
> > > > 
> > > > This is even true if it is a crash after a successful attach.
> > > > 
> > > 
> > > It is suggested by Bjorn:
> > > https://lore.kernel.org/all/qfls6xlvfppqw7p6rjpmzqesh6sbob4myfc6dz47qh3jywqrjk@5xiutkbybk5d/
> > > 
> > > I did a hack to test the recovery by setting crash_state true, it can recovery
> > > (stop and start) successfully with below patches:
> > > https://lore.kernel.org/all/20260519-rproc-attach-issue-v2-0-caa1eaf75081@xxxxxxxxxxxxxxxx/
> > > 
> > > For coredump, it will return from the first "list_empty(&rproc->dump_segments)" check in
> > > rproc_coredump as segments are not configured in attach.
> > 
> > 
> > I was not against any of the stuff, but mostly checking, if we agreed on not collecting dump
> > for first crash when soccp minidump is not initialized which falls back to full dump of the soccp.
> > I see soccp minidump id in the downstream but we have not added in 6/6.
> > 
> 
> Hi Mukesh,
> 
> I prefer to add base rproc attach feature only in this patch and skipping the coredump in attach
> workflow.

Should be fine..

> 
> Thanks,
> Jingyi
> 
> > > 
> > > Thanks,
> > > Jingyi
> > > 
> > > 
> > > > @Sibi, has this series been tested on Glymur with KVM?
> > > > I don't see the iommu property in the below patch.
> > > > https://lore.kernel.org/lkml/20260403-glymur-soccp-v3-1-f0e8d57f11ba@xxxxxxxxxxxxxxxx/
> > > > 
> > > 
> > 
> 

-- 
-Mukesh Ojha