stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] bus: mhi: ep: Change state_lock to mutex
@ 2023-01-23  7:50 Manivannan Sadhasivam
  2023-01-23 17:10 ` Jeffrey Hugo
  2023-01-27  6:58 ` Manivannan Sadhasivam
  0 siblings, 2 replies; 3+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-23  7:50 UTC (permalink / raw)
  To: mhi
  Cc: linux-arm-msm, linux-kernel, Manivannan Sadhasivam, stable,
	Dan Carpenter

state_lock, the spinlock type is meant to protect race against concurrent
MHI state transitions. In mhi_ep_set_m0_state(), while the state_lock is
being held, the channels are resumed in mhi_ep_resume_channels() if the
previous state was M3. This causes sleeping in atomic bug, since
mhi_ep_resume_channels() use mutex internally.

Since the state_lock is supposed to be held throughout the state change,
it is not ideal to drop the lock before calling mhi_ep_resume_channels().
So to fix this issue, let's change the type of state_lock to mutex. This
would also allow holding the lock throughout all state transitions thereby
avoiding any potential race.

Cc: <stable@vger.kernel.org> # 5.19
Fixes: e4b7b5f0f30a ("bus: mhi: ep: Add support for suspending and resuming channels")
Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/ep/main.c |  8 +++++---
 drivers/bus/mhi/ep/sm.c   | 42 ++++++++++++++++++++++-----------------
 include/linux/mhi_ep.h    |  4 ++--
 3 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index bcaaba97ef63..528c00b232bf 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -1001,11 +1001,11 @@ static void mhi_ep_reset_worker(struct work_struct *work)
 
 	mhi_ep_power_down(mhi_cntrl);
 
-	spin_lock_bh(&mhi_cntrl->state_lock);
+	mutex_lock(&mhi_cntrl->state_lock);
+
 	/* Reset MMIO to signal host that the MHI_RESET is completed in endpoint */
 	mhi_ep_mmio_reset(mhi_cntrl);
 	cur_state = mhi_cntrl->mhi_state;
-	spin_unlock_bh(&mhi_cntrl->state_lock);
 
 	/*
 	 * Only proceed further if the reset is due to SYS_ERR. The host will
@@ -1014,6 +1014,8 @@ static void mhi_ep_reset_worker(struct work_struct *work)
 	 */
 	if (cur_state == MHI_STATE_SYS_ERR)
 		mhi_ep_power_up(mhi_cntrl);
+
+	mutex_unlock(&mhi_cntrl->state_lock);
 }
 
 /*
@@ -1386,8 +1388,8 @@ int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl,
 
 	INIT_LIST_HEAD(&mhi_cntrl->st_transition_list);
 	INIT_LIST_HEAD(&mhi_cntrl->ch_db_list);
-	spin_lock_init(&mhi_cntrl->state_lock);
 	spin_lock_init(&mhi_cntrl->list_lock);
+	mutex_init(&mhi_cntrl->state_lock);
 	mutex_init(&mhi_cntrl->event_lock);
 
 	/* Set MHI version and AMSS EE before enumeration */
diff --git a/drivers/bus/mhi/ep/sm.c b/drivers/bus/mhi/ep/sm.c
index 3655c19e23c7..fd200b2ac0bb 100644
--- a/drivers/bus/mhi/ep/sm.c
+++ b/drivers/bus/mhi/ep/sm.c
@@ -63,24 +63,23 @@ int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl)
 	int ret;
 
 	/* If MHI is in M3, resume suspended channels */
-	spin_lock_bh(&mhi_cntrl->state_lock);
+	mutex_lock(&mhi_cntrl->state_lock);
+
 	old_state = mhi_cntrl->mhi_state;
 	if (old_state == MHI_STATE_M3)
 		mhi_ep_resume_channels(mhi_cntrl);
 
 	ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M0);
