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: <[email protected]> # 5.19
Fixes: e4b7b5f0f30a ("bus: mhi: ep: Add support for suspending and resuming channels")
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
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
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: <[email protected]> # 5.19
> Fixes: e4b7b5f0f30a ("bus: mhi: ep: Add support for suspending and resuming channels")
> Reported-by: Dan Carpenter <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Looks sane.
Reviewed-by: Jeffrey Hugo <[email protected]>
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: <[email protected]> # 5.19
> Fixes: e4b7b5f0f30a ("bus: mhi: ep: Add support for suspending and resuming channels")
> Reported-by: Dan Carpenter <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
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
>
--
மணிவண்ணன் சதாசிவம்