linux-kernel - Re: [PATCH V1 3/3] scsi: ufs: qcom: Add support for multiple ICE algorithms

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: <cb2ecd04-ae0b-40c3-ac3c-9bcb1bf46e7d@quicinc.com>
Date: Tue, 29 Oct 2024 16:40:23 +0530
From: Ram Kumar Dwivedi <quic_rdwivedi@...cinc.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        <manivannan.sadhasivam@...aro.org>, <alim.akhtar@...sung.com>,
        <avri.altman@....com>, <bvanassche@....org>, <robh@...nel.org>,
        <krzk+dt@...nel.org>, <conor+dt@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <James.Bottomley@...senPartnership.com>,
        <martin.petersen@...cle.com>, <agross@...nel.org>
CC: <linux-arm-msm@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_narepall@...cinc.com>, <quic_nitirawa@...cinc.com>,
        Can Guo
	<quic_cang@...cinc.com>
Subject: Re: [PATCH V1 3/3] scsi: ufs: qcom: Add support for multiple ICE
 algorithms



On 06-Oct-24 1:03 AM, Christophe JAILLET wrote:
> Le 05/10/2024 à 08:43, Ram Kumar Dwivedi a écrit :
>> Add support for ICE algorithms for Qualcomm UFS V5.0 and above which
>> uses a pool of crypto cores for TX stream (UFS Write – Encryption)
>> and RX stream (UFS Read – Decryption).
>>
>> Using these algorithms, crypto cores can be dynamically allocated
>> to either RX stream or TX stream based on algorithm selected.
>> Qualcomm UFS controller supports three ICE algorithms:
>> Floor based algorithm, Static Algorithm and Instantaneous algorithm
>> to share crypto cores between TX and RX stream.
>>
>> Floor Based allocation is selected by default after power On or Reset.
>>
>> Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@...cinc.com>
>> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@...cinc.com>
>> Co-developed-by: Nitin Rawat <quic_nitirawa@...cinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@...cinc.com>
>> Co-developed-by: Can Guo <quic_cang@...cinc.com>
>> Signed-off-by: Can Guo <quic_cang@...cinc.com>
>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@...cinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 232 ++++++++++++++++++++++++++++++++++++
>>   drivers/ufs/host/ufs-qcom.h |  38 +++++-
>>   2 files changed, 269 insertions(+), 1 deletion(-)
> 
> Hi,
> 
> a few nitpicks below.
> 
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 810e637047d0..c0ca835f13f3 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -105,6 +105,217 @@ static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>>   }
>>     #ifdef CONFIG_SCSI_UFS_CRYPTO
>> +/*
>> + * Default overrides:
>> + * There're 10 sets of settings for floor-based algorithm
>> + */
>> +static struct ice_alg2_config alg2_config[] = {
> 
> I think that this could easily be a const struct.
> 
>> +    {"G0", {5, 12, 0, 0, 32, 0}},
>> +    {"G1", {12, 5, 32, 0, 0, 0}},
>> +    {"G2", {6, 11, 4, 1, 32, 1}},
>> +    {"G3", {6, 11, 7, 1, 32, 1}},
>> +    {"G4", {7, 10, 11, 1, 32, 1}},
>> +    {"G5", {7, 10, 14, 1, 32, 1}},
>> +    {"G6", {8, 9, 18, 1, 32, 1}},
>> +    {"G7", {9, 8, 21, 1, 32, 1}},
>> +    {"G8", {10, 7, 24, 1, 32, 1}},
>> +    {"G9", {10, 7, 32, 1, 32, 1}},
>> +};
>> +
>> +/**
> 
> This does nor look like a kernel-doc. Just /* ?
> 
>> + * Refer struct ice_alg2_config
>> + */
>> +static inline void __get_alg2_grp_params(unsigned int *val, int *c, int *t)
>> +{
>> +    *c = ((val[0] << 8) | val[1] | (1 << 31));
>> +    *t = ((val[2] << 24) | (val[3] << 16) | (val[4] << 8) | val[5]);
>> +}
> 
> ...
> 
>> +/**
>> + * ufs_qcom_ice_config_alg2 - Floor based ICE algorithm
>> + *
>> + * @hba: host controller instance
>> + * Return: zero for success and non-zero in case of a failure.
>> + */
>> +static int ufs_qcom_ice_config_alg2(struct ufs_hba *hba)
>> +{
>> +    struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> +    unsigned int reg = REG_UFS_MEM_ICE_ALG2_NUM_CORE_0;
>> +    /* 6 values for each group, refer struct ice_alg2_config */
>> +    unsigned int override_val[ICE_ALG2_NUM_PARAMS];
>> +    char name[8] = {0};
>> +    int i, ret;
>> +
>> +    ufshcd_writel(hba, FLOOR_BASED_ALG2, REG_UFS_MEM_ICE_CONFIG);
>> +    for (i = 0; i < ARRAY_SIZE(alg2_config); i++) {
>> +        int core = 0, task = 0;
>> +
>> +        if (host->ice_conf) {
>> +            snprintf(name, sizeof(name), "%s%d", "g", i);
> 
> Why not just "g%d"?
> 
>> +            ret = of_property_read_variable_u32_array(host->ice_conf,
>> +                                  name,
>> +                                  override_val,
>> +                                  ICE_ALG2_NUM_PARAMS,
>> +                                  ICE_ALG2_NUM_PARAMS);
> 
> ...
> 
> CJ
> 


Hi Christophe,

I have addressed your comments in latest patchset.

Thanks,
Ram.