Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4caaf07f-115a-eb2c-ed53-f62698167802@quicinc.com>
Date:   Wed, 7 Sep 2022 20:01:36 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.1
Subject: Re: [RFC PATCH v3 2/4] ufs: core: mcq: Adds Multi-Circular Queue
 support
To:     Bean Huo <huobean@gmail.com>, <quic_nguyenb@quicinc.com>,
        <quic_xiaosenh@quicinc.com>, <stanley.chu@mediatek.com>,
        <adrian.hunter@intel.com>, <bvanassche@acm.org>,
        <avri.altman@wdc.com>, <mani@kernel.org>, <quic_cang@quicinc.com>,
        <beanhuo@micron.com>, <martin.petersen@oracle.com>,
        <linux-scsi@vger.kernel.org>
CC:     <linux-arm-msm@vger.kernel.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Jinyoung Choi <j-young.choi@samsung.com>,
        Kiwoong Kim <kwmad.kim@samsung.com>,
        open list <linux-kernel@vger.kernel.org>
References: <cover.1662157846.git.quic_asutoshd@quicinc.com>
 <04f4949e4dea991a93bdf6727bf12948ecc586be.1662157846.git.quic_asutoshd@quicinc.com>
 <c3d2d8b307f7e11b7a2be751673220049b9a1a77.camel@gmail.com>
From:   "Asutosh Das (asd)" <quic_asutoshd@quicinc.com>
In-Reply-To: <c3d2d8b307f7e11b7a2be751673220049b9a1a77.camel@gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hello Bean
Thanks for your comments. Responses inline.

On 9/5/2022 9:27 AM, Bean Huo wrote:
> On Fri, 2022-09-02 at 15:41 -0700, Asutosh Das wrote:
>>
>> +
>> +static void ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
>> +{
>> +       int i, rem;
>> +       u32 hbaq_cap, cmp;
>> +       struct Scsi_Host *host = hba->host;
>> +
>> +       hbaq_cap = hba->mcq_capabilities & 0xff;
>> +
>> +       rem = hbaq_cap - dev_cmd_queue;
>> +       /* min() compares variables of same type */
>> +       hba->nr_queues[HCTX_TYPE_DEFAULT] = min(hbaq_cap -
>> dev_cmd_queue,
> 
>   hba->nr_queues[HCTX_TYPE_DEFAULT] = min(rem, num_possible_cpus());
> 
min() compares variables of same type only.
>> +                                               num_possible_cpus());
>> +       rem -= hba->nr_queues[HCTX_TYPE_DEFAULT];
>> +       if (rem <= 0)
>> +               goto out;
>> +       cmp = rem;
>> +       hba->nr_queues[HCTX_TYPE_POLL] = min(cmp, poll_queues);
>> +       rem -= hba->nr_queues[HCTX_TYPE_POLL];
>> +       if (rem <= 0)
>> +               goto out;
>> +       cmp = rem;
>> +       hba->nr_queues[HCTX_TYPE_READ] = min(cmp, read_queues);
> 
> static read_queues is not initialized.
> 
It would be initialized to 0. But I think we should configure it as a 
module parameter which is missing. Let me add it in the next version.
>>
> .....
>> +static inline void ufshcd_inc_tp(struct ufs_hw_queue *q)
>> +{
>> +       u32 mask = q->max_entries - 1;
>> +       u32 val;
>> +
>> +       q->sq_tp_slot = (q->sq_tp_slot + 1) & mask;
>> +       val = q->sq_tp_slot * sizeof(struct utp_transfer_req_desc);
>> +       writel(val, q->mcq_sq_tp);
>> +}
> 
> This function just accesses the submission queue tail pointer. The
> function name should clearly explain this.
> 
The reason for this naming was that only SQ's tail pointer can be 
updated by SW. I can add a _sq_ to this function in the next version. 
Plmk if you prefer a better name.

> 
> 
>