Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org CBDCEC433CB
Subject: Re: [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi
 channel
To:     Jeffrey Hugo <jhugo@codeaurora.org>,
        Bhaumik Bhatt <bbhatt@codeaurora.org>, mani@kernel.org
Cc:     linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1588042766-17496-1-git-send-email-bbhatt@codeaurora.org>
 <1588042766-17496-2-git-send-email-bbhatt@codeaurora.org>
 <59e6a99b-9fde-cd00-82ba-c8604a11c618@codeaurora.org>
From:   Hemant Kumar <hemantk@codeaurora.org>
Message-ID: <4dfbd3d0-7547-e01e-736a-94881949592a@codeaurora.org>
Date:   Wed, 29 Apr 2020 10:27:42 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <59e6a99b-9fde-cd00-82ba-c8604a11c618@codeaurora.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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