Re: [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel

[Hemant Kumar]

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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> > ---
> > Â 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