On Fri, 15 Jan 2021 at 18:56, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > On Fri, Jan 15, 2021 at 10:22:03AM +0100, Ulf Hansson wrote: > > On Mon, 4 Jan 2021 at 19:48, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > > > From: Eric Biggers <ebiggers@xxxxxxxxxx> > > > > > > In preparation for adding CQHCI crypto engine (inline encryption) > > > support, add the code required to make mmc_core and mmc_block aware of > > > inline encryption. Specifically: > > > > > > - Add a capability flag MMC_CAP2_CRYPTO to struct mmc_host. Drivers > > > will set this if the host and driver support inline encryption. > > > > > > - Embed a blk_keyslot_manager in struct mmc_host. Drivers will > > > initialize this if the host and driver support inline encryption. > > > mmc_block registers this keyslot manager with the request_queue of any > > > MMC card attached to the host. mmc_core destroys this keyslot manager > > > when freeing the mmc_host. > > > > > > - Make mmc_block copy the crypto keyslot and crypto data unit number > > > from struct request to struct mmc_request, so that drivers will have > > > access to them. > > > > > > - If the MMC host is reset, reprogram all the keyslots to ensure that > > > the software state stays in sync with the hardware state. > > > > > > Co-developed-by: Satya Tangirala <satyat@xxxxxxxxxx> > > > Signed-off-by: Satya Tangirala <satyat@xxxxxxxxxx> > > > Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx> > > > Reviewed-by: Satya Tangirala <satyat@xxxxxxxxxx> > > > Reviewed-and-tested-by: Peng Zhou <peng.zhou@xxxxxxxxxxxx> > > > Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx> > > > > Eric, again, my apologies for the delay. Overall, I think this looks good. > > > > My only hesitation to merge this as is, is that I want to make sure > > you have thought of the life cycle issues for the struct > > blk_keyslot_manager ksm. It's being used both from the mmc core/block > > device driver and the mmc host driver. I am looking at this right now > > and will get back to you very soon, if I find some issues with it. > > > > If you have some time, feel free to elaborate around how this is > > intended to work. > > > > Kind regards > > Uffe > > The blk_keyslot_manager is initialized early on when the other host structures > (struct mmc_host, struct cqhci_host, struct sdhci_host, struct sdhci_msm_host) > are initialized, prior to mmc_add_host(). > > It is destroyed when the struct mmc_host is freed by mmc_free_host(). > > So it should just work; it's the same lifecycle as the existing host structures. > Is there something you think I'm overlooking? I think so, but let me elaborate a bit. As I understand it, to initialize the data structures, blk_ksm_init() is getting called and via cqhci_init(). To hook up the block request queue, blk_ksm_register() is called via mmc_setup_queue(), which means this happens when the mmc block device driver is probed. To free up the data structures, blk_ksm_destroy() is called from mmc_free_host(). To me, this can be made more consistent. For example, it looks like blk_ksm_destroy() could be called, even if blk_ksm_init() hasn't been called (depending on the probe error path of the mmc host). There are a couple of options to better deal with this. 1) Extend the blk_ksm interface with a devm_blk_ksm_init() function (thus let it deal with lifecycle problems for us) and simply drop the call to blk_ksm_destroy(). 2) Extend the cqhci interface with a cleanup function (perhaps "cqhci_deinit") and let it call blk_ksm_destroy(). 3) Convert to let blk_ksm_init() to be called from mmc_add_host() and blk_ksm_destroy() from mmc_remove_host(). Moreover, even if there seems to be no real need to call blk_ksm_unregister() for the mmc block device driver, perhaps we should still do it to be consistent with blk_ksm_register()? Then a final concern. It looks like the mmc core relies on checking "host->caps2 & MMC_CAP2_CRYPTO", when it calls blk_ksm_register() and blk_ksm_reprogram_all_keys(), for example. Normally, host->caps2 bits are considered as static configurations and set during the host driver probe path, which may not be a good match for this case. Instead, it seems like we should set a new separate flag, to indicate for the mmc core that blk_ksm_init has been enabled. Otherwise it looks like we could end up calling blk_ksm_reprogram_all_keys(), even if blk_ksm_init() hasn't been called. Kind regards Uffe