-	spin_unlock_bh(&mhi_cntrl->state_lock);
-
 	if (ret) {
 		mhi_ep_handle_syserr(mhi_cntrl);
-		return ret;
+		goto err_unlock;
 	}
 
 	/* Signal host that the device moved to M0 */
 	ret = mhi_ep_send_state_change_event(mhi_cntrl, MHI_STATE_M0);
 	if (ret) {
 		dev_err(dev, "Failed sending M0 state change event\n");
-		return ret;
+		goto err_unlock;
 	}
 
 	if (old_state == MHI_STATE_READY) {
@@ -88,11 +87,14 @@ int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl)
 		ret = mhi_ep_send_ee_event(mhi_cntrl, MHI_EE_AMSS);
 		if (ret) {
 			dev_err(dev, "Failed sending AMSS EE event\n");
-			return ret;
+			goto err_unlock;
 		}
 	}
 
-	return 0;
+err_unlock:
+	mutex_unlock(&mhi_cntrl->state_lock);
+
+	return ret;
 }
 
 int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl)
@@ -100,13 +102,12 @@ int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl)
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 	int ret;
 
-	spin_lock_bh(&mhi_cntrl->state_lock);
-	ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M3);
-	spin_unlock_bh(&mhi_cntrl->state_lock);
+	mutex_lock(&mhi_cntrl->state_lock);
 
+	ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M3);
 	if (ret) {
 		mhi_ep_handle_syserr(mhi_cntrl);
-		return ret;
+		goto err_unlock;
 	}
 
 	mhi_ep_suspend_channels(mhi_cntrl);
@@ -115,10 +116,13 @@ int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl)
 	ret = mhi_ep_send_state_change_event(mhi_cntrl, MHI_STATE_M3);
 	if (ret) {
 		dev_err(dev, "Failed sending M3 state change event\n");
-		return ret;
+		goto err_unlock;
 	}
 
-	return 0;
+err_unlock:
+	mutex_unlock(&mhi_cntrl->state_lock);
+
+	return ret;
 }
 
 int mhi_ep_set_ready_state(struct mhi_ep_cntrl *mhi_cntrl)
@@ -127,22 +131,24 @@ int mhi_ep_set_ready_state(struct mhi_ep_cntrl *mhi_cntrl)
 	enum mhi_state mhi_state;
 	int ret, is_ready;
 
-	spin_lock_bh(&mhi_cntrl->state_lock);
+	mutex_lock(&mhi_cntrl->state_lock);
+
 	/* Ensure that the MHISTATUS is set to RESET by host */
 	mhi_state = mhi_ep_mmio_masked_read(mhi_cntrl, EP_MHISTATUS, MHISTATUS_MHISTATE_MASK);
 	is_ready = mhi_ep_mmio_masked_read(mhi_cntrl, EP_MHISTATUS, MHISTATUS_READY_MASK);
 
 	if (mhi_state != MHI_STATE_RESET || is_ready) {
 		dev_err(dev, "READY state transition failed. MHI host not in RESET state\n");
-		spin_unlock_bh(&mhi_cntrl->state_lock);
-		return -EIO;
+		ret = -EIO;
+		goto err_unlock;
 	}
 
 	ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_READY);
-	spin_unlock_bh(&mhi_cntrl->state_lock);
-
 	if (ret)
 		mhi_ep_handle_syserr(mhi_cntrl);
 
+err_unlock:
+	mutex_unlock(&mhi_cntrl->state_lock);
+
 	return ret;
 }
diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
index 478aece17046..f198a8ac7ee7 100644
--- a/include/linux/mhi_ep.h
+++ b/include/linux/mhi_ep.h
@@ -70,8 +70,8 @@ struct mhi_ep_db_info {
  * @cmd_ctx_cache_phys: Physical address of the host command context cache
  * @chdb: Array of channel doorbell interrupt info
  * @event_lock: Lock for protecting event rings
- * @list_lock: Lock for protecting state transition and channel doorbell lists
  * @state_lock: Lock for protecting state transitions
+ * @list_lock: Lock for protecting state transition and channel doorbell lists
  * @st_transition_list: List of state transitions
  * @ch_db_list: List of queued channel doorbells
  * @wq: Dedicated workqueue for handling rings and state changes
@@ -117,8 +117,8 @@ struct mhi_ep_cntrl {
 
 	struct mhi_ep_db_info chdb[4];
 	struct mutex event_lock;
+	struct mutex state_lock;
 	spinlock_t list_lock;
-	spinlock_t state_lock;
 
 	struct list_head st_transition_list;
 	struct list_head ch_db_list;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] bus: mhi: ep: Change state_lock to mutex
  2023-01-23  7:50 [PATCH] bus: mhi: ep: Change state_lock to mutex Manivannan Sadhasivam
@ 2023-01-23 17:10 ` Jeffrey Hugo
  2023-01-27  6:58 ` Manivannan Sadhasivam
  1 sibling, 0 replies; 3+ messages in thread
From: Jeffrey Hugo @ 2023-01-23 17:10 UTC (permalink / raw)
  To: Manivannan Sadhasivam, mhi
  Cc: linux-arm-msm, linux-kernel, stable, Dan Carpenter

On 1/23/2023 12:50 AM, Manivannan Sadhasivam wrote:
> state_lock, the spinlock type is meant to protect race against concurrent
> MHI state transitions. In mhi_ep_set_m0_state(), while the state_lock is
> being held, the channels are resumed in mhi_ep_resume_channels() if the
> previous state was M3. This causes sleeping in atomic bug, since
> mhi_ep_resume_channels() use mutex internally.
> 
> Since the state_lock is supposed to be held throughout the state change,
> it is not ideal to drop the lock before calling mhi_ep_resume_channels().
> So to fix this issue, let's change the type of state_lock to mutex. This
> would also allow holding the lock throughout all state transitions thereby
> avoiding any potential race.
> 
> Cc: <stable@vger.kernel.org> # 5.19
> Fixes: e4b7b5f0f30a ("bus: mhi: ep: Add support for suspending and resuming channels")
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Looks sane.
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] bus: mhi: ep: Change state_lock to mutex
  2023-01-23  7:50 [PATCH] bus: mhi: ep: Change state_lock to mutex Manivannan Sadhasivam
  2023-01-23 17:10 ` Jeffrey Hugo
@ 2023-01-27  6:58 ` Manivannan Sadhasivam
  1 sibling, 0 replies; 3+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-27  6:58 UTC (permalink / raw)
  To: mhi; +Cc: linux-arm-msm, linux-kernel, stable, Dan Carpenter

On Mon, Jan 23, 2023 at 01:20:49PM +0530, Manivannan Sadhasivam wrote:
> state_lock, the spinlock type is meant to protect race against concurrent
> MHI state transitions. In mhi_ep_set_m0_state(), while the state_lock is
> being held, the channels are resumed in mhi_ep_resume_channels() if the
> previous state was M3. This causes sleeping in atomic bug, since
> mhi_ep_resume_channels() use mutex internally.
> 
> Since the state_lock is supposed to be held throughout the state change,
> it is not ideal to drop the lock before calling mhi_ep_resume_channels().
> So to fix this issue, let's change the type of state_lock to mutex. This
> would also allow holding the lock throughout all state transitions thereby
> avoiding any potential race.
> 
> Cc: <stable@vger.kernel.org> # 5.19
> Fixes: e4b7b5f0f30a ("bus: mhi: ep: Add support for suspending and resuming channels")
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Applied to mhi-next!

Thanks,
Mani

