linux-kernel - Re: [PATCH v5 07/16] ufs: core: mcq: Calculate queue depth

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221128195430.GD20677@asutoshd-linux1.qualcomm.com>
Date:   Mon, 28 Nov 2022 11:54:30 -0800
From:   Asutosh Das <quic_asutoshd@...cinc.com>
To:     Manivannan Sadhasivam <mani@...nel.org>
CC:     <quic_cang@...cinc.com>, <martin.petersen@...cle.com>,
        <linux-scsi@...r.kernel.org>, <quic_nguyenb@...cinc.com>,
        <quic_xiaosenh@...cinc.com>, <stanley.chu@...iatek.com>,
        <eddie.huang@...iatek.com>, <daejun7.park@...sung.com>,
        <bvanassche@....org>, <avri.altman@....com>, <beanhuo@...ron.com>,
        <linux-arm-msm@...r.kernel.org>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Jinyoung Choi <j-young.choi@...sung.com>,
        Arthur Simchaev <Arthur.Simchaev@....com>,
        Kiwoong Kim <kwmad.kim@...sung.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 07/16] ufs: core: mcq: Calculate queue depth

On Mon, Nov 28 2022 at 07:15 -0800, Manivannan Sadhasivam wrote:
>On Tue, Nov 22, 2022 at 08:10:20PM -0800, Asutosh Das wrote:
>> The ufs device defines the supported queuedepth by
>> bqueuedepth which has a max value of 256.
>> The HC defines MAC (Max Active Commands) that define
>> the max number of commands that in flight to the ufs
>> device.
>> Calculate and configure the nutrs based on both these
>> values.
>>
>> Co-developed-by: Can Guo <quic_cang@...cinc.com>
>> Signed-off-by: Can Guo <quic_cang@...cinc.com>
>> Signed-off-by: Asutosh Das <quic_asutoshd@...cinc.com>
>> ---
>>  drivers/ufs/core/ufs-mcq.c     | 32 ++++++++++++++++++++++++++++++++
>>  drivers/ufs/core/ufshcd-priv.h |  9 +++++++++
>>  drivers/ufs/core/ufshcd.c      | 17 ++++++++++++++++-
>>  drivers/ufs/host/ufs-qcom.c    |  8 ++++++++
>>  include/ufs/ufs.h              |  2 ++
>>  include/ufs/ufshcd.h           |  2 ++
>>  include/ufs/ufshci.h           |  1 +
>>  7 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
>> index 4aaa6aa..e95f748 100644
>> --- a/drivers/ufs/core/ufs-mcq.c
>> +++ b/drivers/ufs/core/ufs-mcq.c
>> @@ -18,6 +18,8 @@
>>  #define UFS_MCQ_NUM_DEV_CMD_QUEUES 1
>>  #define UFS_MCQ_MIN_POLL_QUEUES 0
>>
>> +#define MAX_DEV_CMD_ENTRIES	2
>> +#define MCQ_CFG_MAC_MASK	GENMASK(16, 8)
>>  #define MCQ_QCFGPTR_MASK	GENMASK(7, 0)
>>  #define MCQ_QCFGPTR_UNIT	0x200
>>  #define MCQ_SQATTR_OFFSET(c) \
>> @@ -88,6 +90,36 @@ static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
>>  	{.name = "mcq_vs",},
>>  };
[...]

Hello Mani,
Thanks for taking a look.

>> +	WARN_ON(!hba->dev_info.bqueuedepth);
>
>Instead of panic, you could just print and return an error.
>
I'd make it WARN_ON_ONCE();

>> +	/*
>> +	 * max. value of bqueuedepth = 256, mac is host dependent.
>> +	 * It is mandatory for UFS device to define bQueueDepth if
>> +	 * shared queuing architecture is enabled.
>> +	 */
>> +	return min_t(int, mac, hba->dev_info.bqueuedepth);
>> +}
>> +
>>  static int ufshcd_mcq_config_resource(struct ufs_hba *hba)
>>  {
>>  	struct platform_device *pdev = to_platform_device(hba->dev);
>> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
>> index 9368ba2..9f40fa5 100644
>> --- a/drivers/ufs/core/ufshcd-priv.h
>> +++ b/drivers/ufs/core/ufshcd-priv.h
>> @@ -62,6 +62,7 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
>>  	enum flag_idn idn, u8 index, bool *flag_res);
>>  void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit);
>>  int ufshcd_mcq_init(struct ufs_hba *hba);
>> +int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba);
>>
>>  #define SD_ASCII_STD true
>>  #define SD_RAW false
>> @@ -227,6 +228,14 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
>>  		hba->vops->config_scaling_param(hba, p, data);
>>  }
>>
>> +static inline int ufshcd_mcq_vops_get_hba_mac(struct ufs_hba *hba)
>
>Again, no inline please.
>
It spits out the following warning for all files that include this header, when
inline is removed:
warning: 'ufshcd_mcq_vops_get_hba_mac' defined but not used [-Wunused-function]

[...]
>> +#define MAX_SUPP_MAC 64
>
>Similar definitions are part of ufs-qcom.h.
>
>Thanks,
>Mani
>
>
>-- 
>மணிவண்ணன் சதாசிவம்