Messages in this thread | | | Subject | Re: [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel | From | Hemant Kumar <> | Date | Wed, 29 Apr 2020 10:27:42 -0700 |
| |
Hi Jeff
On 4/28/20 7:39 AM, Jeffrey Hugo wrote: > On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote: >> From: Hemant Kumar <hemantk@codeaurora.org> >> >> Driver is using zero initialized intmod value from mhi channel when >> configuring TRE for bei field. This prevents interrupt moderation to >> take effect in case it is supported by an event ring. Fix this by >> copying intmod value from associated event ring to mhi channel upon >> registering mhi controller. >> >> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org> >> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >> --- >> drivers/bus/mhi/core/init.c | 4 ++++ >> drivers/bus/mhi/core/main.c | 9 ++++++--- >> 2 files changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c >> index b38359c..4dc7f22 100644 >> --- a/drivers/bus/mhi/core/init.c >> +++ b/drivers/bus/mhi/core/init.c >> @@ -864,6 +864,10 @@ int mhi_register_controller(struct mhi_controller >> *mhi_cntrl, >> mutex_init(&mhi_chan->mutex); >> init_completion(&mhi_chan->completion); >> rwlock_init(&mhi_chan->lock); >> + >> + /* used in setting bei field of TRE */ >> + mhi_event = &mhi_cntrl->mhi_event[mhi_chan->er_index]; >> + mhi_chan->intmod = mhi_event->intmod; >> } >> if (mhi_cntrl->bounce_buf) { >> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c >> index eb4256b..23154f1 100644 >> --- a/drivers/bus/mhi/core/main.c >> +++ b/drivers/bus/mhi/core/main.c >> @@ -929,7 +929,7 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum >> dma_data_direction dir, >> struct mhi_ring *buf_ring = &mhi_chan->buf_ring; >> struct mhi_buf_info *buf_info; >> struct mhi_tre *mhi_tre; >> - int ret; >> + int ret, bei; >> /* If MHI host pre-allocates buffers then client drivers cannot >> queue */ >> if (mhi_chan->pre_alloc) >> @@ -966,10 +966,11 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, >> enum dma_data_direction dir, >> goto map_error; >> mhi_tre = tre_ring->wp; >> + bei = !!(mhi_chan->intmod); >> mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr); >> mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(buf_info->len); >> - mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(1, 1, 0, 0); >> + mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, 1, 0, 0); > > Why are we not using mhi_gen_tre() here? Its used in mhi_queue_buf(), > which seems to be why the issue doesn't appear there. It looks like we > have 3 instances of the same code, which all need to be kept in sync. > Seems like this bug exists because they were not in sync. > > Simplifying the code by removing the duplication seems to be the better > approach by not only addressing this issue, but also preventing future > ones. > Agree, i came up with a re-factor change to have all queue APIs calling mhi_gen_tre(). Will include as part of V3. >> /* increment WP */ >> mhi_add_ring_element(mhi_cntrl, tre_ring); >> @@ -1006,6 +1007,7 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, >> enum dma_data_direction dir, >> struct mhi_ring *buf_ring = &mhi_chan->buf_ring; >> struct mhi_buf_info *buf_info; >> struct mhi_tre *mhi_tre; >> + int bei; >> /* If MHI host pre-allocates buffers then client drivers cannot >> queue */ >> if (mhi_chan->pre_alloc) >> @@ -1043,10 +1045,11 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, >> enum dma_data_direction dir, >> buf_info->len = len; >> mhi_tre = tre_ring->wp; >> + bei = !!(mhi_chan->intmod); >> mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr); >> mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(buf_info->len); >> - mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(1, 1, 0, 0); >> + mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, 1, 0, 0); >> /* increment WP */ >> mhi_add_ring_element(mhi_cntrl, tre_ring); >> > >
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
| |