HI Bhaumik,
On Sat, 10 Oct 2020 at 02:23, <bbhatt@xxxxxxxxxxxxxx> wrote:
>
> On 2020-10-09 02:07, Loic Poulain wrote:
> > Some MHI device drivers need to stop the channels in their driver
> > remove callback (e.g. module unloading), but the unprepare function
> > is aborted because MHI core moved the channels to suspended state
> > prior calling driver remove callback. This prevents the driver to
> > send a proper MHI RESET CHAN command to the device. Device is then
> > unaware of the stopped state of these channels.
> >
> > This causes issue when driver tries to start the channels again (e.g.
> > module is reloaded), since device considers channels as already
> > started (inconsistent state).
> >
> > Fix this by allowing channel reset when channel is suspended.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxx>
> > ---
> > drivers/bus/mhi/core/main.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > index d20967a..a588eac 100644
> > --- a/drivers/bus/mhi/core/main.c
> > +++ b/drivers/bus/mhi/core/main.c
> > @@ -1232,7 +1232,8 @@ static void __mhi_unprepare_channel(struct
> > mhi_controller *mhi_cntrl,
> > /* no more processing events for this channel */
> > mutex_lock(&mhi_chan->mutex);
> > write_lock_irq(&mhi_chan->lock);
> > - if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) {
> > + if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED &&
> > + mhi_chan->ch_state != MHI_CH_STATE_SUSPENDED) {
> > write_unlock_irq(&mhi_chan->lock);
> > mutex_unlock(&mhi_chan->mutex);
> > return;
> Hi Loic,
>
> There should not be any reason for drivers to do an "unprepare" and send
> a reset channel
> command during remove, as the channel context gets cleaned up after the
> remove callback
> returns.
Well, a good practice is to have a balanced interface, and everything we do in
probe() should be undoable in remove(). Here we start the channel in probe()
and explicitly stop them in remove(), So I think doing unprepare in
remove should
work anyway, even if the MHI stack does some cleanup on its own.
>
>
> We do not want to allow moving from MHI_CH_STATE_SUSPENDED to
> MHI_CH_STATE_DISABLED state
> because if a remove is called, channel context being cleaned up implies
> a reset.
AFAIK today, no reset command is sent on remove.
>
> Also, I have a bunch of channel state machine related patches coming up
> soon which solve
> this issue and more. We are also introducing some missing features with
> that.
>
> It would be nice if you can review/comment on those as it overhauls the
> state machine.
Sure, feel free to submit.
Regards,
Loic
