Re: [PATCH v12 04/36] remoteproc: k3-m4: Don't assert reset in detach routine

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Sat, May 17, 2025 at 06:53:29PM +0530, Beleswar Prasad Padhi wrote:
> 
> On 5/16/2025 9:15 PM, Mathieu Poirier wrote:
> > On Tue, May 13, 2025 at 11:14:38AM +0530, Beleswar Padhi wrote:
> > > The rproc_detach() function invokes __rproc_detach() before
> > > rproc_unprepare_device(). The __rproc_detach() function sets the
> > > rproc->state to "RPROC_DETACHED".
> > > 
> > > However, the TI K3 M4 driver erroneously looks for "RPROC_ATTACHED"
> > > state in its .unprepare ops to identify IPC-only mode; which leads to
> > > resetting the rproc in detach routine.
> > > 
> > > Therefore, correct the IPC-only mode detection logic to look for
> > > "RPROC_DETACHED" in k3_m4_rproc_unprepare() function.
> > > 
> > This driver has been upstream for 9 whole months, it is hard for me to believe
> > this but was just noticed.  Martyn from Collabora should be CC'ed on this, and I
> > will also need the required R-b/T-b tags.
> 
> 
> Cc: Martyn Welch martyn.welch@collabora.com
> 
> Requesting Andrew/Judith for review and test too.
> 
> > 
> > Typically bug fixes are not part of refactoring exercises.
> 
> 
> Typically, yes. But the refactor depends on this fix. This
> k3_m4_rproc_unprepare() function is entirely refactored to common driver in
> [PATCH v12 26/36].
>
> So, If the refactor is picked without this patch fix, the mainline M4 driver
> would be fixed, but the older stable kernels would always have this bug. Let
> me know what you think.
> 

I suggest you send this patch on its own and then the series (without this
patch) with a note in the cover letter that it depends on the fix.  That way we
get the best of both worlds.

> Thanks,
> Beleswar
> 
> >   I suggest to apply
> > this set without this patch - you can then work on fixing this bug.
> > 
> > Thanks,
> > Mathieu
> > 
> > > Fixes: ebcf9008a895 ("remoteproc: k3-m4: Add a remoteproc driver for M4F subsystem")
> > > Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> > > ---
> > > v12: Changelog:
> > > 1. New patch. Fixup a state detection logic.
> > > 
> > >   drivers/remoteproc/ti_k3_m4_remoteproc.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c b/drivers/remoteproc/ti_k3_m4_remoteproc.c
> > > index a16fb165fcedd..6cd50b16a8e82 100644
> > > --- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
> > > +++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
> > > @@ -228,7 +228,7 @@ static int k3_m4_rproc_unprepare(struct rproc *rproc)
> > >   	int ret;
> > >   	/* If the core is going to be detached do not assert the module reset */
> > > -	if (rproc->state == RPROC_ATTACHED)
> > > +	if (rproc->state == RPROC_DETACHED)
> > >   		return 0;
> > >   	ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
> > > -- 
> > > 2.34.1
> > >