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

[Md Sadre Alam]

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.