diff mbox series

[v1,3/5] bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed

Message ID 1589248989-23824-4-git-send-email-hemantk@codeaurora.org (mailing list archive)
State Accepted
Commit ada5e1def5da623cbf5709c921fd22cc321cd233
Headers show
Series MHI Misc Fixes | expand

Commit Message

Hemant Kumar May 12, 2020, 2:03 a.m. UTC 
Driver continues handling of BHI interrupt even if MHI register access
is not allowed. By doing so it calls the status call back and performs
early notification for the MHI client. This is not needed when MHI
register access is not allowed. Hence skip the handling in this case and
return. Also add debug log to print device state, local EE and device EE
when reg access is valid.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Manivannan Sadhasivam May 12, 2020, 6:53 a.m. UTC | #1 
On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
> Driver continues handling of BHI interrupt even if MHI register access
> is not allowed. By doing so it calls the status call back and performs
> early notification for the MHI client. This is not needed when MHI
> register access is not allowed. Hence skip the handling in this case and
> return. Also add debug log to print device state, local EE and device EE
> when reg access is valid.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
>  drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 9ec9b36..467c0ba 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>  	return IRQ_HANDLED;
>  }
>  
> -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
> +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  {
> -	struct mhi_controller *mhi_cntrl = dev;
> +	struct mhi_controller *mhi_cntrl = priv;
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	enum mhi_state state = MHI_STATE_MAX;
>  	enum mhi_pm_state pm_state = 0;
>  	enum mhi_ee_type ee = 0;
>  
>  	write_lock_irq(&mhi_cntrl->pm_lock);
> -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> -		state = mhi_get_mhi_state(mhi_cntrl);
> -		ee = mhi_cntrl->ee;
> -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> +		write_unlock_irq(&mhi_cntrl->pm_lock);

write_lock is only used for protecting 'mhi_cntrl->ee' but here we are not
updating it if reg access is not valid. So there is no reason to hold this lock.

> +		goto exit_intvec;
>  	}
>  
> +	state = mhi_get_mhi_state(mhi_cntrl);
> +	ee = mhi_cntrl->ee;
> +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);

But it is needed here.

Thanks,
Mani

> +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
> +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
> +		TO_MHI_STATE_STR(state));
> +
>  	if (state == MHI_STATE_SYS_ERR) {
> -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
> +		dev_dbg(dev, "System error detected\n");
>  		pm_state = mhi_tryset_pm_state(mhi_cntrl,
>  					       MHI_PM_SYS_ERR_DETECT);
>  	}
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
Hemant Kumar May 13, 2020, 12:28 a.m. UTC | #2 
Hi Mani,

