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.we...@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-pa...@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
> > >
