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

["Jain, Harsh (AECG-SSW)" <h.jain@amd.com>]

On 7/7/2026 3:42 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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?

We will check crypto and keyctl subsystem

>
>> 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?

I ran it on files instead of patches. Will take care next time.

Thanks for quick reply,

>
>> 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