On Wed, Nov 03, 2021 at 04:18:37PM -0700, Gaurav Kashyap wrote:

The Inline Crypto Engine functionality is not limited to
ufs and it can be used by other storage controllers like emmc
which have the HW capabilities. It would be better to move this
functionality to a common location.

I think you should be a bit more concrete here: both sdhci-msm and ufs-qcom
already have ICE support, and this common library allows code to be shared.

Moreover, when wrapped key functionality is added, it would
reduce the effort required to add it for all storage
controllers.

Signed-off-by: Gaurav Kashyap <redacted>
---
 drivers/scsi/ufs/ufs-qcom-ice.c   | 172 ++++--------------------------
 drivers/soc/qcom/Kconfig          |   7 ++
 drivers/soc/qcom/Makefile         |   1 +
 drivers/soc/qcom/qti-ice-common.c | 135 +++++++++++++++++++++++
 drivers/soc/qcom/qti-ice-regs.h   | 145 +++++++++++++++++++++++++
 include/linux/qti-ice-common.h    |  26 +++++
 6 files changed, 334 insertions(+), 152 deletions(-)
 create mode 100644 drivers/soc/qcom/qti-ice-common.c
 create mode 100644 drivers/soc/qcom/qti-ice-regs.h
 create mode 100644 include/linux/qti-ice-common.h

This should be split up into two patches: one that adds the library, and one
that converts ufs-qcom to use it.  There should also be a third patch that
converts sdhci-msm to use it.

+static void get_ice_mmio_data(struct ice_mmio_data *data,
+			      const struct ufs_qcom_host *host)
+{
+	data->ice_mmio = host->ice_mmio;
+}

I think the struct ice_mmio_data should just be a field of struct ufs_qcom_host.
Then you wouldn't have to keep initializing a new one.

"Libraries" should not be user-selectable.  Instead, they should be selected by
the kconfig options that need them.  That also means that the "depends on"
clause should not be here.

So it should look more like:

config QTI_ICE_COMMON
	tristate
	help
	  Enable the common ICE library that can be used
	  by UFS and EMMC drivers for ICE functionality.

If the library itself has dependencies (perhaps ARCH_QCOM?), then add those.

+
+int qti_ice_init(const struct ice_mmio_data *mmio)
+{
+	return qti_ice_supported(mmio);
+}
+EXPORT_SYMBOL(qti_ice_init);

Please use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL.

The exported functions could also use kerneldoc comments.

Is the definition of AES_256_XTS_KEY_SIZE needed in this header?  It's not
properly "namespaced", so it's sort of the odd thing out in this header.

- Eric