* Re: [PATCH v5 1/2] bus: mhi: host: Add spinlock to protect WP access when queueing TREs
2023-12-11 6:42 ` [PATCH v5 1/2] bus: mhi: host: Add spinlock to protect WP access when queueing TREs Qiang Yu
@ 2023-12-15 18:23 ` Jeffrey Hugo
2023-12-16 5:15 ` Manivannan Sadhasivam
1 sibling, 0 replies; 3+ messages in thread
From: Jeffrey Hugo @ 2023-12-15 18:23 UTC (permalink / raw)
To: Qiang Yu, mani
Cc: mhi, linux-arm-msm, linux-kernel, quic_cang, quic_mrana,
Bhaumik Bhatt, stable
On 12/10/2023 11:42 PM, Qiang Yu wrote:
> From: Bhaumik Bhatt <bbhatt@codeaurora.org>
>
> Protect WP accesses such that multiple threads queueing buffers for
> incoming data do not race.
>
> Meanwhile, if CONFIG_TRACE_IRQFLAGS is enabled, irq will be enabled once
> __local_bh_enable_ip is called as part of write_unlock_bh. Hence, let's
> take irqsave lock after TRE is generated to avoid running write_unlock_bh
> when irqsave lock is held.
>
> Cc: <stable@vger.kernel.org>
> Fixes: 189ff97cca53 ("bus: mhi: core: Add support for data transfer")
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
Seems to work fine for AIC100
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v5 1/2] bus: mhi: host: Add spinlock to protect WP access when queueing TREs
2023-12-11 6:42 ` [PATCH v5 1/2] bus: mhi: host: Add spinlock to protect WP access when queueing TREs Qiang Yu
2023-12-15 18:23 ` Jeffrey Hugo
@ 2023-12-16 5:15 ` Manivannan Sadhasivam
1 sibling, 0 replies; 3+ messages in thread
From: Manivannan Sadhasivam @ 2023-12-16 5:15 UTC (permalink / raw)
To: Qiang Yu
Cc: mani, quic_jhugo, mhi, linux-arm-msm, linux-kernel, quic_cang,
quic_mrana, Bhaumik Bhatt, stable
On Mon, Dec 11, 2023 at 02:42:51PM +0800, Qiang Yu wrote:
> From: Bhaumik Bhatt <bbhatt@codeaurora.org>
>
> Protect WP accesses such that multiple threads queueing buffers for
> incoming data do not race.
>
> Meanwhile, if CONFIG_TRACE_IRQFLAGS is enabled, irq will be enabled once
> __local_bh_enable_ip is called as part of write_unlock_bh. Hence, let's
> take irqsave lock after TRE is generated to avoid running write_unlock_bh
> when irqsave lock is held.
>
> Cc: <stable@vger.kernel.org>
> Fixes: 189ff97cca53 ("bus: mhi: core: Add support for data transfer")
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
- Mani
> ---
> drivers/bus/mhi/host/main.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index dcf627b..32021fe 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -1122,17 +1122,15 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info,
> if (unlikely(MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)))
> return -EIO;
>
> - read_lock_irqsave(&mhi_cntrl->pm_lock, flags);
> -
> ret = mhi_is_ring_full(mhi_cntrl, tre_ring);
> - if (unlikely(ret)) {
> - ret = -EAGAIN;
> - goto exit_unlock;
> - }
> + if (unlikely(ret))
> + return -EAGAIN;
>
> ret = mhi_gen_tre(mhi_cntrl, mhi_chan, buf_info, mflags);
> if (unlikely(ret))
> - goto exit_unlock;
> + return ret;
> +
> + read_lock_irqsave(&mhi_cntrl->pm_lock, flags);
>
> /* Packet is queued, take a usage ref to exit M3 if necessary
> * for host->device buffer, balanced put is done on buffer completion
> @@ -1152,7 +1150,6 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info,
> if (dir == DMA_FROM_DEVICE)
> mhi_cntrl->runtime_put(mhi_cntrl);
>
> -exit_unlock:
> read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags);
>
> return ret;
> @@ -1204,6 +1201,9 @@ int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
> int eot, eob, chain, bei;
> int ret;
>
> + /* Protect accesses for reading and incrementing WP */
> + write_lock_bh(&mhi_chan->lock);
> +
> buf_ring = &mhi_chan->buf_ring;
> tre_ring = &mhi_chan->tre_ring;
>
> @@ -1221,8 +1221,10 @@ int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
>
> if (!info->pre_mapped) {
> ret = mhi_cntrl->map_single(mhi_cntrl, buf_info);
> - if (ret)
> + if (ret) {
> + write_unlock_bh(&mhi_chan->lock);
> return ret;
> + }
> }
>
> eob = !!(flags & MHI_EOB);
> @@ -1239,6 +1241,8 @@ int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
> mhi_add_ring_element(mhi_cntrl, tre_ring);
> mhi_add_ring_element(mhi_cntrl, buf_ring);
>
> + write_unlock_bh(&mhi_chan->lock);
> +
> return 0;
> }
>
> --
> 2.7.4
>
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 3+ messages in thread