Re: [PATCH 2/2] misc: Add Xilinx PUF driver

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jul 07, 2026 at 03:26:20PM +0530, Harsh Jain wrote:
> Versal devices contain a physically unclonable function (PUF) block
> that derives device-unique secrets from intrinsic silicon
> characteristics. The PUF generates a Key Encryption Key (KEK) used
> internally by the AES engine for secure key storage, which is not
> externally accessible. In addition, the PUF provides a readable
> device-unique identifier derived from the same entropy source.

So why isn't this tied into the normal crypto/keys/tee/whatever
subsystem for this type of hardware?  Why is a custom user/kernel api ok
for this one specific piece of hardware?

And where is the userspace code that interacts with this?

> Add a misc character driver to support PUF registration and regeneration.
> Registration generates a unique secret and outputs binary helper data
> that can later be used during regeneration to reproduce the same
> secrets.
> 
> The driver also implements PUF_CLEAR_ID and PUF_CLEAR_KEY ioctls to clear
> the PUF ID and PUF-derived AES key via firmware. They are disabled by default
> unless puf_clear is set.

Please wrap the changelog at 72 columns, checkpatch should have caught
this, right?

> 
> Signed-off-by: Harsh Jain <h.jain@amd.com>
> ---
>  MAINTAINERS                    |   6 +
>  drivers/misc/Kconfig           |  11 +
>  drivers/misc/Makefile          |   1 +
>  drivers/misc/xilinx_puf.c      | 377 +++++++++++++++++++++++++++++++++
>  include/uapi/misc/xilinx_puf.h |  82 +++++++
>  5 files changed, 477 insertions(+)
>  create mode 100644 drivers/misc/xilinx_puf.c
>  create mode 100644 include/uapi/misc/xilinx_puf.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 15011f5752a9..a46a34f163b4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -29546,6 +29546,12 @@ L:	netdev@vger.kernel.org
>  S:	Orphan
>  F:	drivers/net/ethernet/xilinx/ll_temac*
>  
> +XILINX PUF DRIVER
> +M:	Harsh Jain <h.jain@amd.com>
> +S:	Maintained

You aren't paid to support this?  AMD doesn't care?  That's sad.

> +F:	drivers/misc/xilinx_puf.c
> +F:	include/uapi/misc/xilinx_puf.h
> +
>  XILINX PWM DRIVER
>  M:	Sean Anderson <sean.anderson@linux.dev>
>  S:	Maintained
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 390256ed91f4..98ec00765647 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -490,6 +490,17 @@ config XILINX_SDFEC
>  
>  	  If unsure, say N.
>  
> +config XILINX_PUF
> +	tristate "Xilinx PUF driver"
> +	depends on ZYNQMP_FIRMWARE
> +	help
> +	  This option enables support for the Xilinx Physically Unclonable Function
> +	  (PUF) driver on AMD Versal devices.
> +	  It is a configurable driver to generate PUF KEK source either by using
> +	  PUF registration or regeneration command.
> +
> +	  If unsure, say N.

Module name?

> +
>  config MISC_RTSX



>  	tristate
>  	default MISC_RTSX_PCI || MISC_RTSX_USB
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index fed47c7672b9..66d1ea5d1f13 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_BCM_VK)		+= bcm-vk/
>  obj-y				+= cardreader/
>  obj-$(CONFIG_PVPANIC)   	+= pvpanic/
>  obj-$(CONFIG_UACCE)		+= uacce/
> +obj-$(CONFIG_XILINX_PUF)	+= xilinx_puf.o
>  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
>  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
>  obj-$(CONFIG_NTSYNC)		+= ntsync.o
> diff --git a/drivers/misc/xilinx_puf.c b/drivers/misc/xilinx_puf.c
> new file mode 100644
> index 000000000000..ad8ada58b105
> --- /dev/null
> +++ b/drivers/misc/xilinx_puf.c
> @@ -0,0 +1,377 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Xilinx PUF device.
> + *
> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
> + *
> + * Description:
> + * This driver is developed for PUF registration and regeneration support.
> + */
> +
> +#include <linux/dma-mapping.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <uapi/misc/xilinx_puf.h>
> +
> +static bool puf_clear;
> +module_param(puf_clear, bool, 0600);
> +MODULE_PARM_DESC(puf_clear, "Flag to enable clearing of PUF ID and key.");

