Re: [RFC PATCH 04/21] PCI/IDE: Define Integrity and Data Encryption (IDE) extended capability

[Dan Williams <dan.j.williams@intel.com>]

Alexey Kardashevskiy wrote:
> PCIe 6.0 introduces the "Integrity & Data Encryption (IDE)" feature which
> adds a new capability with id=0x30.
> 
> Add the new id to the list of capabilities. Add new flags from pciutils.
> Add a module with a helper to control selective IDE capability.
> 
> TODO: get rid of lots of magic numbers. It is one annoying flexible
> capability to deal with.

This changelog needs a theory of operation to explain how it is used and
to give some chance of reviewing whether this is implementing more than
is required to get the job done.

> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> ---
>  drivers/pci/Makefile          |   1 +
>  include/linux/pci-ide.h       |  18 ++
>  include/uapi/linux/pci_regs.h |  76 +++++++-
>  drivers/pci/ide.c             | 186 ++++++++++++++++++++
>  drivers/pci/Kconfig           |   4 +
>  5 files changed, 284 insertions(+), 1 deletion(-)
> 
[..]
> diff --git a/include/linux/pci-ide.h b/include/linux/pci-ide.h
> new file mode 100644
> index 000000000000..983a8daf1199
> --- /dev/null
> +++ b/include/linux/pci-ide.h
[..]
> diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> new file mode 100644
> index 000000000000..dc0632e836e7
> --- /dev/null
> +++ b/drivers/pci/ide.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Integrity & Data Encryption (IDE)
> + *	PCIe r6.0, sec 6.33 DOE
> + */
> +
> +#define dev_fmt(fmt) "IDE: " fmt
> +
> +#include <linux/pci.h>
> +#include <linux/pci-ide.h>
> +#include <linux/bitfield.h>
> +#include <linux/module.h>
> +
> +#define DRIVER_VERSION	"0.1"
> +#define DRIVER_AUTHOR	"aik@amd.com"
> +#define DRIVER_DESC	"Integrity and Data Encryption driver"

This is not a driver. It is a helper library for the TSM core and maybe
native PCI IDE establishment in the future.

I am not what purpose AUTHOR, VERSION and DESC, serve as I do not see
how this can get away with being a module when the TSM core is built-in
to be initialized via pci_init_capabilities().

[..]

Difficult to review the implementation without a clear idea of the user,
and the exports are not necessary if the only consumer is the PCI core.

> +
> +static int __init ide_init(void)
> +{
> +	int ret = 0;
> +
> +	pr_info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
> +	return ret;
> +}
> +
> +static void __exit ide_cleanup(void)
> +{
> +}
> +
> +module_init(ide_init);
> +module_exit(ide_cleanup);
> +
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index b0b14468ba5d..8e908d684c77 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -137,6 +137,10 @@ config PCI_CMA
>  config PCI_DOE
>  	bool
>  
> +config PCI_IDE
> +	tristate
> +	default m

Setting aside the tristate, no new kernel code should unconditionally
enable itself by default.