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 <quic_gaurkash@xxxxxxxxxxx> > --- > 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. > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index 79b568f82a1c..39f223ed8cdd 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -209,4 +209,11 @@ config QCOM_APR > application processor and QDSP6. APR is > used by audio driver to configure QDSP6 > ASM, ADM and AFE modules. > + > +config QTI_ICE_COMMON > + tristate "QTI common ICE functionality" > + depends on SCSI_UFS_CRYPTO && SCSI_UFS_QCOM > + help > + Enable the common ICE library that can be used > + by UFS and EMMC drivers for ICE functionality. "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. > diff --git a/include/linux/qti-ice-common.h b/include/linux/qti-ice-common.h > new file mode 100644 > index 000000000000..433422b34a7d > --- /dev/null > +++ b/include/linux/qti-ice-common.h > @@ -0,0 +1,26 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2021, The Linux Foundation. All rights reserved. > + */ > + > +#ifndef _QTI_ICE_COMMON_H > +#define _QTI_ICE_COMMON_H > + > +#include <linux/types.h> > +#include <linux/device.h> > + > +#define AES_256_XTS_KEY_SIZE 64 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
![]() |