Re: [PATCH v3 04/12] bus: mhi: core: Move to SYS_ERROR regardless of RDDM capability

[Bhaumik Bhatt]

Hi Mani,

On 2020-10-30 06:52, Manivannan Sadhasivam wrote:
> On Thu, Oct 29, 2020 at 09:10:49PM -0700, Bhaumik Bhatt wrote:
> > In some cases, the entry of device to RDDM execution environment
> > can occur after a significant amount of time has elapsed and a
> > SYS_ERROR state change event has already arrived.
>
> I don't quite understand this statement. Can you elaborate? This 
> doesn't
> relate to what the patch is doing.
So the mhi_intvec_threaded_handler() (BHI) MSI that fires to switch the 
EE
to RDDM may come much later than the SYS_ERROR state change event from 
the
control event ring. We currently, do not move to MHI_PM_SYS_ERR_DETECT
state if RDDM is supported i.e. mhi_cntrl->rddm_image is set. However, 
it
means that we will remain in an "active" MHI PM state for the duration 
of
time until RDDM EE (BHI) MSI comes in. We have seen it take 5 seconds in
some bad cases.
> > This can result
> > in scenarios where MHI controller and client drivers are unaware
> > of the error state of the device. Remove the check for rddm_image
> > when processing the SYS_ERROR state change as it is present in
> > mhi_pm_sys_err_handler() already and prevent further activity
> > until the expected RDDM execution environment change occurs or
> > the controller driver decides further action.
> >
> > Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> > ---
> >  drivers/bus/mhi/core/main.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > index 2cff5dd..1f32d67 100644
> > --- a/drivers/bus/mhi/core/main.c
> > +++ b/drivers/bus/mhi/core/main.c
> > @@ -733,19 +733,15 @@ int mhi_process_ctrl_ev_ring(struct 
> > mhi_controller *mhi_cntrl,
> >  				break;
> >  			case MHI_STATE_SYS_ERR:
> >  			{
> > -				enum mhi_pm_state new_state;
> > -
> > -				/* skip SYS_ERROR handling if RDDM supported */
> > -				if (mhi_cntrl->ee == MHI_EE_RDDM ||
> > -				    mhi_cntrl->rddm_image)
> > -					break;
> > +				enum mhi_pm_state state = MHI_PM_STATE_MAX;
> >
> >  				dev_dbg(dev, "System error detected\n");
> >  				write_lock_irq(&mhi_cntrl->pm_lock);
> > -				new_state = mhi_tryset_pm_state(mhi_cntrl,
> > +				if (mhi_cntrl->ee != MHI_EE_RDDM)
>
> But you are still checking for RDDM EE?
>
> Please explain why you want to skip RDDM check.
>
> Thanks,
> Mani
Yes, the point is to only remove the mhi_cntrl->rddm_image check but 
still
retain the "has EE moved to become RDDM" check. This allows us to avoid 
any
extra processing of moving states to MHI_PM_SYS_ERR_DETECT state if it 
may
be unnecessary (EE already changed to RDDM). The mhi_cntrl->rddm_image 
is
also present in mhi_pm_sys_err_handler(mhi_cntrl) function so it is not
needed here.
> > +					state = mhi_tryset_pm_state(mhi_cntrl,
> >  							MHI_PM_SYS_ERR_DETECT);
> >  				write_unlock_irq(&mhi_cntrl->pm_lock);
> > -				if (new_state == MHI_PM_SYS_ERR_DETECT)
> > +				if (state == MHI_PM_SYS_ERR_DETECT)
> >  					mhi_pm_sys_err_handler(mhi_cntrl);
> >  				break;
> >  			}
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
> > Forum,
> > a Linux Foundation Collaborative Project

This is why I mention the word RDDM "capability". If controller supports 
RDDM
is not enough to skip the move to MHI_PM_SYS_ERR_DETECT state as it is 
safer
to move and stop client drivers from pushing data.

Thanks,
Bhaumik
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project