Hi Peng, thanks for your patch! On Tue, Mar 9, 2021 at 3:06 AM Peng Zhou <peng.zhou@xxxxxxxxxxxx> wrote: > Use SMC call enable hardware crypto engine > due to it only be changed in ATF(EL3). > > Signed-off-by: Peng Zhou <peng.zhou@xxxxxxxxxxxx> Unfortunately this commit message is way to short to understand what is going on, and has a lot of assumed previous knowledge. Can you expand the commit message so that anyone who just know MMC and some SoC basics can understand what an SMC call and and what ATF(EL3) means? I assume this some kind of inline encryption? I think maybe linux-block mailing list need to be involved because there is certain a Linux standard way of setting up inline encryption for the block layer. For example: how is the key to be used derived? How is the device unlocked in the first place? If I insert a LUKS encrypted harddrive in a Linux machine the whole system is pretty much aware of how this should be handled and everything "just works", I enter a pass phrase and off it goes. I can use symmetric keys as well. How is this stuff done for this hardware? > + /* > + * 1: MSDC_AES_CTL_INIT > + * 4: cap_id, no-meaning now > + * 1: cfg_id, we choose the second cfg group > + */ > + if (mmc->caps2 & MMC_CAP2_CRYPTO) > + arm_smccc_smc(MTK_SIP_MMC_CONTROL, > + 1, 4, 1, 0, 0, 0, 0, &smccc_res); The same as above: these comments assume that everyone already knows what is going on. AES encryption requires a key and I don't see the driver setting up any key. How is the code in this file: drivers/mmc/core/crypto.c interacting with your driver? drivers/mmc/host/cqhci-crypto.c is used by SDHCI and is quite readable and I see what is going on. For example it contains functions like: cqhci_crypto_program_key() cqhci_crypto_keyslot_program() cqhci_crypto_clear_keyslot() cqhci_crypto_keyslot_evict() cqhci_find_blk_crypto_mode() MMC_CAP2_CRYPTO is used as a sign that the driver can do inline encryption, then devm_blk_ksm_init() is called to initialize a block encryption abstraction with the block layer. Ops are registered using struct blk_ksm_ll_ops cqhci_ksm_ops. This is very straight forward. But where does all the above happen for this driver? I get the feeling that some magic is happening in outoftree patches or in the secure world, and that is not how we do these things, you have to use the frameworks. Yours, Linus Walleij