On 5/11/20 11:53 PM, Manivannan Sadhasivam wrote:
> On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
>> Driver continues handling of BHI interrupt even if MHI register access
>> is not allowed. By doing so it calls the status call back and performs
>> early notification for the MHI client. This is not needed when MHI
>> register access is not allowed. Hence skip the handling in this case and
>> return. Also add debug log to print device state, local EE and device EE
>> when reg access is valid.
>>
>> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
>> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 9ec9b36..467c0ba 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
>> +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>>   {
>> -	struct mhi_controller *mhi_cntrl = dev;
>> +	struct mhi_controller *mhi_cntrl = priv;
>> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>   	enum mhi_state state = MHI_STATE_MAX;
>>   	enum mhi_pm_state pm_state = 0;
>>   	enum mhi_ee_type ee = 0;
>>   
>>   	write_lock_irq(&mhi_cntrl->pm_lock);
>> -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>> -		state = mhi_get_mhi_state(mhi_cntrl);
>> -		ee = mhi_cntrl->ee;
>> -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
>> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>> +		write_unlock_irq(&mhi_cntrl->pm_lock);
> 
> write_lock is only used for protecting 'mhi_cntrl->ee' but here we are not
> updating it if reg access is not valid. So there is no reason to hold this lock.
Original code is using write_lock to protect pm_state as well as 
mhi_cntrl->ee. This patch is keeping the lock same as original code. 
Just if condition logic is negated here due to that write_unlock_irq is 
added under if condition.
> 
>> +		goto exit_intvec;
>>   	}
>>   
>> +	state = mhi_get_mhi_state(mhi_cntrl);
>> +	ee = mhi_cntrl->ee;
>> +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> 
> But it is needed here.
> 
> Thanks,
> Mani
> 
>> +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
>> +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
>> +		TO_MHI_STATE_STR(state));
>> +
>>   	if (state == MHI_STATE_SYS_ERR) {
>> -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
>> +		dev_dbg(dev, "System error detected\n");
>>   		pm_state = mhi_tryset_pm_state(mhi_cntrl,
>>   					       MHI_PM_SYS_ERR_DETECT);
>>   	}
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
Manivannan Sadhasivam May 13, 2020, 7:04 a.m. UTC | #3 
On Tue, May 12, 2020 at 05:28:45PM -0700, Hemant Kumar wrote:
> Hi Mani,
> 
> On 5/11/20 11:53 PM, Manivannan Sadhasivam wrote:
> > On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
> > > Driver continues handling of BHI interrupt even if MHI register access
> > > is not allowed. By doing so it calls the status call back and performs
> > > early notification for the MHI client. This is not needed when MHI
> > > register access is not allowed. Hence skip the handling in this case and
> > > return. Also add debug log to print device state, local EE and device EE
> > > when reg access is valid.
> > > 
> > > Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> > > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
> > > ---
> > >   drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
> > >   1 file changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > index 9ec9b36..467c0ba 100644
> > > --- a/drivers/bus/mhi/core/main.c
> > > +++ b/drivers/bus/mhi/core/main.c
> > > @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
> > >   	return IRQ_HANDLED;
> > >   }
> > > -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
> > > +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
> > >   {
> > > -	struct mhi_controller *mhi_cntrl = dev;
> > > +	struct mhi_controller *mhi_cntrl = priv;
> > > +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > >   	enum mhi_state state = MHI_STATE_MAX;
> > >   	enum mhi_pm_state pm_state = 0;
> > >   	enum mhi_ee_type ee = 0;
> > >   	write_lock_irq(&mhi_cntrl->pm_lock);
> > > -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> > > -		state = mhi_get_mhi_state(mhi_cntrl);
> > > -		ee = mhi_cntrl->ee;
> > > -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> > > +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> > > +		write_unlock_irq(&mhi_cntrl->pm_lock);
> > 
> > write_lock is only used for protecting 'mhi_cntrl->ee' but here we are not
> > updating it if reg access is not valid. So there is no reason to hold this lock.
> Original code is using write_lock to protect pm_state as well as
> mhi_cntrl->ee. This patch is keeping the lock same as original code. Just if
> condition logic is negated here due to that write_unlock_irq is added under
> if condition.

'mhi_cntrl->pm_state' is not always protected by 'pm_lock' and that too
write_lock is used here but 'pm_state' is not modified. So as like in most of
the places, locks are abused here as well.

I think after 5.8, you should really think about fixing the usage of locks
throughout the MHI stack.

So I'll take this patch as it is.

Thanks,
Mani

> > 
> > > +		goto exit_intvec;
> > >   	}
> > > +	state = mhi_get_mhi_state(mhi_cntrl);
> > > +	ee = mhi_cntrl->ee;
> > > +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> > 
> > But it is needed here.
> > 
> > Thanks,
> > Mani
> > 
> > > +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
> > > +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
> > > +		TO_MHI_STATE_STR(state));
> > > +
> > >   	if (state == MHI_STATE_SYS_ERR) {
> > > -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
> > > +		dev_dbg(dev, "System error detected\n");
> > >   		pm_state = mhi_tryset_pm_state(mhi_cntrl,
> > >   					       MHI_PM_SYS_ERR_DETECT);
> > >   	}
> > > -- 
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
Manivannan Sadhasivam May 13, 2020, 7:58 a.m. UTC | #4 
On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
> Driver continues handling of BHI interrupt even if MHI register access
> is not allowed. By doing so it calls the status call back and performs
> early notification for the MHI client. This is not needed when MHI
> register access is not allowed. Hence skip the handling in this case and
> return. Also add debug log to print device state, local EE and device EE
> when reg access is valid.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 9ec9b36..467c0ba 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>  	return IRQ_HANDLED;
>  }
>  
> -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
> +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  {
> -	struct mhi_controller *mhi_cntrl = dev;
> +	struct mhi_controller *mhi_cntrl = priv;
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	enum mhi_state state = MHI_STATE_MAX;
>  	enum mhi_pm_state pm_state = 0;
>  	enum mhi_ee_type ee = 0;
>  
>  	write_lock_irq(&mhi_cntrl->pm_lock);
> -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> -		state = mhi_get_mhi_state(mhi_cntrl);
> -		ee = mhi_cntrl->ee;
> -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> +		write_unlock_irq(&mhi_cntrl->pm_lock);
> +		goto exit_intvec;
>  	}
>  
> +	state = mhi_get_mhi_state(mhi_cntrl);
> +	ee = mhi_cntrl->ee;
> +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
> +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
> +		TO_MHI_STATE_STR(state));
> +
>  	if (state == MHI_STATE_SYS_ERR) {
> -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
> +		dev_dbg(dev, "System error detected\n");
>  		pm_state = mhi_tryset_pm_state(mhi_cntrl,
>  					       MHI_PM_SYS_ERR_DETECT);
>  	}
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
Hemant Kumar May 13, 2020, 6:39 p.m. UTC | #5 
Hi Mani,