> ---
>  drivers/bus/mhi/ep/main.c |  8 +++++---
>  drivers/bus/mhi/ep/sm.c   | 42 ++++++++++++++++++++++-----------------
>  include/linux/mhi_ep.h    |  4 ++--
>  3 files changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> index bcaaba97ef63..528c00b232bf 100644
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -1001,11 +1001,11 @@ static void mhi_ep_reset_worker(struct work_struct *work)
>  
>  	mhi_ep_power_down(mhi_cntrl);
>  
> -	spin_lock_bh(&mhi_cntrl->state_lock);
> +	mutex_lock(&mhi_cntrl->state_lock);
> +
>  	/* Reset MMIO to signal host that the MHI_RESET is completed in endpoint */
>  	mhi_ep_mmio_reset(mhi_cntrl);
>  	cur_state = mhi_cntrl->mhi_state;
> -	spin_unlock_bh(&mhi_cntrl->state_lock);
>  
>  	/*
>  	 * Only proceed further if the reset is due to SYS_ERR. The host will
> @@ -1014,6 +1014,8 @@ static void mhi_ep_reset_worker(struct work_struct *work)
>  	 */
>  	if (cur_state == MHI_STATE_SYS_ERR)
>  		mhi_ep_power_up(mhi_cntrl);
> +
> +	mutex_unlock(&mhi_cntrl->state_lock);
>  }
>  
>  /*
> @@ -1386,8 +1388,8 @@ int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl,
>  
>  	INIT_LIST_HEAD(&mhi_cntrl->st_transition_list);
>  	INIT_LIST_HEAD(&mhi_cntrl->ch_db_list);
> -	spin_lock_init(&mhi_cntrl->state_lock);
>  	spin_lock_init(&mhi_cntrl->list_lock);
> +	mutex_init(&mhi_cntrl->state_lock);
>  	mutex_init(&mhi_cntrl->event_lock);
>  
>  	/* Set MHI version and AMSS EE before enumeration */
> diff --git a/drivers/bus/mhi/ep/sm.c b/drivers/bus/mhi/ep/sm.c
> index 3655c19e23c7..fd200b2ac0bb 100644
> --- a/drivers/bus/mhi/ep/sm.c
> +++ b/drivers/bus/mhi/ep/sm.c
> @@ -63,24 +63,23 @@ int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl)
>  	int ret;
>  
>  	/* If MHI is in M3, resume suspended channels */
> -	spin_lock_bh(&mhi_cntrl->state_lock);
> +	mutex_lock(&mhi_cntrl->state_lock);
> +
>  	old_state = mhi_cntrl->mhi_state;
>  	if (old_state == MHI_STATE_M3)
>  		mhi_ep_resume_channels(mhi_cntrl);
>  
>  	ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M0);
> -	spin_unlock_bh(&mhi_cntrl->state_lock);
> -
>  	if (ret) {
>  		mhi_ep_handle_syserr(mhi_cntrl);
> -		return ret;
> +		goto err_unlock;
>  	}
>  
>  	/* Signal host that the device moved to M0 */
>  	ret = mhi_ep_send_state_change_event(mhi_cntrl, MHI_STATE_M0);
>  	if (ret) {
>  		dev_err(dev, "Failed sending M0 state change event\n");
> -		return ret;
> +		goto err_unlock;
>  	}
>  
>  	if (old_state == MHI_STATE_READY) {
> @@ -88,11 +87,14 @@ int mhi_ep_set_m0_state(struct mhi_ep_cntrl *mhi_cntrl)
>  		ret = mhi_ep_send_ee_event(mhi_cntrl, MHI_EE_AMSS);
>  		if (ret) {
>  			dev_err(dev, "Failed sending AMSS EE event\n");
> -			return ret;
> +			goto err_unlock;
>  		}
>  	}
>  
> -	return 0;
> +err_unlock:
> +	mutex_unlock(&mhi_cntrl->state_lock);
> +
> +	return ret;
>  }
>  
>  int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl)
> @@ -100,13 +102,12 @@ int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl)
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	int ret;
>  
> -	spin_lock_bh(&mhi_cntrl->state_lock);
> -	ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M3);
> -	spin_unlock_bh(&mhi_cntrl->state_lock);
> +	mutex_lock(&mhi_cntrl->state_lock);
>  
> +	ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_M3);
>  	if (ret) {
>  		mhi_ep_handle_syserr(mhi_cntrl);
> -		return ret;
> +		goto err_unlock;
>  	}
>  
>  	mhi_ep_suspend_channels(mhi_cntrl);
> @@ -115,10 +116,13 @@ int mhi_ep_set_m3_state(struct mhi_ep_cntrl *mhi_cntrl)
>  	ret = mhi_ep_send_state_change_event(mhi_cntrl, MHI_STATE_M3);
>  	if (ret) {
>  		dev_err(dev, "Failed sending M3 state change event\n");
> -		return ret;
> +		goto err_unlock;
>  	}
>  
> -	return 0;
> +err_unlock:
> +	mutex_unlock(&mhi_cntrl->state_lock);
> +
> +	return ret;
>  }
>  
>  int mhi_ep_set_ready_state(struct mhi_ep_cntrl *mhi_cntrl)
> @@ -127,22 +131,24 @@ int mhi_ep_set_ready_state(struct mhi_ep_cntrl *mhi_cntrl)
>  	enum mhi_state mhi_state;
>  	int ret, is_ready;
>  
> -	spin_lock_bh(&mhi_cntrl->state_lock);
> +	mutex_lock(&mhi_cntrl->state_lock);
> +
>  	/* Ensure that the MHISTATUS is set to RESET by host */
>  	mhi_state = mhi_ep_mmio_masked_read(mhi_cntrl, EP_MHISTATUS, MHISTATUS_MHISTATE_MASK);
>  	is_ready = mhi_ep_mmio_masked_read(mhi_cntrl, EP_MHISTATUS, MHISTATUS_READY_MASK);
>  
>  	if (mhi_state != MHI_STATE_RESET || is_ready) {
>  		dev_err(dev, "READY state transition failed. MHI host not in RESET state\n");
> -		spin_unlock_bh(&mhi_cntrl->state_lock);
> -		return -EIO;
> +		ret = -EIO;
> +		goto err_unlock;
>  	}
>  
>  	ret = mhi_ep_set_mhi_state(mhi_cntrl, MHI_STATE_READY);
> -	spin_unlock_bh(&mhi_cntrl->state_lock);
> -
>  	if (ret)
>  		mhi_ep_handle_syserr(mhi_cntrl);
>  
> +err_unlock:
> +	mutex_unlock(&mhi_cntrl->state_lock);
> +
>  	return ret;
>  }
> diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
> index 478aece17046..f198a8ac7ee7 100644
> --- a/include/linux/mhi_ep.h
> +++ b/include/linux/mhi_ep.h
> @@ -70,8 +70,8 @@ struct mhi_ep_db_info {
>   * @cmd_ctx_cache_phys: Physical address of the host command context cache
>   * @chdb: Array of channel doorbell interrupt info
>   * @event_lock: Lock for protecting event rings
> - * @list_lock: Lock for protecting state transition and channel doorbell lists
>   * @state_lock: Lock for protecting state transitions
> + * @list_lock: Lock for protecting state transition and channel doorbell lists
>   * @st_transition_list: List of state transitions
>   * @ch_db_list: List of queued channel doorbells
>   * @wq: Dedicated workqueue for handling rings and state changes
> @@ -117,8 +117,8 @@ struct mhi_ep_cntrl {
>  
>  	struct mhi_ep_db_info chdb[4];
>  	struct mutex event_lock;
> +	struct mutex state_lock;
>  	spinlock_t list_lock;
> -	spinlock_t state_lock;
>  
>  	struct list_head st_transition_list;
>  	struct list_head ch_db_list;
> -- 
> 2.25.1
> 

-- 
மணிவண்ணன் சதாசிவம்

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-27  6:58 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-23  7:50 [PATCH] bus: mhi: ep: Change state_lock to mutex Manivannan Sadhasivam
2023-01-23 17:10 ` Jeffrey Hugo
2023-01-27  6:58 ` Manivannan Sadhasivam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).