This is not the 1990's, please do not add module parameters for things
that should be done other ways.  Especially for a device-specific thing.

> +
> +/**
> + * struct puf_params - parameters for PUF
> + * @pufoperation: PUF registration or regeneration operation
> + * @globalvarfilter: global variation filter
> + * @readoption: option to read PUF data from efuse cache or ram address
> + * @reserved: explicit padding to match the Versal PLM PUF parameter layout.
> + * @shuttervalue: shutter value for PUF registration/regeneration
> + * @readsyndromeaddr: address to store the syndrome data during registration
> + * @chashaddr: CHASH address
> + * @auxaddr: AUX address
> + * @pufidaddr: PUF ID address
> + * @writesyndromeaddr: address where syndrome data is present and it is passed to the user
> + * @trimsyndataaddr: trimmed syndrome data will be stored
> + */
> +struct puf_params {
> +	u8 pufoperation;
> +	u8 globalvarfilter;
> +	u8 readoption;
> +	u8 reserved;
> +	u32 shuttervalue;
> +	u64 readsyndromeaddr;
> +	u64 chashaddr;
> +	u64 auxaddr;
> +	u64 pufidaddr;
> +	u64 writesyndromeaddr;
> +	u64 trimsyndataaddr;
> +};
> +
> +/**
> + * struct xpuf_dev - Driver data for PUF
> + * @dev: pointer to device struct
> + * @miscdev: misc device handle
> + */
> +struct xpuf_dev {
> +	struct device *dev;
> +	struct miscdevice miscdev;
> +};
> +
> +static int xlnx_puf_regis(struct xpuf_dev *puf, struct puf_usrparams *pufreq)
> +{
> +	struct puf_params *pufin;
> +	struct pufdata *pufdat;
> +	dma_addr_t dma_addr_data;
> +	dma_addr_t dma_addr_in;
> +	u32 buflen;
> +	void *buf;
> +	int ret;
> +
> +	if (pufreq->pufoperation != PUF_REGIS)
> +		return -EINVAL;
> +
> +	if (pufreq->readoption != PUF_READ_FROM_RAM &&
> +	    pufreq->readoption != PUF_READ_FROM_EFUSE_CACHE)
> +		return -EINVAL;
> +
> +	buflen = sizeof(struct puf_params) + sizeof(struct pufdata);
> +	buf = kzalloc(buflen, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	pufin = buf;
> +	pufin->pufoperation = pufreq->pufoperation;
> +	pufin->globalvarfilter = pufreq->globalvarfilter;
> +	pufin->readoption = pufreq->readoption;
> +	pufin->shuttervalue = pufreq->shuttervalue;
> +
> +	pufdat = buf + sizeof(struct puf_params);
> +	dma_addr_in = dma_map_single(puf->dev, buf, buflen, DMA_BIDIRECTIONAL);
> +	if (dma_mapping_error(puf->dev, dma_addr_in)) {
> +		ret = -ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	dma_addr_data = dma_addr_in + sizeof(struct puf_params);
> +	pufin->readsyndromeaddr = (u64)dma_addr_data;
> +	pufin->chashaddr = (u64)(pufin->readsyndromeaddr + sizeof(pufdat->pufhd.syndata));
> +	pufin->auxaddr = (u64)(pufin->chashaddr + sizeof(pufdat->pufhd.chash));
> +	pufin->pufidaddr = (u64)(pufin->auxaddr + sizeof(pufdat->pufhd.aux));
> +	pufin->trimsyndataaddr = (u64)(pufin->pufidaddr + sizeof(pufdat->pufid));
> +
> +	dma_sync_single_for_device(puf->dev, dma_addr_in, buflen, DMA_BIDIRECTIONAL);
> +	ret = versal_pm_puf_registration(dma_addr_in);
> +	dma_unmap_single(puf->dev, dma_addr_in, buflen, DMA_BIDIRECTIONAL);
> +	if (ret)
> +		goto cleanup;
> +
> +	if (copy_to_user(u64_to_user_ptr(pufreq->pufdataaddr), pufdat, sizeof(struct pufdata))) {
> +		ret = -EFAULT;
> +		goto cleanup;
> +	}
> +
> +cleanup:
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
> +static int xlnx_puf_regen_id(struct xpuf_dev *puf, struct puf_usrparams *pufreq)
> +{
> +	struct puf_helperdata *pufhd;
> +	struct puf_params *pufin;
> +	dma_addr_t dma_addr_data;
> +	dma_addr_t dma_addr_in;
> +	u32 buflen;
> +	void *buf;
> +	int ret;
> +
> +	if (pufreq->pufoperation != PUF_REGEN &&
> +	    pufreq->pufoperation != PUF_REGEN_ID)
> +		return -EINVAL;
> +
> +	if (pufreq->readoption != PUF_READ_FROM_RAM &&
> +	    pufreq->readoption != PUF_READ_FROM_EFUSE_CACHE)
> +		return -EINVAL;
> +
> +	buflen = sizeof(struct puf_params) + sizeof(struct puf_helperdata) +
> +		 PUF_ID_LEN_IN_BYTES;
> +
> +	buf = kzalloc(buflen, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	pufin = buf;
> +	pufin->pufoperation = pufreq->pufoperation;
> +	pufin->globalvarfilter = pufreq->globalvarfilter;
> +	pufin->shuttervalue = pufreq->shuttervalue;
> +	pufin->readoption = pufreq->readoption;
> +
> +	pufhd = buf + sizeof(struct puf_params);
> +	if (copy_from_user(pufhd, u64_to_user_ptr(pufreq->pufdataaddr),
> +			   sizeof(struct puf_helperdata))) {
> +		ret = -EFAULT;
> +		goto cleanup;
> +	}
> +
> +	dma_addr_in = dma_map_single(puf->dev, buf, buflen, DMA_BIDIRECTIONAL);
> +	if (dma_mapping_error(puf->dev, dma_addr_in)) {
> +		ret = -ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	dma_addr_data = dma_addr_in + sizeof(struct puf_params);
> +	pufin->writesyndromeaddr = (u64)dma_addr_data;
> +	pufin->chashaddr = (u64)(pufin->writesyndromeaddr + sizeof(pufhd->syndata));
> +	pufin->auxaddr = (u64)(pufin->chashaddr + sizeof(pufhd->chash));
> +	pufin->pufidaddr = (u64)(pufin->auxaddr + sizeof(pufhd->aux));
> +
> +	dma_sync_single_for_device(puf->dev, dma_addr_in, buflen, DMA_BIDIRECTIONAL);
> +	ret = versal_pm_puf_regeneration(dma_addr_in);
> +	dma_unmap_single(puf->dev, dma_addr_in, buflen, DMA_BIDIRECTIONAL);
> +	if (ret)
> +		goto cleanup;
> +
> +	if (copy_to_user(u64_to_user_ptr(pufreq->pufidaddr), ((char *)pufhd +
> +			 sizeof(struct puf_helperdata)),
> +			 PUF_ID_LEN_IN_BYTES)) {
> +		ret = -EFAULT;
> +		goto cleanup;
> +	}
> +
> +cleanup:
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
> +static long xlnx_puf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct xpuf_dev *puf = file->private_data;
> +	struct puf_usrparams pufreq;
> +	void __user *data = NULL;
> +	int ret;
> +
> +	if (_IOC_TYPE(cmd) != PUF_IOC_MAGIC)
> +		return -ENOTTY;
> +
> +	/* check if ioctl argument is present and valid */
> +	if (_IOC_DIR(cmd) != _IOC_NONE) {
> +		data = (void __user *)arg;
> +		if (!data)
> +			return -EINVAL;
> +
> +		if (copy_from_user(&pufreq, data, sizeof(struct puf_usrparams)))
> +			return -EFAULT;
> +	}
> +
> +	switch (cmd) {
> +	case PUF_REGISTRATION:
> +		ret = xlnx_puf_regis(puf, &pufreq);
> +		break;
> +	case PUF_REGENERATION:
> +	case PUF_REGEN_ID_ONLY:
> +		ret = xlnx_puf_regen_id(puf, &pufreq);
> +		break;
> +	case PUF_CLEAR_ID:
> +		if (!puf_clear) {
> +			ret = -EOPNOTSUPP;
> +			break;
> +		}
> +		ret = versal_pm_puf_clear_id();
> +		break;
> +	case PUF_CLEAR_KEY:
> +		if (!puf_clear) {
> +			ret = -EOPNOTSUPP;
> +			break;
> +		}
> +		ret = versal_pm_aes_init();
> +		if (!ret)
> +			ret = versal_pm_puf_key_zero();
> +		break;
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * xlnx_puf_open - open puf device
> + * @inode:	inode object
> + * @file:	file object
> + *
> + * Return:	0 if successful; otherwise -errno
> + */
> +static int xlnx_puf_open(struct inode *inode, struct file *file)
> +{
> +	struct xpuf_dev *xpuf;
> +
> +	xpuf = container_of(file->private_data, struct xpuf_dev, miscdev);
> +	file->private_data = xpuf;
> +
> +	dev_dbg(xpuf->dev, "device /dev/xpuf opened\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * xlnx_puf_release - release puf resources
> + * @inode:	inode object
> + * @file:	file object
> + *
> + * Return:	0 if successful; otherwise -errno
> + */
> +static int xlnx_puf_release(struct inode *inode, struct file *file)
> +{
> +	struct xpuf_dev *xpuf = file->private_data;
> +
> +	dev_dbg(xpuf->dev, "device /dev/xpuf closed\n");
> +
> +	return 0;
> +}

This does nothing, why is it needed at all?

> +
> +static const struct file_operations xlnx_puf_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= xlnx_puf_open,
> +	.release	= xlnx_puf_release,
> +	.unlocked_ioctl	= xlnx_puf_ioctl,
> +	.compat_ioctl	= compat_ptr_ioctl,
> +};
> +
> +/**
> + * xlnx_puf_probe - probe puf device
> + * @pdev: Pointer to puf platform device structure
> + *
> + * Return: 0 if successful; otherwise -errno
> + */
> +static int xlnx_puf_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct xpuf_dev *xpuf;
> +	int ret;
> +
> +	xpuf = devm_kzalloc(dev, sizeof(*xpuf), GFP_KERNEL);
> +	if (!xpuf)
> +		return -ENOMEM;
> +
> +	xpuf->dev = dev;
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +	if (ret < 0) {
> +		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +		if (ret < 0) {
> +			dev_err(dev, "no usable DMA configuration\n");
> +			return ret;
> +		}
> +	}
> +
> +	xpuf->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	xpuf->miscdev.name = "xpuf";
> +	xpuf->miscdev.fops = &xlnx_puf_fops;
> +	xpuf->miscdev.parent = dev;
> +
> +	ret = misc_register(&xpuf->miscdev);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, xpuf);
> +
> +	dev_dbg(dev, "puf registered as /dev/xpuf successfully\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * xlnx_puf_remove - clean up structures
> + * @pdev:	The structure containing the device's details
> + */
> +static void xlnx_puf_remove(struct platform_device *pdev)
> +{
> +	struct xpuf_dev *xpuf = platform_get_drvdata(pdev);
> +
> +	misc_deregister(&xpuf->miscdev);
> +
> +	dev_dbg(xpuf->dev, "device /dev/xpuf removed\n");

ftrace is your friend :)

> +}
> +
> +static struct platform_driver xlnx_puf_drv = {
> +	.probe = xlnx_puf_probe,
> +	.remove = xlnx_puf_remove,
> +	.driver = {
> +		.name = "xlnx-puf",
> +	},
> +};
> +
> +static struct platform_device *xlnx_puf_pdev;
> +
> +static int __init xlnx_puf_driver_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&xlnx_puf_drv);
> +	if (ret)
> +		return ret;
> +
> +	xlnx_puf_pdev = platform_device_register_simple(xlnx_puf_drv.driver.name,
> +							0, NULL, 0);

Why isn't this using the faux bus instead?  This isn't a real platform
device at all as you have no resources for it that describe it.




> +	if (IS_ERR(xlnx_puf_pdev)) {
> +		ret = PTR_ERR(xlnx_puf_pdev);
> +		platform_driver_unregister(&xlnx_puf_drv);
> +	}
> +
> +	return ret;
> +}
> +
> +static void __exit xlnx_puf_driver_exit(void)
> +{
> +	platform_device_unregister(xlnx_puf_pdev);
> +	platform_driver_unregister(&xlnx_puf_drv);
> +}
> +
> +module_init(xlnx_puf_driver_init);
> +module_exit(xlnx_puf_driver_exit);
> +
> +MODULE_AUTHOR("AMD");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Xilinx Versal PUF driver");
> diff --git a/include/uapi/misc/xilinx_puf.h b/include/uapi/misc/xilinx_puf.h
> new file mode 100644
> index 000000000000..32565a748df3
> --- /dev/null
> +++ b/include/uapi/misc/xilinx_puf.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Driver for Xilinx PUF device.
> + *
> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
> + *
> + * Description:
> + * This driver is developed for PUF registration and regeneration support.
> + */
> +
> +#ifndef _UAPI_MISC_XILINX_PUF_H_
> +#define _UAPI_MISC_XILINX_PUF_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +#define PUF_MAX_SYNDROME_DATA_LEN_IN_WORDS	140
> +#define PUF_EFUSE_TRIM_SYN_DATA_IN_WORDS	127
> +#define PUF_ID_LEN_IN_WORDS			8
> +#define PUF_ID_LEN_IN_BYTES			32
> +#define PUF_REGIS				0
> +#define PUF_REGEN				1
> +#define PUF_REGEN_ID				2
> +
> +/**
> + * struct puf_usrparams - user parameters for PUF from user space.
> + * @pufoperation: PUF registration or regeneration operation
> + * @globalvarfilter: global variation filter
> + * @readoption: option to read PUF data from efuse cache or ram address
> + * @reserved: explicit padding to match the Versal PLM PUF parameter layout
> + * @shuttervalue: shutter value for PUF registration/regeneration
> + * @pufdataaddr: address to store/get the puf data during registration/regeneration
> + * @pufidaddr: puf id will be stored either during registration/regeneration
> + */
> +struct puf_usrparams {
> +	__u8 pufoperation;
> +	__u8 globalvarfilter;
> +	__u8 readoption;
> +	__u8 reserved;
> +	__u32 shuttervalue;
> +	__u64 pufdataaddr;
> +	__u64 pufidaddr;
> +};
> +
> +/**
> + * struct puf_helperdata - parameters for puf helper data.
> + * @syndata: PUF syndrome data
> + * @chash: PUF chash
> + * @aux: PUF aux
> + */
> +struct puf_helperdata {
> +	__u32 syndata[PUF_MAX_SYNDROME_DATA_LEN_IN_WORDS];
> +	__u32 chash;
> +	__u32 aux;
> +};
> +
> +/**
> + * struct pufdata - parameters for puf data.
> + * @pufhd: puf helper data of type struct puf_helperdata
> + * @pufid: PUF id
> + * @efusesyndata: PUF efuse syndrome data
> + */
> +struct pufdata {
> +	struct puf_helperdata pufhd;
> +	__u32 pufid[PUF_ID_LEN_IN_WORDS];
> +	__u32 efusesyndata[PUF_EFUSE_TRIM_SYN_DATA_IN_WORDS];
> +};
> +
> +enum pufreadoption {
> +	PUF_READ_FROM_RAM = 0,
> +	PUF_READ_FROM_EFUSE_CACHE = 1
> +};
> +
> +#define PUF_IOC_MAGIC 'P'

Why are you allowed to use "P" for your ioctl?  Isn't that already used
by other drivers?

thanks,

greg k-h