Hi Hemant,
On Thu, Apr 16, 2020 at 08:37:16PM -0700, Hemant Kumar wrote:
>
> On 4/7/20 7:33 AM, Manivannan Sadhasivam wrote:
> > Hi Dan,
> >
> > On Tue, Apr 07, 2020 at 04:55:59PM +0300, Dan Carpenter wrote:
> > > Hello Manivannan Sadhasivam,
> > >
> > > The patch 189ff97cca53: "bus: mhi: core: Add support for data
> > > transfer" from Feb 20, 2020, leads to the following static checker
> > > warning:
> > >
> > > drivers/bus/mhi/core/main.c:1153 mhi_queue_buf()
> > > error: double locked 'mhi_chan->lock' (orig line 1110)
> > >
> > > drivers/bus/mhi/core/main.c
> > > 1142 }
> > > 1143
> > > 1144 /* Toggle wake to exit out of M2 */
> > > 1145 mhi_cntrl->wake_toggle(mhi_cntrl);
> > > 1146
> > > 1147 if (mhi_chan->dir == DMA_TO_DEVICE)
> > > 1148 atomic_inc(&mhi_cntrl->pending_pkts);
> > > 1149
> > > 1150 if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl))) {
> > > 1151 unsigned long flags;
> > > 1152
> > > 1153 read_lock_irqsave(&mhi_chan->lock, flags);
>
> parse_xfer_event is taking read lock : read_lock_bh(&mhi_chan->lock); first
> and later
>
> mhi_queue_buf takes read lock: read_lock_irqsave(&mhi_chan->lock, flags);
>
> Both are read locks which are recursive, is this problematic ?
>
read_locks are recursive but I wanted to make the static checker happy. But
looking into it further (and after having a chat with Arnd), we might need to
refactor the locking here.
Since 'chan->lock' only prevents 'mhi_chan->ch_state', how about doing something
like below?
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 3e9aa3b2da77..904f9be7a142 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -474,19 +474,12 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
result.transaction_status = (ev_code == MHI_EV_CC_OVERFLOW) ?
-EOVERFLOW : 0;
- /*
- * If it's a DB Event then we need to grab the lock
- * with preemption disabled and as a write because we
- * have to update db register and there are chances that
- * another thread could be doing the same.
- */
- if (ev_code >= MHI_EV_CC_OOB)
- write_lock_irqsave(&mhi_chan->lock, flags);
- else
- read_lock_bh(&mhi_chan->lock);
-
- if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED)
- goto end_process_tx_event;
+ read_lock_bh(&mhi_chan->lock);
+ if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) {
+ read_unlock_bh(&mhi_chan->lock);
+ return 0;
+ }
+ read_unlock_bh(&mhi_chan->lock);
switch (ev_code) {
case MHI_EV_CC_OVERFLOW:
@@ -559,10 +552,12 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
mhi_chan->db_cfg.db_mode = 1;
read_lock_irqsave(&mhi_cntrl->pm_lock, flags);
+ write_lock_irqsave(&mhi_chan->lock, flags);
if (tre_ring->wp != tre_ring->rp &&
MHI_DB_ACCESS_VALID(mhi_cntrl)) {
mhi_ring_chan_db(mhi_cntrl, mhi_chan);
}
+ write_unlock_irqrestore(&mhi_chan->lock, flags);
read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags);
break;
}
@@ -572,12 +567,6 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
break;
} /* switch(MHI_EV_READ_CODE(EV_TRB_CODE,event)) */
-end_process_tx_event:
- if (ev_code >= MHI_EV_CC_OOB)
- write_unlock_irqrestore(&mhi_chan->lock, flags);
- else
- read_unlock_bh(&mhi_chan->lock);
-
return 0;
}
Moreover, I do have couple of concerns:
1. 'mhi_chan->db_cfg.db_mode = 1' needs to be added to the critical section
above.
2. Why we have {write/read}_lock_irq variants for chan->lock? I don't see where
the db or ch_state got shared with hardirq handler. Maybe we should only have
*_bh (softirq) variants all over the place?
Thanks,
Mani
> > > ^^^^^^^^^^^^^^^
> > > The caller is already holding this lock.
> > >
> > Hmm. We have one internal user of this function and that's where the locking
> > has gone wrong. Will fix it.
> >
> > Thanks for reporting!
> >
> > Regards,
> > Mani
> >
> > > 1154 mhi_ring_chan_db(mhi_cntrl, mhi_chan);
> > > 1155 read_unlock_irqrestore(&mhi_chan->lock, flags);
> > > 1156 }
> > > 1157
> > > 1158 read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags);
> > > 1159
> > > 1160 return 0;
> > > 1161 }
> > > 1162 EXPORT_SYMBOL_GPL(mhi_queue_buf);
> > >
> > > regards,
> > > dan carpenter
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
