From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: mhi@lists.linux.dev
Cc: linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
stable@vger.kernel.org, Dan Carpenter <error27@gmail.com>
Subject: Re: [PATCH] bus: mhi: ep: Change state_lock to mutex
Date: Fri, 27 Jan 2023 12:28:21 +0530 [thread overview]
Message-ID: <20230127065821.GF7809@thinkpad> (raw)
In-Reply-To: <20230123075049.168040-1-manivannan.sadhasivam@linaro.org>
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
>
--
மணிவண்ணன் சதாசிவம்
prev parent reply other threads:[~2023-01-27 6:58 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230127065821.GF7809@thinkpad \
--to=manivannan.sadhasivam@linaro.org \
--cc=error27@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mhi@lists.linux.dev \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.