On 5/13/20 12:04 AM, Manivannan Sadhasivam wrote:
> On Tue, May 12, 2020 at 05:28:45PM -0700, Hemant Kumar wrote:
>> Hi Mani,
>>
>> On 5/11/20 11:53 PM, Manivannan Sadhasivam wrote:
>>> On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
>>>> Driver continues handling of BHI interrupt even if MHI register access
>>>> is not allowed. By doing so it calls the status call back and performs
>>>> early notification for the MHI client. This is not needed when MHI
>>>> register access is not allowed. Hence skip the handling in this case and
>>>> return. Also add debug log to print device state, local EE and device EE
>>>> when reg access is valid.
>>>>
>>>> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
>>>> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
>>>> ---
>>>>    drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
>>>>    1 file changed, 14 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>>>> index 9ec9b36..467c0ba 100644
>>>> --- a/drivers/bus/mhi/core/main.c
>>>> +++ b/drivers/bus/mhi/core/main.c
>>>> @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>>>>    	return IRQ_HANDLED;
>>>>    }
>>>> -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
>>>> +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>>>>    {
>>>> -	struct mhi_controller *mhi_cntrl = dev;
>>>> +	struct mhi_controller *mhi_cntrl = priv;
>>>> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>>>    	enum mhi_state state = MHI_STATE_MAX;
>>>>    	enum mhi_pm_state pm_state = 0;
>>>>    	enum mhi_ee_type ee = 0;
>>>>    	write_lock_irq(&mhi_cntrl->pm_lock);
>>>> -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>>>> -		state = mhi_get_mhi_state(mhi_cntrl);
>>>> -		ee = mhi_cntrl->ee;
>>>> -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
>>>> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>>>> +		write_unlock_irq(&mhi_cntrl->pm_lock);
>>>
>>> write_lock is only used for protecting 'mhi_cntrl->ee' but here we are not
>>> updating it if reg access is not valid. So there is no reason to hold this lock.
>> Original code is using write_lock to protect pm_state as well as
>> mhi_cntrl->ee. This patch is keeping the lock same as original code. Just if
>> condition logic is negated here due to that write_unlock_irq is added under
>> if condition.
> 
> 'mhi_cntrl->pm_state' is not always protected by 'pm_lock' and that too
> write_lock is used here but 'pm_state' is not modified. So as like in most of
> the places, locks are abused here as well.
> 
> I think after 5.8, you should really think about fixing the usage of locks
> throughout the MHI stack.
> 
> So I'll take this patch as it is.
> 
Thanks for accepting this patch. Will revisit and review usage of locks 
in MHI stack and identify areas of improvement.
> Thanks,
> Mani
> 
>>>
>>>> +		goto exit_intvec;
>>>>    	}
>>>> +	state = mhi_get_mhi_state(mhi_cntrl);
>>>> +	ee = mhi_cntrl->ee;
>>>> +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
>>>
>>> But it is needed here.
>>>
>>> Thanks,
>>> Mani
>>>
>>>> +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
>>>> +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
>>>> +		TO_MHI_STATE_STR(state));
>>>> +
>>>>    	if (state == MHI_STATE_SYS_ERR) {
>>>> -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
>>>> +		dev_dbg(dev, "System error detected\n");
>>>>    		pm_state = mhi_tryset_pm_state(mhi_cntrl,
>>>>    					       MHI_PM_SYS_ERR_DETECT);
>>>>    	}
>>>> -- 
>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>>> a Linux Foundation Collaborative Project
>>
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
diff mbox series

Patch

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 9ec9b36..467c0ba 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -369,22 +369,29 @@  irqreturn_t mhi_irq_handler(int irq_number, void *dev)
 	return IRQ_HANDLED;
 }
 
-irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
+irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
 {
-	struct mhi_controller *mhi_cntrl = dev;
+	struct mhi_controller *mhi_cntrl = priv;
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 	enum mhi_state state = MHI_STATE_MAX;
 	enum mhi_pm_state pm_state = 0;
 	enum mhi_ee_type ee = 0;
 
 	write_lock_irq(&mhi_cntrl->pm_lock);
-	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
-		state = mhi_get_mhi_state(mhi_cntrl);
-		ee = mhi_cntrl->ee;
-		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
+	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
+		write_unlock_irq(&mhi_cntrl->pm_lock);
+		goto exit_intvec;
 	}
 
+	state = mhi_get_mhi_state(mhi_cntrl);
+	ee = mhi_cntrl->ee;
+	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
+	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
+		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
+		TO_MHI_STATE_STR(state));
+
 	if (state == MHI_STATE_SYS_ERR) {
-		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
+		dev_dbg(dev, "System error detected\n");
 		pm_state = mhi_tryset_pm_state(mhi_cntrl,
 					       MHI_PM_SYS_ERR_DETECT);
 	}