linux-kernel - Re: [PATCH v5] mmc: sdhci-msm: Enable ICE for CQE-capable controllers with non-CQE cards

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] [day] [month] [year] [list]

Message-ID: <4b65983a-4450-5d00-b52b-0bd5b1f7221b@quicinc.com>
Date: Fri, 21 Nov 2025 12:03:54 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Eric Biggers <ebiggers@...nel.org>
CC: <adrian.hunter@...el.com>, <ulf.hansson@...aro.org>,
        <abel.vesa@...aro.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-mmc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] mmc: sdhci-msm: Enable ICE for CQE-capable controllers
 with non-CQE cards

Hi,

On 11/21/2025 7:42 AM, Eric Biggers wrote:
> On Wed, Nov 19, 2025 at 05:16:53PM +0530, Md Sadre Alam wrote:
>>   struct sdhci_msm_offset {
>>   	u32 core_hc_mode;
>>   	u32 core_mci_data_cnt;
>> @@ -300,6 +312,7 @@ struct sdhci_msm_host {
>>   	u32 dll_config;
>>   	u32 ddr_config;
>>   	bool vqmmc_enabled;
>> +	bool ice_init_done;
> 
> Rename to non_cqe_ice_init_done
Ok
> 
>> +static void sdhci_msm_non_cqe_ice_init(struct sdhci_host *host)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	struct mmc_host *mmc = msm_host->mmc;
>> +	struct cqhci_host *cq_host = mmc->cqe_private;
>> +	u32 config;
>> +	u32 ice_cap;
>> +
>> +	config = sdhci_readl(host, HC_VENDOR_SPECIFIC_FUNC4);
>> +	config &= ~DISABLE_CRYPTO;
>> +	sdhci_writel(host, config, HC_VENDOR_SPECIFIC_FUNC4);
>> +	ice_cap = cqhci_readl(cq_host, CQHCI_CAP);
>> +	if (ice_cap & ICE_HCI_SUPPORT) {
>> +		config = cqhci_readl(cq_host, CQHCI_CFG);
>> +		config |= CRYPTO_GENERAL_ENABLE;
>> +		cqhci_writel(cq_host, config, CQHCI_CFG);
>> +	}
>> +}
> 
> Why would ICE_HCI_SUPPORT not be set here?  When this is called, the
> driver is already in the middle of processing an mmc_request with an
> encryption context, due to the driver advertising that it supports
> inline crypto earlier.  If the hardware doesn't actually support inline
> crypto, that has to be detected earlier.  But I thought it already does.
> So it's unclear what checking ICE_HCI_SUPPORT here is meant to achieve.
You're right — by the time we reach sdhci_msm_request(), the driver has 
already advertised inline crypto support and is processing a request 
with an encryption context.The check for ICE_HCI_SUPPORT here seems
redundant given the existing capability detection.
I’ll drop this check in next revision.

> 
>> +static int sdhci_msm_ice_cfg(struct sdhci_host *host, struct mmc_request *mrq)
> 
> This should return void.
> 
>> +	if (mrq->crypto_ctx) {
>> +		if (!msm_host->ice_init_done) {
>> +			sdhci_msm_non_cqe_ice_init(host);
>> +			msm_host->ice_init_done = true;
>> +		}
>> +
>> +		crypto_enable = true;
>> +		dun = mrq->crypto_ctx->bc_dun[0];
>> +		key_index = mrq->crypto_key_slot;
>> +		crypto_params = FIELD_PREP(ICE_HCI_PARAM_CE, crypto_enable) |
>> +				FIELD_PREP(ICE_HCI_PARAM_CCI, key_index);
>> +
>> +		cqhci_writel(cq_host, crypto_params, NONCQ_CRYPTO_PARM);
>> +		cqhci_writel(cq_host, lower_32_bits(dun), NONCQ_CRYPTO_DUN);
> 
> No need for the crypto_enable variable.  Just use:
> 
> 	FIELD_PREP(ICE_HCI_PARAM_CE, 1).
ok
> 
> Also no need for the dun variable.  Just use:
> 
> 	cqhci_writel(cq_host, lower_32_bits(mrq->crypto_ctx->bc_dun[0]),
> 		     NONCQ_CRYPTO_DUN);
ok
> 
>> + * For CQE requests, crypto is handled in cqhci_request() in
>> + * drivers/mmc/host/cqhci-core.c using the existing CQE crypto infrastructure.
> 
> It's recommended to not reference file paths like this in kernel code,
> since files are sometimes moved around.
Sure, Will rewrite the comment in next revision.

Thanks,
Alam.