| 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
| ||
|
Message-ID: <0faa9c12-8678-4744-af49-b75c45e674f5@quicinc.com>
Date: Thu, 9 Jan 2025 12:36:55 +0530
From: Ram Kumar Dwivedi <quic_rdwivedi@...cinc.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
CC: <James.Bottomley@...senPartnership.com>, <martin.petersen@...cle.com>,
<andersson@...nel.org>, <bvanassche@....org>, <ebiggers@...nel.org>,
<linux-arm-msm@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
Naveen Kumar Goud Arepalli
<quic_narepall@...cinc.com>,
Nitin Rawat <quic_nitirawa@...cinc.com>
Subject: Re: [PATCH V9] scsi: ufs: qcom: Enable UFS Shared ICE Feature
On 08-Jan-25 6:30 PM, Manivannan Sadhasivam wrote:
> On Tue, Jan 07, 2025 at 07:26:24PM +0530, Ram Kumar Dwivedi wrote:
>> By default, the UFS controller allocates a fixed number of RX
>> and TX engines statically. Consequently, when UFS reads are in
>> progress, the TX ICE engines remain idle, and vice versa.
>> This leads to inefficient utilization of RX and TX engines.
>>
>> To address this limitation, enable the UFS shared ICE feature for
>> Qualcomm UFS V5.0 and above. This feature utilizes a pool of crypto
>> cores for both TX streams (UFS Write – Encryption) and RX streams
>> (UFS Read – Decryption). With this approach, crypto cores are
>> dynamically allocated to either the RX or TX stream as needed.
>>
>> 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>
>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@...cinc.com>
>> ---
>> Changes from v8:
>> 1. Addressed Manivannan's comment to call ufs_qcom_config_ice_allocator()
>> from ufs_qcom_ice_enable().
>
> No I did not. More below.
>
>> 2. Addressed Manivannan's comment to place UFS_QCOM_CAP_ICE_CONFIG
>> definition outside of the ufs_qcom_host struct.
>> 3. Addressed Manivannan's comment to align ICE definitions with
>> other definitions.
>>
>> Changes from v7:
>> 1. Addressed Eric's comment to perform ice configuration only if
>> UFSHCD_CAP_CRYPTO is enabled.
>>
>> Changes from v6:
>> 1. Addressed Eric's comment to replace is_ice_config_supported() helper
>> function with a conditional check for UFS_QCOM_CAP_ICE_CONFIG.
>>
>> Changes from v5:
>> 1. Addressed Bart's comment to declare the "val" variable with
>> the "static" keyword.
>>
>> Changes from v4:
>> 1. Addressed Bart's comment to use get_unaligned_le32() instead of
>> bit shifting and to declare val with the const keyword.
>>
>> Changes from v3:
>> 1. Addressed Bart's comment to change the data type of "config" to u32
>> and "val" to uint8_t.
>>
>> Changes from v2:
>> 1. Refactored the code to have a single algorithm in the code and
>> enabled by default.
>> 2. Revised the commit message to incorporate the refactored change.
>> 3. Qcom host capabilities are now enabled in a separate function.
>>
>> Changes from v1:
>> 1. Addressed Rob's and Krzysztof's comment to fix dt binding compilation
>> issue.
>> 2. Addressed Rob's comment to enable the nodes in example.
>> 3. Addressed Eric's comment to rephrase patch commit description.
>> Used terminology as ICE allocator instead of ICE algorithm.
>> 4. Addressed Christophe's comment to align the comment as per kernel doc.
>> ---
>> drivers/ufs/host/ufs-qcom.c | 38 +++++++++++++++++++++++++++++++++
>> drivers/ufs/host/ufs-qcom.h | 42 ++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 68040b2ab5f8..f4b9fb0740b4 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -15,6 +15,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/reset-controller.h>
>> #include <linux/time.h>
>> +#include <linux/unaligned.h>
>>
>> #include <soc/qcom/ice.h>
>>
>> @@ -105,11 +106,33 @@ static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>> }
>>
>> #ifdef CONFIG_SCSI_UFS_CRYPTO
>> +/**
>> + * ufs_qcom_config_ice_allocator() - ICE core allocator configuration
>> + *
>> + * @host: pointer to qcom specific variant structure.
>> + */
>> +static void ufs_qcom_config_ice_allocator(struct ufs_qcom_host *host)
>> +{
>> + struct ufs_hba *hba = host->hba;
>> + static const uint8_t val[4] = { NUM_RX_R1W0, NUM_TX_R0W1, NUM_RX_R1W1, NUM_TX_R1W1 };
>> + u32 config;
>> +
>> + if (!(host->caps & UFS_QCOM_CAP_ICE_CONFIG) ||
>> + !(host->hba->caps & UFSHCD_CAP_CRYPTO))
>> + return;
>> +
>> + config = get_unaligned_le32(val);
>> +
>> + ufshcd_writel(hba, ICE_ALLOCATOR_TYPE, REG_UFS_MEM_ICE_CONFIG);
>> + ufshcd_writel(hba, config, REG_UFS_MEM_ICE_NUM_CORE);
>> +}
>>
>> static inline void ufs_qcom_ice_enable(struct ufs_qcom_host *host)
>> {
>> if (host->hba->caps & UFSHCD_CAP_CRYPTO)
>> qcom_ice_enable(host->ice);
>> +
>> + ufs_qcom_config_ice_allocator(host);
>
> I did not ask you to move ufs_qcom_config_ice_allocator() inside
> ufs_qcom_ice_enable(). Rather do below in ufs_qcom_hce_enable_notify():
Hi Mani,
I have addressed this in the latest patchset.
Thanks,
Ram.
>
> ufs_qcom_config_ice_allocator(host);
> ufs_qcom_ice_enable(host);
>
>> }
>>
>> static int ufs_qcom_ice_init(struct ufs_qcom_host *host)
>> @@ -196,6 +219,11 @@ static inline int ufs_qcom_ice_suspend(struct ufs_qcom_host *host)
>> {
>> return 0;
>> }
>> +
>> +static void ufs_qcom_config_ice_allocator(struct ufs_qcom_host *host)
>> +{
>> +}
>> +
>> #endif
>>
>> static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)
>> @@ -932,6 +960,14 @@ static void ufs_qcom_set_host_params(struct ufs_hba *hba)
>> host_params->hs_tx_gear = host_params->hs_rx_gear = ufs_qcom_get_hs_gear(hba);
>> }
>>
>> +static void ufs_qcom_set_host_caps(struct ufs_hba *hba)
>> +{
>> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> +
>> + if (host->hw_ver.major >= 0x5)
>> + host->caps |= UFS_QCOM_CAP_ICE_CONFIG;
>> +}
>> +
>> static void ufs_qcom_set_caps(struct ufs_hba *hba)
>> {
>> hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
>> @@ -940,6 +976,8 @@ static void ufs_qcom_set_caps(struct ufs_hba *hba)
>> hba->caps |= UFSHCD_CAP_WB_EN;
>> hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
>> hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
>> +
>> + ufs_qcom_set_host_caps(hba);
>> }
>>
>> /**
>> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
>> index b9de170983c9..de41028ecee0 100644
>> --- a/drivers/ufs/host/ufs-qcom.h
>> +++ b/drivers/ufs/host/ufs-qcom.h
>> @@ -135,6 +135,46 @@ enum {
>> #define UNIPRO_CORE_CLK_FREQ_201_5_MHZ 202
>> #define UNIPRO_CORE_CLK_FREQ_403_MHZ 403
>>
>> +
>
> Extra newline
Hi Mani,
I have addressed this in the latest patchset.
Thanks,
Ram.
>
>> +#ifdef CONFIG_SCSI_UFS_CRYPTO
>
> As I said in previous iteration, you do not need the guard for definitions.
Hi Mani,
I have addressed this in the latest patchset.
Thanks,
Ram.
>
>> +
>> +/* ICE configuration to share AES engines among TX stream and RX stream */
>> +#define UFS_QCOM_CAP_ICE_CONFIG BIT(0)
>
> This is a bit definition. So define this as like other bit definitions in this
> header.
Hi Mani,
I have addressed this in the latest patchset.
Thanks,
Ram.
>
>> +#define ICE_ALLOCATOR_TYPE 2
>> +#define REG_UFS_MEM_ICE_CONFIG 0x260C
>> +#define REG_UFS_MEM_ICE_NUM_CORE 0x2664
>
> I asked you to move these two register definitions to register enum but still
> not addressed.
Hi Mani,
I have addressed this in the latest patchset.
Thanks,
Ram.
>
>> +
>> +/*
>> + * Number of cores allocated for RX stream when Read data block received and
>> + * Write data block is not in progress
>> + */
>> +#define NUM_RX_R1W0 28
>> +
>> +/*
>> + * Number of cores allocated for TX stream when Device asked to send write
>> + * data block and Read data block is not in progress
>> + */
>> +#define NUM_TX_R0W1 28
>> +
>> +/*
>> + * Number of cores allocated for RX stream when Read data block received and
>> + * Write data block is in progress
>> + * OR
>> + * Device asked to send write data block and Read data block is in progress
>> + */
>> +#define NUM_RX_R1W1 15
>> +
>> +/*
>> + * Number of cores allocated for TX stream (UFS write) when Read data block
>> + * received and Write data block is in progress
>> + * OR
>> + * Device asked to send write data block and Read data block is in progress
>> + */
>> +#define NUM_TX_R1W1 13
>> +
>> +#endif /* UFS_CRYPTO */
>> +
>
> Extra newline.
Hi Mani,
I have addressed this in the latest patchset.
Thanks,
Ram.
>
> - Mani
>
Powered by blists - more mailing lists