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. 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 <[email protected]>
---
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)
+ 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
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.
> 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 <[email protected]>
> ---
> 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
> + 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
>
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 <[email protected]>
>> ---
>> 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