diff mbox series

[v5,6/9] bus: mhi: core: Check channel execution environment before issuing reset

Message ID 1610139297-36435-7-git-send-email-bbhatt@codeaurora.org
State Superseded
Headers show
Series None | expand

Commit Message

Bhaumik Bhatt Jan. 8, 2021, 8:54 p.m. UTC
A client can attempt to unprepare certain channels for transfer even
after the execution environment they are supposed to run in has changed.
In the event that happens, the device need not be notified of the reset
and the host can proceed with clean up for the channel context and
memory allocated for it on the host as the device will no longer be able
to respond to such a request.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/core/main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Manivannan Sadhasivam Jan. 21, 2021, 3:12 p.m. UTC | #1
On Fri, Jan 08, 2021 at 12:54:54PM -0800, Bhaumik Bhatt wrote:
> A client can attempt to unprepare certain channels for transfer even

> after the execution environment they are supposed to run in has changed.


Just out of curiosity, under what circumstances it can happen? We don't
have any in kernel user for the mhi_unprepare_from_transfer() API :/

> In the event that happens, the device need not be notified of the reset

> and the host can proceed with clean up for the channel context and

> memory allocated for it on the host as the device will no longer be able

> to respond to such a request.

> 

> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>

> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

> ---

>  drivers/bus/mhi/core/main.c | 13 +++++++++++++

>  1 file changed, 13 insertions(+)

> 

> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c

> index a84e6aa..ec720fe 100644

> --- a/drivers/bus/mhi/core/main.c

> +++ b/drivers/bus/mhi/core/main.c

> @@ -1351,11 +1351,24 @@ static void __mhi_unprepare_channel(struct mhi_controller *mhi_cntrl,

>  	/* no more processing events for this channel */

>  	mutex_lock(&mhi_chan->mutex);

>  

> +	if (!(BIT(mhi_cntrl->ee) & mhi_chan->ee_mask)) {

> +		dev_err(dev,

> +			"Current EE: %s Required EE Mask: 0x%x for chan: %s\n",

> +			TO_MHI_EXEC_STR(mhi_cntrl->ee), mhi_chan->ee_mask,

> +			mhi_chan->name);


Again, use channel's struct dev here.

Thanks,
Mani
Bhaumik Bhatt Feb. 4, 2021, 8:23 p.m. UTC | #2
Hi Mani,

On 2021-01-21 07:12 AM, Manivannan Sadhasivam wrote:
> On Fri, Jan 08, 2021 at 12:54:54PM -0800, Bhaumik Bhatt wrote:

>> A client can attempt to unprepare certain channels for transfer even

>> after the execution environment they are supposed to run in has 

>> changed.

> 

> Just out of curiosity, under what circumstances it can happen? We don't

> have any in kernel user for the mhi_unprepare_from_transfer() API :/

> 

We do and it's the qrtr-mhi driver where we can potentially see this 
happen.
Mainly, since they want to undo whatever they have done in probe(), this 
can
come from the remove() after MHI power down is initiated.

I have updated this patch in upcoming v6 series with a debug message as 
we only
want to proceed with clean-up directly.

>> In the event that happens, the device need not be notified of the 

>> reset

>> and the host can proceed with clean up for the channel context and

>> memory allocated for it on the host as the device will no longer be 

>> able

>> to respond to such a request.

>> 

>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>

>> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

>> ---

>>  drivers/bus/mhi/core/main.c | 13 +++++++++++++

>>  1 file changed, 13 insertions(+)

>> 

>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c

>> index a84e6aa..ec720fe 100644

>> --- a/drivers/bus/mhi/core/main.c

>> +++ b/drivers/bus/mhi/core/main.c

>> @@ -1351,11 +1351,24 @@ static void __mhi_unprepare_channel(struct 

>> mhi_controller *mhi_cntrl,

>>  	/* no more processing events for this channel */

>>  	mutex_lock(&mhi_chan->mutex);

>> 

>> +	if (!(BIT(mhi_cntrl->ee) & mhi_chan->ee_mask)) {

>> +		dev_err(dev,

>> +			"Current EE: %s Required EE Mask: 0x%x for chan: %s\n",

>> +			TO_MHI_EXEC_STR(mhi_cntrl->ee), mhi_chan->ee_mask,

>> +			mhi_chan->name);

> 

> Again, use channel's struct dev here.

> 

Done.
> Thanks,

> Mani


Thanks,
Bhaumik
---
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 a84e6aa..ec720fe 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -1351,11 +1351,24 @@  static void __mhi_unprepare_channel(struct mhi_controller *mhi_cntrl,
 	/* no more processing events for this channel */
 	mutex_lock(&mhi_chan->mutex);
 
+	if (!(BIT(mhi_cntrl->ee) & mhi_chan->ee_mask)) {
+		dev_err(dev,
+			"Current EE: %s Required EE Mask: 0x%x for chan: %s\n",
+			TO_MHI_EXEC_STR(mhi_cntrl->ee), mhi_chan->ee_mask,
+			mhi_chan->name);
+		goto exit_unprepare_channel;
+	}
+
 	ret = mhi_update_channel_state(mhi_cntrl, mhi_chan,
 				       MHI_CH_STATE_TYPE_RESET);
 	if (ret)
 		dev_err(dev, "Failed to reset channel, still resetting\n");
 
+exit_unprepare_channel:
+	write_lock_irq(&mhi_chan->lock);
+	mhi_chan->ch_state = MHI_CH_STATE_DISABLED;
+	write_unlock_irq(&mhi_chan->lock);
+
 	if (!mhi_chan->offload_ch) {
 		mhi_reset_chan(mhi_cntrl, mhi_chan);
 		mhi_deinit_chan_ctxt(mhi_cntrl, mhi_chan);