On Thu, 15 May 2025, Ron Li wrote:
> Add the mlxbf_pka driver to support the BlueField SoC Public Key
> Acceleration (PKA) hardware. The PKA provides a simple, complete
> framework for crypto public key hardware offload. It supports direct
> access to the public key hardware resources from the user space, and
> makes available several arithmetic operations: some basic operations
> (e.g., addition and multiplication), some complex operations (e.g.,
> modular exponentiation and modular inversion), and high-level
> operations such as RSA, Diffie-Hallman, Elliptic Curve Cryptography,
> and the Federal Digital Signature Algorithm (DSA as documented in
> FIPS-186) public-private key systems.
>
> The PKA driver initializes the PKA hardware interface and implements
> file operations so that user space libraries can bypass the kernel and
> have direct access to a specific set of device registers. The Arm cores
> interface to the PKA hardware through rings and a 64KB memory known as
> Window RAM. There are multiple PKA devices on the BlueField SoC. In
> general, each PKA device has 4 rings, 1 window RAM and 1 True Random
> Number Generator (TRNG). Thus, the driver has been designed to probe
> each PKA and each individual ring inside a given PKA. It also registers
> the TRNG to feed the kernel entropy (i.e., /dev/hwrng). To implement
> such design, the driver creates individual device files for each ring
> and TRNG module. The ring device files are identified using their ids,
> i.e., /dev/mlxbf_pka/<ring_id>.
>
> The main driver logic such as probe() and remove() are implemented in
> mlxbf_pka_drv.c. The PKA ring device operations are also implemented in
> this source file, such as open(), release() and mmap().
>
> The mlxbf_pka_dev.c source file implements functions to operate the
> underlying PKA hardware, such as TRNG operation, PKA hardware I/O
> access, PKA memory resource operation, etc.
>
> The PKA driver is a lighweight driver that implements file operations
> and map memory regions of the PKA hardware to user space drivers and
> libraries. There is no in-kernel crypto support. Therefore, the PKA
> driver is included under drivers/platform/mellanox.
>
> Testing
>
> - Successful build of kernel for ARM64.
>
> - Tested ARM64 build on several Mellanox BlueField 2 and 3 SoC boards
> that include the PKA hardware. The testing includes the validation of
> the PKA hardware execution, random number generation and public key
> acceleration performance.
Hi,
We've the in-kernel crypto framework but I don't see any attempt to build
into that framework AFAICT. Why is that? You brush it off as "The PKA
driver is a lightweight driver ..." but lets see if the crypto people
agree with that approach (I added them).
(Please also Cc crypto people in any further submission.)
> Reviewed-by: David Thompson <davthompson@xxxxxxxxxx>
> Reviewed-by: Khalil Blaiech <kblaiech@xxxxxxxxxx>
Remove the extra space. x2
> Signed-off-by: Ron Li <xiangrongl@xxxxxxxxxx>
> ---
> .../ABI/testing/sysfs-platform-mellanox-pka | 35 +
> MAINTAINERS | 8 +
> drivers/platform/mellanox/Kconfig | 10 +
> drivers/platform/mellanox/Makefile | 1 +
> drivers/platform/mellanox/mlxbf_pka/Makefile | 10 +
> .../mellanox/mlxbf_pka/mlxbf_pka_dev.c | 1891 +++++++++++++++++
> .../mellanox/mlxbf_pka/mlxbf_pka_dev.h | 657 ++++++
> .../mellanox/mlxbf_pka/mlxbf_pka_drv.c | 1066 ++++++++++
> 8 files changed, 3678 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-platform-mellanox-pka
> create mode 100644 drivers/platform/mellanox/mlxbf_pka/Makefile
> create mode 100644 drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
> create mode 100644 drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
> create mode 100644 drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-mellanox-pka b/Documentation/ABI/testing/sysfs-platform-mellanox-pka
> new file mode 100644
> index 000000000000..2ec5ed70dbf7
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-mellanox-pka
> @@ -0,0 +1,35 @@
> +What: /sys/devices/platform/<pka-device-id>/<pka-ring-device-id>
> +Date: Jun 2025
> +KernelVersion: 6.15
> +Contact: "Ron Li <xiangrongl@xxxxxxxxxx>"
> +Description:
> + The mlxbf_pka driver to support the BlueField SoC Public Key Acceleration (PKA)
> + hardware. It supports direct access to the public key hardware resources from the
> + user space.
> +
> + There are three PKA device IDs that support different BlueField product:
> +
> + =========== ==============================================
> + BlueField-1 MLNXBF10:xx, where xx ranges from '00' to '03'
> + BlueField-2 MLNXBF20:xx, where xx ranges from '00' to '07'
> + BlueField-3 MLNXBF51:xx, where xx ranges from '00' to '17'
> + =========== ==============================================
> +
> + Each PKA device supports four PKA ring devices. The PKA ring device IDs are:
> +
> + =========== ==============================================
> + BlueField-1 MLNXBF11:xx, where xx ranges from '00' to '0F'
> + BlueField-2 MLNXBF21:xx, where xx ranges from '00' to '20'
> + BlueField-3 MLNXBF52:xx, where xx ranges from '00' to '60'
> + =========== ==============================================
> +
> + For each PKA ring device, the following operation interfaces:
> +
> + - open(): open the PKA ring device specified by the device ID, and initiate the
> + related RAM regions.
> + - release(): close the PKA ring device specified by the device ID, and release the
> + related RAM regions.
> + - unlocked_ioctl(): make PKA related system calls, including getting ring device or
> + RAM region inofrmation, clearing PKA ring counter and getting random bytes from
> + the TRNG module from the PKA device.
> + - mmap(): map the PKA ring device to the virtual memory region.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f21f1dabb5fe..31821caf8a81 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15299,6 +15299,14 @@ L: linux-i2c@xxxxxxxxxxxxxxx
> S: Supported
> F: drivers/i2c/busses/i2c-mlxbf.c
>
> +MELLANOX BLUEFIELD PKA DRIVER
> +M: Ron Li <xiangrongl@xxxxxxxxxx>
> +M: Khalil Blaiech <kblaiech@xxxxxxxxxx>
> +L: platform-driver-x86@xxxxxxxxxxxxxxx
> +S: Supported
> +F: Documentation/ABI/testing/sysfs-platform-mellanox-pka
> +F: drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_*
> +
> MELLANOX ETHERNET DRIVER (mlx4_en)
> M: Tariq Toukan <tariqt@xxxxxxxxxx>
> L: netdev@xxxxxxxxxxxxxxx
> diff --git a/drivers/platform/mellanox/Kconfig b/drivers/platform/mellanox/Kconfig
> index aa760f064a17..6b99a7d866d8 100644
> --- a/drivers/platform/mellanox/Kconfig
> +++ b/drivers/platform/mellanox/Kconfig
> @@ -108,4 +108,14 @@ config NVSW_SN2201
> C3338R which is one of the Denverton product families.
> System equipped with Nvidia®Spectrum-1 32x100GbE Ethernet switch.
>
> +config MLXBF_PKA
> + tristate "Mellanox BlueField Public Key Accelerator driver"
> + depends on ARM64 && ACPI
> + help
> + If you say yes to this option, support will be included for the
> + Public Key Accelerator device on Mellanox BlueField SoCs.
> +
> + This driver can also be built as a module. If so, the module will
> + be called pka-mlxbf.
> +
> endif # MELLANOX_PLATFORM
> diff --git a/drivers/platform/mellanox/Makefile b/drivers/platform/mellanox/Makefile
> index ba56485cbe8c..1b2c61b26639 100644
> --- a/drivers/platform/mellanox/Makefile
> +++ b/drivers/platform/mellanox/Makefile
> @@ -5,6 +5,7 @@
> #
> obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
> obj-$(CONFIG_MLXBF_BOOTCTL) += mlxbf-bootctl.o
> +obj-$(CONFIG_MLXBF_PKA) += mlxbf_pka/
> obj-$(CONFIG_MLXBF_PMC) += mlxbf-pmc.o
> obj-$(CONFIG_MLXBF_TMFIFO) += mlxbf-tmfifo.o
> obj-$(CONFIG_MLXREG_HOTPLUG) += mlxreg-hotplug.o
> diff --git a/drivers/platform/mellanox/mlxbf_pka/Makefile b/drivers/platform/mellanox/mlxbf_pka/Makefile
> new file mode 100644
> index 000000000000..67465a63edb8
> --- /dev/null
> +++ b/drivers/platform/mellanox/mlxbf_pka/Makefile
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause
> +# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION. All rights reserved.
> +#
> +# Mellanox BlueField PKA Driver
> +#
> +
> +obj-$(CONFIG_MLXBF_PKA) += mlxbf-pka.o
> +
> +mlxbf-pka-objs := mlxbf_pka_drv.o
> +mlxbf-pka-objs += mlxbf_pka_dev.o
> diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
> new file mode 100644
> index 000000000000..0c953c9e48c7
> --- /dev/null
> +++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.c
> @@ -0,0 +1,1891 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause
> +// SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION. All rights reserved.
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/ioport.h>
> +#include <linux/timex.h>
> +#include <linux/types.h>
> +#include <linux/compiler.h>
> +#include <linux/io.h>
> +
> +#include "mlxbf_pka_dev.h"
> +
> +#define MASTER_CONTROLLER_OUT_OF_RESET 0
> +
> +/* Personalization string "NVIDIA-MELLANOX-BLUEFIELD-TRUE_RANDOM_NUMBER_GEN". */
> +static u32 mlxbf_pka_trng_drbg_ps_str[] = {
> + 0x4e564944, 0x49412d4d, 0x454c4c41, 0x4e4f582d,
> + 0x424c5545, 0x4649454c, 0x442d5452, 0x55455f52,
> + 0x414e444f, 0x4d5f4e55, 0x4d424552, 0x5f47454e
> +};
> +
> +/* Personalization string for DRBG test. */
> +static u32 mlxbf_pka_trng_drbg_test_ps_str[] = {
> + 0x64299d83, 0xc34d7098, 0x5bd1f51d, 0xddccfdc1,
> + 0xdd0455b7, 0x166279e5, 0x0974cb1b, 0x2f2cd100,
> + 0x59a5060a, 0xca79940d, 0xd4e29a40, 0x56b7b779
> +};
> +
> +/* First Entropy string for DRBG test. */
> +static u32 mlxbf_pka_trng_drbg_test_etpy_str1[] = {
> + 0xaa6bbcab, 0xef45e339, 0x136ca1e7, 0xbce1c881,
> + 0x9fa37b09, 0x63b53667, 0xb36e0053, 0xa202ed81,
> + 0x4650d90d, 0x8eed6127, 0x666f2402, 0x0dfd3af9
> +};
> +
> +/* Second Entropy string for DRBG test. */
> +static u32 mlxbf_pka_trng_drbg_test_etpy_str2[] = {
> + 0x35c1b7a1, 0x0154c52b, 0xd5777390, 0x226a4fdb,
> + 0x5f16080d, 0x06b68369, 0xd0c93d00, 0x3336e27f,
> + 0x1abf2c37, 0xe6ab006c, 0xa4adc6e1, 0x8e1907a2
> +};
> +
> +/* Known answer for DRBG test. */
> +static u32 mlxbf_pka_trng_drbg_test_output[] = {
> + 0xb663b9f1, 0x24943e13, 0x80f7dce5, 0xaba1a16f
> +};
> +
> +/* Known answer for poker test. */
> +static u64 poker_test_exp_cnt[] = {
> + 0x20f42bf4, 0xaf415f4, 0xf4f4fff4, 0xfff4f4f4
> +};
> +
> +struct mlxbf_pka_dev_gbl_config_t mlxbf_pka_gbl_config;
> +
> +/* Global PKA shim resource info table. */
> +static struct mlxbf_pka_dev_gbl_shim_res_info_t mlxbf_pka_gbl_res_tbl[MLXBF_PKA_MAX_NUM_IO_BLOCKS];
> +
> +/* Start a PKA device timer. */
> +static inline u64 mlxbf_pka_dev_timer_start_msec(u32 msec)
> +{
> + u64 cur_time = get_cycles();
> +
> + return (cur_time + (mlxbf_pka_early_cpu_speed() * msec) / MSEC_PER_SEC);
+ #include <linux/time.h>
Should this be rounded to some direction?
Remove all unnecessary parentheses from return statements.
> +}
> +
> +/* Test a PKA device timer for completion. */
> +static inline int mlxbf_pka_dev_timer_done(u64 timer)
> +{
> + return (get_cycles() >= timer);
> +}
> +
> +/* Return register base address. */
> +static inline u64 mlxbf_pka_dev_get_register_base(u64 base, u64 reg_addr)
> +{
> + return (base + reg_addr) & PAGE_MASK;
> +}
> +
> +/* Return register offset. */
> +static inline u64 mlxbf_pka_dev_get_register_offset(u64 base, u64 reg_addr)
> +{
> + return (base + reg_addr) & ~PAGE_MASK;
> +}
> +
> +/* Return word offset within io memory. */
> +static inline u64 mlxbf_pka_dev_get_word_offset(u64 mem_base, u64 word_addr, u64 mem_size)
> +{
> + return (mem_base + word_addr) & (mem_size - 1);
> +}
> +
> +static inline u64 mlxbf_pka_dev_io_read(void __iomem *mem_ptr, u64 mem_off)
> +{
> + return readq_relaxed(mem_ptr + mem_off);
> +}
> +
> +static inline void mlxbf_pka_dev_io_write(void __iomem *mem_ptr, u64 mem_off, u64 value)
> +{
> + writeq_relaxed(value, mem_ptr + mem_off);
> +}
> +
> +/*
> + * Mapping PKA Ring address into Window RAM address.
> + * It converts the ring address, either physical address or virtual address, to valid address into
> + * the Window RAM. This is done assuming the Window RAM base, size and mask. Here, base is the
> + * actual physical address of the Window RAM, with the help of mask it is reduced to Window RAM
> + * offset within that PKA block. Further, with the help of addr and size, we arrive at the Window
> + * RAM offset address for a PKA Ring within the given Window RAM.
All comments must be folded to 80 chars.
> + */
> +static inline u64 mlxbf_pka_ring_mem_addr(u64 base, u64 mask, u64 addr, u64 size)
> +{
> + return ((base) & (mask)) |
> + (((addr) & MLXBF_PKA_WINDOW_RAM_RING_ADDR_MASK) |
> + ((((addr) & ~((size) - 1)) & MLXBF_PKA_WINDOW_RAM_RING_SIZE_MASK) >>
> + MLXBF_PKA_WINDOW_RAM_RING_SIZE_SHIFT));
Please use FIELD_GET() and get rid of _SHIFT defines if they're only used
for constructs like this.
> +}
> +
> +/* Add the resource to the global resource table. */
> +static int mlxbf_pka_dev_add_resource(struct mlxbf_pka_dev_res_t *res_ptr, u32 shim_idx)
> +{
> + u8 res_cnt;
> +
> + res_cnt = mlxbf_pka_gbl_res_tbl[shim_idx].res_cnt;
> + if (res_cnt >= MLXBF_PKA_DEV_SHIM_RES_CNT)
> + return -ENOMEM;
> +
> + mlxbf_pka_gbl_res_tbl[shim_idx].res_tbl[res_cnt] = res_ptr;
> + mlxbf_pka_gbl_res_tbl[shim_idx].res_cnt++;
> +
> + return 0;
> +}
> +
> +/* Remove the resource from the global resource table. */
> +static int mlxbf_pka_dev_put_resource(struct mlxbf_pka_dev_res_t *res, u32 shim_idx)
> +{
> + struct mlxbf_pka_dev_res_t *res_ptr;
> + u8 res_idx;
> +
> + for (res_idx = 0; res_idx < MLXBF_PKA_DEV_SHIM_RES_CNT; res_idx++) {
> + res_ptr = mlxbf_pka_gbl_res_tbl[shim_idx].res_tbl[res_idx];
> + if (res_ptr && strcmp(res_ptr->name, res->name) == 0) {
!strcmp(), although I'd reverse the entire logic and use continue instead.
Add #include for strcmp().
> + mlxbf_pka_gbl_res_tbl[shim_idx].res_tbl[res_idx] = NULL;
> + mlxbf_pka_gbl_res_tbl[shim_idx].res_cnt--;
> + break;
> + }
> + }
> +
> + /*
> + * Check whether the resource shares the same memory map; If so, the memory map shouldn't
> + * be released.
> + */
> + for (res_idx = 0; res_idx < MLXBF_PKA_DEV_SHIM_RES_CNT; res_idx++) {
> + res_ptr = mlxbf_pka_gbl_res_tbl[shim_idx].res_tbl[res_idx];
> + if (res_ptr && res_ptr->base == res->base)
> + return -EBUSY;
> + }
> +
> + return 0;
> +}
> +
> +static void __iomem *mlxbf_pka_dev_get_resource_ioaddr(u64 res_base, u32 shim_idx)
> +{
> + struct mlxbf_pka_dev_res_t *res_ptr;
> + u8 res_cnt, res_idx;
> +
> + res_cnt = mlxbf_pka_gbl_res_tbl[shim_idx].res_cnt;
> + if (!res_cnt)
> + return NULL;
> +
> + for (res_idx = 0; res_idx < res_cnt; res_idx++) {
> + res_ptr = mlxbf_pka_gbl_res_tbl[shim_idx].res_tbl[res_idx];
> + if (res_ptr->base == res_base)
> + return res_ptr->ioaddr;
> + }
> +
> + return NULL;
> +}
> +
> +/* Set PKA device resource config and map io memory if needed. */
> +static int mlxbf_pka_dev_set_resource_config(struct device *dev,
> + struct mlxbf_pka_dev_shim_s *shim,
> + struct mlxbf_pka_dev_res_t *res_ptr,
> + u64 res_base,
> + u64 res_size,
> + u64 res_type,
> + char *res_name)
> +{
> + if (res_ptr->status == MLXBF_PKA_DEV_RES_STATUS_MAPPED)
> + return -EPERM;
> +
> + switch (res_type) {
> + case MLXBF_PKA_DEV_RES_TYPE_REG:
> + res_ptr->base = res_base;
> + break;
> + case MLXBF_PKA_DEV_RES_TYPE_MEM:
> + res_ptr->base = shim->mem_res.eip154_base + res_base;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + res_ptr->size = res_size;
> + res_ptr->type = res_type;
> + res_ptr->name = res_name;
> + res_ptr->status = MLXBF_PKA_DEV_RES_STATUS_UNMAPPED;
> + res_ptr->ioaddr = mlxbf_pka_dev_get_resource_ioaddr(res_ptr->base, shim->shim_id);
> + if (!res_ptr->ioaddr) {
> + if (!devm_request_mem_region(dev, res_ptr->base, res_ptr->size, res_ptr->name)) {
> + dev_err(dev, "failed to get io memory region\n");
> + return -EPERM;
> + }
> +
> + res_ptr->ioaddr = devm_ioremap(dev, res_ptr->base, res_ptr->size);
> + if (!res_ptr->ioaddr) {
> + dev_err(dev, "unable to map io memory into CPU space\n");
> + return -ENOMEM;
> + }
> + }
> +
> + res_ptr->status = MLXBF_PKA_DEV_RES_STATUS_MAPPED;
> +
> + if (!res_ptr->ioaddr || mlxbf_pka_dev_add_resource(res_ptr, shim->shim_id)) {
> + dev_err(dev, "unable to map io memory\n");
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +/* Unset PKA device resource config - unmap io memory if needed. */
> +static void mlxbf_pka_dev_unset_resource_config(struct mlxbf_pka_dev_shim_s *shim,
> + struct mlxbf_pka_dev_res_t *res_ptr)
> +{
> + if (res_ptr->status != MLXBF_PKA_DEV_RES_STATUS_MAPPED)
> + return;
> +
> + if (res_ptr->ioaddr && -EBUSY != mlxbf_pka_dev_put_resource(res_ptr, shim->shim_id)) {
Reverse logic and return.
I suggest you split this into two as well.
> + pr_debug("mlxbf_pka: PKA device resource released\n");
> + res_ptr->status = MLXBF_PKA_DEV_RES_STATUS_UNMAPPED;
> + }
> +}
> +
> +int mlxbf_pka_dev_clear_ring_counters(struct mlxbf_pka_dev_ring_t *ring)
> +{
> + struct mlxbf_pka_dev_res_t *master_seq_ctrl_ptr;
> + u64 master_reg_base, master_reg_off;
> + struct mlxbf_pka_dev_shim_s *shim;
> + void __iomem *master_reg_ptr;
> +
> + shim = ring->shim;
> + master_seq_ctrl_ptr = &shim->resources.master_seq_ctrl;
> + master_reg_base = master_seq_ctrl_ptr->base;
> + master_reg_ptr = master_seq_ctrl_ptr->ioaddr;
> + master_reg_off = mlxbf_pka_dev_get_register_offset(master_reg_base,
> + MLXBF_PKA_MASTER_SEQ_CTRL_ADDR);
> +
> + /* Push the EIP-154 master controller into reset. */
> + mlxbf_pka_dev_io_write(master_reg_ptr, master_reg_off, MLXBF_PKA_MASTER_SEQ_CTRL_RESET_VAL);
> +
> + /* Clear counters. */
> + mlxbf_pka_dev_io_write(master_reg_ptr, master_reg_off,
> + MLXBF_PKA_MASTER_SEQ_CTRL_CLEAR_COUNTERS_VAL);
> +
> + /* Take the EIP-154 master controller out of reset. */
> + mlxbf_pka_dev_io_write(master_reg_ptr, master_reg_off, MASTER_CONTROLLER_OUT_OF_RESET);
> +
> + return 0;
> +}
> +
> +/*
> + * Initialize ring. Set ring parameters and configure ring resources. It returns 0 on success, a
> + * negative error code on failure.
> + */
> +static int mlxbf_pka_dev_init_ring(struct device *dev,
> + struct mlxbf_pka_dev_ring_t *ring,
> + u32 ring_id,
> + struct mlxbf_pka_dev_shim_s *shim)
> +{
> + struct mlxbf_pka_dev_res_t *ring_window_ram_ptr;
> + struct mlxbf_pka_dev_res_t *ring_info_words_ptr;
> + struct mlxbf_pka_dev_res_t *ring_counters_ptr;
> + u8 window_ram_split;
> + u32 ring_words_off;
> + u32 ring_cntrs_off;
> + u32 ring_mem_base;
> + u32 ring_mem_off;
> + u32 shim_ring_id;
> +
> + if (ring->status != MLXBF_PKA_DEV_RING_STATUS_UNDEFINED) {
> + dev_err(dev, "PKA ring must be undefined\n");
> + return -EPERM;
> + }
> +
> + if (ring_id > MLXBF_PKA_MAX_NUM_RINGS - 1) {
> + dev_err(dev, "invalid ring identifier\n");
> + return -EINVAL;
> + }
> +
> + ring->ring_id = ring_id;
> + ring->shim = shim;
> + ring->resources_num = MLXBF_PKA_MAX_NUM_RING_RESOURCES;
> + shim_ring_id = ring_id % MLXBF_PKA_MAX_NUM_IO_BLOCK_RINGS;
> + shim->rings[shim_ring_id] = ring;
> +
> + /* Configure ring information control/status words resource. */
> + ring_info_words_ptr = &ring->resources.info_words;
> + ring_words_off = shim_ring_id * MLXBF_PKA_RING_WORDS_SPACING;
> + ring_info_words_ptr->base = ring_words_off + shim->mem_res.eip154_base +
> + MLXBF_PKA_RING_WORDS_ADDR;
> + ring_info_words_ptr->size = MLXBF_PKA_RING_WORDS_SIZE;
> + ring_info_words_ptr->type = MLXBF_PKA_DEV_RES_TYPE_MEM;
> + ring_info_words_ptr->status = MLXBF_PKA_DEV_RES_STATUS_UNMAPPED;
> + ring_info_words_ptr->name = "MLXBF_PKA_RING_INFO";
> +
> + /* Configure ring counters registers resource. */
> + ring_counters_ptr = &ring->resources.counters;
> + ring_cntrs_off = shim_ring_id * MLXBF_PKA_RING_CNTRS_SPACING;
> + ring_counters_ptr->base = ring_cntrs_off + shim->mem_res.eip154_base +
> + MLXBF_PKA_RING_CNTRS_ADDR;
> + ring_counters_ptr->size = MLXBF_PKA_RING_CNTRS_SIZE;
> + ring_counters_ptr->type = MLXBF_PKA_DEV_RES_TYPE_REG;
> + ring_counters_ptr->status = MLXBF_PKA_DEV_RES_STATUS_UNMAPPED;
> + ring_counters_ptr->name = "MLXBF_PKA_RING_CNTRS";
> +
> + /* Configure ring window RAM resource. */
> + window_ram_split = shim->window_ram_split;
> + if (window_ram_split == MLXBF_PKA_SHIM_WINDOW_RAM_SPLIT_ENABLED) {
> + ring_mem_off = shim_ring_id * MLXBF_PKA_RING_MEM_1_SPACING;
> + ring_mem_base = ring_mem_off + shim->mem_res.alt_wndw_ram_0_base;
> + } else {
> + ring_mem_off = shim_ring_id * MLXBF_PKA_RING_MEM_0_SPACING;
> + ring_mem_base = ring_mem_off + shim->mem_res.wndw_ram_base;
> + }
> +
> + ring_window_ram_ptr = &ring->resources.window_ram;
> + ring_window_ram_ptr->base = ring_mem_base;
> + ring_window_ram_ptr->size = MLXBF_PKA_RING_MEM_SIZE;
> + ring_window_ram_ptr->type = MLXBF_PKA_DEV_RES_TYPE_MEM;
> + ring_window_ram_ptr->status = MLXBF_PKA_DEV_RES_STATUS_UNMAPPED;
> + ring_window_ram_ptr->name = "MLXBF_PKA_RING_WINDOW";
> +
> + ring->ring_info = devm_kzalloc(dev, sizeof(*ring->ring_info), GFP_KERNEL);
> + if (!ring->ring_info)
> + return -ENOMEM;
> +
> + mutex_init(&ring->mutex);
devm_mutex_init() + don't forget error handling.
> + ring->status = MLXBF_PKA_DEV_RING_STATUS_INITIALIZED;
> +
> + return 0;
> +}
> +
> +/* Release a given Ring. */
> +static int mlxbf_pka_dev_release_ring(struct mlxbf_pka_dev_ring_t *ring)
> +{
> + struct mlxbf_pka_dev_shim_s *shim;
> + u32 shim_ring_id;
> +
> + if (ring->status == MLXBF_PKA_DEV_RING_STATUS_UNDEFINED)
> + return 0;
> +
> + if (ring->status == MLXBF_PKA_DEV_RING_STATUS_BUSY) {
> + pr_err("mlxbf_pka error: PKA ring is busy\n");
> + return -EBUSY;
> + }
> +
> + shim = ring->shim;
> +
> + if (shim->status == MLXBF_PKA_SHIM_STATUS_RUNNING) {
> + pr_err("mlxbf_pka error: PKA shim is running\n");
dev_err()
> + return -EPERM;
> + }
> +
> + mlxbf_pka_dev_unset_resource_config(shim, &ring->resources.info_words);
> + mlxbf_pka_dev_unset_resource_config(shim, &ring->resources.counters);
> + mlxbf_pka_dev_unset_resource_config(shim, &ring->resources.window_ram);
> +
> + ring->status = MLXBF_PKA_DEV_RING_STATUS_UNDEFINED;
> + shim_ring_id = ring->ring_id % MLXBF_PKA_MAX_NUM_IO_BLOCK_RINGS;
> + shim->rings[shim_ring_id] = NULL;
> + shim->rings_num--;
> +
> + return 0;
> +}
> +
> +/*
> + * Partition the window RAM for a given PKA ring. Here we statically divide the 16K memory region
> + * into three partitions: First partition is reserved for command descriptor ring (1K), second
> + * partition is reserved for result descriptor ring (1K), and the remaining 14K are reserved for
> + * vector data. Through this memory partition scheme, command/result descriptor rings hold a total
> + * of 1KB/64B = 16 descriptors each. The addresses for the rings start at offset 0x3800. Also note
> + * that it is possible to have rings full while the vector data can support more data, the opposite
> + * can also happen, but it is not suitable. For instance ECC point multiplication requires 8 input
> + * vectors and 2 output vectors, a total of 10 vectors. If each vector has a length of 24 words
> + * (24x4B = 96B), we can process 14KB/960B = 14 operations which is close to 16 the total
> + * descriptors supported by rings. On the other hand, using 12K vector data region, allows to
> + * process only 12 operations, while rings can hold 32 descriptors (ring usage is significantly
> + * low).
> + *
> + * For ECDSA verify, we have 12 vectors which require 1152B, with 14KB we can handle 12 operations,
> + * against 10 operations with 12KB vector data memory. We believe that the aforementioned memory
> + * partition help us to leverage the trade-off between supported descriptors and required vectors.
> + * Note that these examples give approximative values and does not include buffer word padding
> + * across vectors.
> + *
> + * The function also writes the result descriptor rings base addresses, size and type. And
> + * initialize the read and write pointers and statistics. It returns 0 on success, a negative error
> + * code on failure.
> + *
> + * This function must be called once per ring, at initialization before any other functions are
> + * called.
> + */
> +static int mlxbf_pka_dev_partition_mem(struct mlxbf_pka_dev_ring_t *ring)
> +{
> + u64 rslt_desc_ring_base;
> + u64 cmd_desc_ring_base;
> + u32 cmd_desc_ring_size;
> + u64 window_ram_base;
> + u64 window_ram_size;
> + u32 ring_mem_base;
> +
> + if (!ring->shim || ring->status != MLXBF_PKA_DEV_RING_STATUS_INITIALIZED)
> + return -EPERM;
> +
> + window_ram_base = ring->resources.window_ram.base;
> + window_ram_size = ring->resources.window_ram.size;
> + /*
> + * Partition ring memory. Give ring pair (cmmd descriptor ring and rslt descriptor ring) an
> + * equal portion of the memory. The cmmd descriptor ring and result descriptor ring are
> + * used as "non-overlapping" ring. Currently set aside 1/8 of the window RAM for command and
> + * result descriptor rings - giving a total of 1K/64B = 16 descriptors per ring. The
> + * remaining memory is "Data Memory" - i.e. memory to hold the command operands and results
> + * - also called input/output vectors (in all cases these vectors are just single large
> + * integers - often in the range of hundreds to thousands of bits long).
> + */
> + ring_mem_base = window_ram_base + MLXBF_PKA_WINDOW_RAM_DATA_MEM_SIZE;
> + cmd_desc_ring_size = MLXBF_PKA_WINDOW_RAM_RING_MEM_SIZE /
> + MLXBF_PKA_WINDOW_RAM_RING_MEM_DIV;
> + ring->num_cmd_desc = MLXBF_PKA_WINDOW_RAM_RING_MEM_SIZE /
> + MLXBF_PKA_WINDOW_RAM_RING_MEM_DIV / CMD_DESC_SIZE;
> + /*
> + * The command and result descriptor rings may be placed at different (non-overlapping)
> + * locations in Window RAM memory space. PKI command interface: Most of the functionality is
> + * defined by the EIP-154 master firmware on the EIP-154 master controller Sequencer.
> + */
> + cmd_desc_ring_base = ring_mem_base;
> + rslt_desc_ring_base = ring_mem_base + cmd_desc_ring_size;
> +
> + cmd_desc_ring_base = mlxbf_pka_ring_mem_addr(window_ram_base,
> + ring->shim->mem_res.wndw_ram_off_mask,
> + cmd_desc_ring_base,
> + window_ram_size);
> + rslt_desc_ring_base = mlxbf_pka_ring_mem_addr(window_ram_base,
> + ring->shim->mem_res.wndw_ram_off_mask,
> + rslt_desc_ring_base,
> + window_ram_size);
> +
> + /* Fill ring information. */
> + memset(ring->ring_info, 0, sizeof(struct mlxbf_pka_dev_hw_ring_info_t));
Please use the destination's sizeof() in the size parameter.
> + ring->ring_info->cmmd_base = cmd_desc_ring_base;
Is cmmd a typo?
> + ring->ring_info->rslt_base = rslt_desc_ring_base;
> + ring->ring_info->size = MLXBF_PKA_WINDOW_RAM_RING_MEM_SIZE /
> + MLXBF_PKA_WINDOW_RAM_RING_MEM_DIV / CMD_DESC_SIZE - 1;
> + ring->ring_info->host_desc_size = CMD_DESC_SIZE / sizeof(u32);
> + ring->ring_info->in_order = ring->shim->ring_type;
> +
> + return 0;
> +}
> +
> +/*
> + * Write the ring base address, ring size and type, and initialize (clear) the read and write
> + * pointers and statistics.
> + */
> +static int mlxbf_pka_dev_write_ring_info(struct mlxbf_pka_dev_res_t *buffer_ram_ptr,
> + u8 ring_id,
> + u32 ring_cmmd_base_val,
> + u32 ring_rslt_base_val,
> + u32 ring_size_type_val)
> +{
> + u32 ring_spacing;
> + u64 word_off;
> +
> + if (buffer_ram_ptr->status != MLXBF_PKA_DEV_RES_STATUS_MAPPED ||
> + buffer_ram_ptr->type != MLXBF_PKA_DEV_RES_TYPE_MEM)
> + return -EPERM;
> +
> + pr_debug("mlxbf_pka: writing ring information control/status words\n");
dev_dbg()
> +
> + ring_spacing = ring_id * MLXBF_PKA_RING_WORDS_SPACING;
> + /*
> + * Write the command ring base address that the EIP-154 master firmware uses with the
> + * command ring read pointer to get command descriptors from the Host ring. After the
> + * initialization, although the word is writeable it should be regarded as read-only.
> + */
> + word_off = mlxbf_pka_dev_get_word_offset(buffer_ram_ptr->base,
> + MLXBF_PKA_RING_CMMD_BASE_0_ADDR + ring_spacing,
> + MLXBF_PKA_BUFFER_RAM_SIZE);
> + mlxbf_pka_dev_io_write(buffer_ram_ptr->ioaddr, word_off, ring_cmmd_base_val);
> +
> + /*
> + * Write the result ring base address that the EIP-154 master firmware uses with the result
> + * ring write pointer to put the result descriptors in the Host ring. After the
> + * initialization, although the word is writeable it should be regarded as read-only.
> + */
> + word_off = mlxbf_pka_dev_get_word_offset(buffer_ram_ptr->base,
> + MLXBF_PKA_RING_RSLT_BASE_0_ADDR + ring_spacing,
> + MLXBF_PKA_BUFFER_RAM_SIZE);
> + mlxbf_pka_dev_io_write(buffer_ram_ptr->ioaddr, word_off, ring_rslt_base_val);
> +
> + /*
> + * Write the ring size (number of descriptors), the size of the descriptor and the result
> + * reporting scheme. After the initialization, although the word is writeable it should be
> + * regarded as read-only.
> + */
> + word_off = mlxbf_pka_dev_get_word_offset(buffer_ram_ptr->base,
> + MLXBF_PKA_RING_SIZE_TYPE_0_ADDR + ring_spacing,
> + MLXBF_PKA_BUFFER_RAM_SIZE);
> + mlxbf_pka_dev_io_write(buffer_ram_ptr->ioaddr, word_off, ring_size_type_val);
> +
> + /*
> + * Write the command and result ring indices that the EIP-154 master firmware uses. This
> + * word should be written with zero when the ring information is initialized. After the
> + * initialization, although the word is writeable it should be regarded as read-only.
> + */
> + word_off = mlxbf_pka_dev_get_word_offset(buffer_ram_ptr->base,
> + MLXBF_PKA_RING_RW_PTRS_0_ADDR + ring_spacing,
> + MLXBF_PKA_BUFFER_RAM_SIZE);
> + mlxbf_pka_dev_io_write(buffer_ram_ptr->ioaddr, word_off, 0);
> +
> + /*
> + * Write the ring statistics (two 16-bit counters, one for commands and one for results)
> + * from EIP-154 master firmware point of view. This word should be written with zero when
> + * the ring information is initialized. After the initialization, although the word is
> + * writeable it should be regarded as read-only.
> + */
This function has overly long and in part repetitively worded comments.
And you should not comment what is readable from the code itself.
> + word_off = mlxbf_pka_dev_get_word_offset(buffer_ram_ptr->base,
> + MLXBF_PKA_RING_RW_STAT_0_ADDR + ring_spacing,
> + MLXBF_PKA_BUFFER_RAM_SIZE);
> + mlxbf_pka_dev_io_write(buffer_ram_ptr->ioaddr, word_off, 0);
> +
> + return 0;
> +}
> +
> +/*
> + * Set up the control/status words. Upon a PKI command the EIP-154 master firmware will read and
> + * partially update the ring information.
> + */
> +static int mlxbf_pka_dev_set_ring_info(struct mlxbf_pka_dev_ring_t *ring)
> +{
> + u32 ring_cmmd_base_val;
> + u32 ring_rslt_base_val;
> + u32 ring_size_type_val;
> + int ret;
> +
> + /* Ring info configuration MUST be done when the PKA ring is initialized. */
> + if ((ring->shim->status != MLXBF_PKA_SHIM_STATUS_INITIALIZED &&
> + ring->shim->status != MLXBF_PKA_SHIM_STATUS_RUNNING &&
> + ring->shim->status != MLXBF_PKA_SHIM_STATUS_STOPPED) ||
> + ring->status != MLXBF_PKA_DEV_RING_STATUS_INITIALIZED)
> + return -EPERM;
> +
> + /* Partition ring memory. */
> + ret = mlxbf_pka_dev_partition_mem(ring);
> + if (ret) {
> + pr_err("mlxbf_pka error: failed to initialize ring memory\n");
dev_err(), please go through all printing.
> + return ret;
> + }
> +
> + /* Fill ring information. */
> + ring_cmmd_base_val = ring->ring_info->cmmd_base;
> + ring_rslt_base_val = ring->ring_info->rslt_base;
> + ring_size_type_val = (ring->ring_info->in_order &
> + MLXBF_PKA_RING_INFO_IN_ORDER_MASK) <<
> + MLXBF_PKA_RING_INFO_IN_ORDER_OFFSET;
Looks FIELD_PREP().
> + ring_size_type_val |= (ring->ring_info->host_desc_size &
> + MLXBF_PKA_RING_INFO_HOST_DESC_SIZE_MASK) <<
> + MLXBF_PKA_RING_INFO_HOST_DESC_SIZE_OFFSET;
Ditto.
> + ring_size_type_val |= (ring->num_cmd_desc - 1) & MLXBF_PKA_RING_NUM_CMD_DESC_MASK;
> +
> + /* Write ring information status/control words in the PKA Buffer RAM. */
> + ret = mlxbf_pka_dev_write_ring_info(&ring->shim->resources.buffer_ram,
> + ring->ring_id % MLXBF_PKA_MAX_NUM_IO_BLOCK_RINGS,
> + ring_cmmd_base_val,
> + ring_rslt_base_val,
> + ring_size_type_val);
> + if (ret) {
> + pr_err("mlxbf_pka error: failed to write ring information\n");
> + return ret;
> + }
> +
> + ring->status = MLXBF_PKA_DEV_RING_STATUS_READY;
> +
> + return ret;
> +}
> +
> +/*
> + * Create shim. Set shim parameters and configure shim resources. It returns 0 on success, a
> + * negative error code on failure.
Even if not explicitly marking things for kerneldoc (with /**), please use
the format compatible with it, so:
/*
* mlxbf_pka_dev_create_shim - Create shim.
*
* Set shim parameters and configure shim resources.
*
* Return: ...
*/
> + */
> +static int mlxbf_pka_dev_create_shim(struct device *dev,
> + struct mlxbf_pka_dev_shim_s *shim,
> + u32 shim_id,
> + u8 split,
> + struct mlxbf_pka_dev_mem_res *mem_res)
> +{
> + u64 reg_base;
> + u64 reg_size;
> + int ret;
> +
> + if (shim->status == MLXBF_PKA_SHIM_STATUS_CREATED)
> + return 0;
> +
> + if (shim->status != MLXBF_PKA_SHIM_STATUS_UNDEFINED) {
> + dev_err(dev, "PKA device must be undefined\n");
> + return -EPERM;
> + }
> +
> + if (shim_id > MLXBF_PKA_MAX_NUM_IO_BLOCKS - 1) {
> + dev_err(dev, "invalid shim identifier\n");
> + return -EINVAL;
> + }
> +
> + shim->shim_id = shim_id;
> + shim->mem_res = *mem_res;
> +
> + if (split)
> + shim->window_ram_split = MLXBF_PKA_SHIM_WINDOW_RAM_SPLIT_ENABLED;
> + else
> + shim->window_ram_split = MLXBF_PKA_SHIM_WINDOW_RAM_SPLIT_DISABLED;
> +
> + shim->ring_type = MLXBF_PKA_RING_TYPE_IN_ORDER;
> + shim->ring_priority = MLXBF_PKA_RING_OPTIONS_PRIORITY;
> + shim->rings_num = MLXBF_PKA_MAX_NUM_IO_BLOCK_RINGS;
> + shim->rings = devm_kzalloc(dev,
> + shim->rings_num * sizeof(struct mlxbf_pka_dev_ring_t),
devm_kcalloc() ?
> + GFP_KERNEL);
> + if (!shim->rings) {
> + dev_err(dev, "unable to allocate memory for ring\n");
> + return -ENOMEM;
> + }
> +
> + /* Set PKA device Buffer RAM config. */
> + ret = mlxbf_pka_dev_set_resource_config(dev,
> + shim,
> + &shim->resources.buffer_ram,
> + MLXBF_PKA_BUFFER_RAM_BASE,
> + MLXBF_PKA_BUFFER_RAM_SIZE,
> + MLXBF_PKA_DEV_RES_TYPE_MEM,
> + "MLXBF_PKA_BUFFER_RAM");
> + if (ret) {
> + dev_err(dev, "unable to set Buffer RAM config\n");
> + return ret;
> + }
> +
> + /* Set PKA device Master Controller register. */
> + reg_size = PAGE_SIZE;
> + reg_base = mlxbf_pka_dev_get_register_base(shim->mem_res.eip154_base,
> + MLXBF_PKA_MASTER_SEQ_CTRL_ADDR);
> + ret = mlxbf_pka_dev_set_resource_config(dev,
> + shim,
> + &shim->resources.master_seq_ctrl,
> + reg_base,
> + reg_size,
> + MLXBF_PKA_DEV_RES_TYPE_REG,
> + "MLXBF_PKA_MASTER_SEQ_CTRL");
> + if (ret) {
> + dev_err(dev, "unable to set Master Controller register config\n");
> + return ret;
> + }
> +
> + /* Set PKA device AIC registers. */
> + reg_size = PAGE_SIZE;
> + reg_base = mlxbf_pka_dev_get_register_base(shim->mem_res.eip154_base,
> + MLXBF_PKA_AIC_POL_CTRL_ADDR);
> + ret = mlxbf_pka_dev_set_resource_config(dev,
> + shim,
> + &shim->resources.aic_csr,
> + reg_base,
> + reg_size,
> + MLXBF_PKA_DEV_RES_TYPE_REG,
> + "MLXBF_PKA_AIC_CSR");
> + if (ret) {
> + dev_err(dev, "unable to set AIC registers config\n");
> + return ret;
> + }
> +
> + /* Set PKA device TRNG registers. */
> + reg_size = PAGE_SIZE;
> + reg_base = mlxbf_pka_dev_get_register_base(shim->mem_res.eip154_base,
> + MLXBF_PKA_TRNG_OUTPUT_0_ADDR);
> + ret = mlxbf_pka_dev_set_resource_config(dev,
> + shim,
> + &shim->resources.trng_csr,
> + reg_base,
> + reg_size,
> + MLXBF_PKA_DEV_RES_TYPE_REG,
> + "MLXBF_PKA_TRNG_CSR");
> + if (ret) {
> + dev_err(dev, "unable to setup the TRNG\n");
> + return ret;
> + }
> +
> + shim->status = MLXBF_PKA_SHIM_STATUS_CREATED;
> +
> + return ret;
> +}
> +
> +/* Delete shim and unset shim resources. */
> +static int mlxbf_pka_dev_delete_shim(struct mlxbf_pka_dev_shim_s *shim)
> +{
> + struct mlxbf_pka_dev_res_t *res_master_seq_ctrl, *res_aic_csr, *res_trng_csr;
> + struct mlxbf_pka_dev_res_t *res_buffer_ram;
> +
> + pr_debug("mlxbf_pka: PKA device delete shim\n");
> +
> + if (shim->status == MLXBF_PKA_SHIM_STATUS_UNDEFINED)
> + return 0;
> +
> + if (shim->status != MLXBF_PKA_SHIM_STATUS_FINALIZED &&
> + shim->status != MLXBF_PKA_SHIM_STATUS_CREATED) {
> + pr_err("mlxbf_pka error: PKA device status must be finalized\n");
> + return -EPERM;
> + }
> +
> + res_buffer_ram = &shim->resources.buffer_ram;
> + res_master_seq_ctrl = &shim->resources.master_seq_ctrl;
> + res_aic_csr = &shim->resources.aic_csr;
> + res_trng_csr = &shim->resources.trng_csr;
> +
> + mlxbf_pka_dev_unset_resource_config(shim, res_buffer_ram);
> + mlxbf_pka_dev_unset_resource_config(shim, res_master_seq_ctrl);
> + mlxbf_pka_dev_unset_resource_config(shim, res_aic_csr);
> + mlxbf_pka_dev_unset_resource_config(shim, res_trng_csr);
> +
> + shim->status = MLXBF_PKA_SHIM_STATUS_UNDEFINED;
> +
> + return 0;
> +}
> +
> +/* Configure ring options. */
> +static int mlxbf_pka_dev_config_ring_options(struct mlxbf_pka_dev_res_t *buffer_ram_ptr,
> + u32 rings_num,
> + u8 ring_priority)
> +{
> + u64 control_word;
> + u64 word_off;
> +
> + if (buffer_ram_ptr->status != MLXBF_PKA_DEV_RES_STATUS_MAPPED ||
> + buffer_ram_ptr->type != MLXBF_PKA_DEV_RES_TYPE_MEM)
> + return -EPERM;
> +
> + if (rings_num > MLXBF_PKA_MAX_NUM_RINGS || rings_num < 1) {
> + pr_err("mlxbf_pka error: invalid rings number\n");
> + return -EINVAL;
> + }
> +
> + pr_debug("mlxbf_pka: configure PKA ring options control word\n");
> +
> + /*
> + * Write MLXBF_PKA_RING_OPTIONS control word located in the MLXBF_PKA_BUFFER_RAM. The value
> + * of this word is determined by the PKA I/O block (Shim). Set the number of implemented
> + * command/result ring pairs that is available in this EIP-154, encoded as binary value,
> + * which is 4.
> + */
> + control_word = (u64)0x0;
Unnecessary cast.
> + control_word |= ring_priority & MLXBF_PKA_RING_OPTIONS_RING_PRIORITY_MASK;
FIELD_PREP()
> + control_word |= ((rings_num - 1) << MLXBF_PKA_RING_OPTIONS_RING_NUM_OFFSET) &
> + MLXBF_PKA_RING_OPTIONS_RING_NUM_MASK;
FIELD_PREP()
> + control_word |= (MLXBF_PKA_RING_OPTIONS_SIGNATURE_BYTE <<
> + MLXBF_PKA_RING_OPTIONS_SIGNATURE_BYTE_OFFSET) &
> + MLXBF_PKA_RING_OPTIONS_SIGNATURE_BYTE_MASK;
FIELD_PREP()
You can alternatively chain these with |.
> + word_off = mlxbf_pka_dev_get_word_offset(buffer_ram_ptr->base,
> + MLXBF_PKA_RING_OPTIONS_ADDR,
> + MLXBF_PKA_BUFFER_RAM_SIZE);
> + mlxbf_pka_dev_io_write(buffer_ram_ptr->ioaddr, word_off, control_word);
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_dev_config_trng_clk(struct mlxbf_pka_dev_res_t *aic_csr_ptr)
> +{
> + u32 trng_clk_en = 0;
> + void __iomem *csr_reg_ptr;
> + u64 csr_reg_base;
> + u64 csr_reg_off;
> + u64 timer;
> +
> + if (aic_csr_ptr->status != MLXBF_PKA_DEV_RES_STATUS_MAPPED ||
> + aic_csr_ptr->type != MLXBF_PKA_DEV_RES_TYPE_REG)
> + return -EPERM;
> +
> + pr_debug("mlxbf_pka: turn on TRNG clock\n");
dev_dbg()
> +
> + csr_reg_base = aic_csr_ptr->base;
> + csr_reg_ptr = aic_csr_ptr->ioaddr;
> +
> + /*
> + * Enable the TRNG clock in MLXBF_PKA_CLK_FORCE. In general, this register should be left in
> + * its default state of all zeroes. Only when the TRNG is directly controlled via the Host
> + * slave interface, the engine needs to be turned on using the 'trng_clk_on' bit in this
> + * register. In case the TRNG is controlled via internal firmware, this is not required.
> + */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_CLK_FORCE_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, MLXBF_PKA_CLK_FORCE_TRNG_ON);
> + /*
> + * Check whether the system clock for TRNG engine is enabled. The clock MUST be running to
> + * provide access to the TRNG.
> + */
> + timer = mlxbf_pka_dev_timer_start_msec(100);
> + while (!trng_clk_en) {
> + trng_clk_en |= mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off)
> + & MLXBF_PKA_CLK_FORCE_TRNG_ON;
> + if (mlxbf_pka_dev_timer_done(timer)) {
> + pr_debug("mlxbf_pka: failed to enable TRNG clock\n");
> + return -EAGAIN;
> + }
> + }
> + pr_debug("mlxbf_pka: trng_clk_on is enabled\n");
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_dev_trng_wait_test_ready(void __iomem *csr_reg_ptr, u64 csr_reg_base)
> +{
> + u64 csr_reg_off, timer, csr_reg_val, test_ready = 0;
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_STATUS_ADDR);
> + timer = mlxbf_pka_dev_timer_start_msec(1000);
MSEC_PER_SEC ?
> +
> + while (!test_ready) {
> + csr_reg_val = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + test_ready = csr_reg_val & MLXBF_PKA_TRNG_STATUS_TEST_READY;
> +
> + if (mlxbf_pka_dev_timer_done(timer)) {
> + pr_debug("mlxbf_pka: TRNG test ready timer done, 0x%llx\n", csr_reg_val);
> + return 1;
> + }
> + }
> +
> + return 0;
Should this function return bool ?
> +}
> +
> +static int mlxbf_pka_dev_trng_enable_test(void __iomem *csr_reg_ptr, u64 csr_reg_base, u32 test)
> +{
> + u64 csr_reg_val, csr_reg_off;
> +
> + /*
> + * Set the 'test_mode' bit in the TRNG_CONTROL register and the 'test_known_noise' bit in
> + * the TRNG_TEST register – this will immediately set the 'test_ready' bit (in the
> + * TRNG_STATUS register) to indicate that data can be written. It will also reset the
> + * 'monobit test', 'run test' and 'poker test' circuits to their initial states. Note that
> + * the TRNG need not be enabled for this test.
> + */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONTROL_ADDR);
> + csr_reg_val = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONTROL_ADDR);
> +
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off,
> + csr_reg_val | MLXBF_PKA_TRNG_CONTROL_TEST_MODE);
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_TEST_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, test);
> + /* Wait until the 'test_ready' bit is set. */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_STATUS_ADDR);
> + do {
> + csr_reg_val = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + } while (!(csr_reg_val & MLXBF_PKA_TRNG_STATUS_TEST_READY));
> +
> + /* Check whether the 'monobit test', 'run test' and 'poker test' are reset. */
> + if (csr_reg_val &
> + (MLXBF_PKA_TRNG_STATUS_MONOBIT_FAIL |
> + MLXBF_PKA_TRNG_STATUS_RUN_FAIL |
> + MLXBF_PKA_TRNG_STATUS_POKER_FAIL)) {
Align.
> + pr_err("mlxbf_pka error: test bits aren't reset, TRNG_STATUS:0x%llx\n",
> + csr_reg_val);
> + return -EAGAIN;
> + }
> +
> + /*
> + * Set 'stall_run_poker' bit to allow inspecting the state of the result counters which
> + * would otherwise be reset immediately for the next 20,000 bits block to test.
> + */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_ALARMCNT_ADDR);
> + csr_reg_val = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + mlxbf_pka_dev_io_write(csr_reg_ptr,
> + csr_reg_off,
> + csr_reg_val | MLXBF_PKA_TRNG_ALARMCNT_STALL_RUN_POKER);
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_dev_trng_test_circuits(void __iomem *csr_reg_ptr,
> + u64 csr_reg_base,
> + u64 datal, u64 datah,
> + int count, u8 add_half,
> + u64 *monobit_fail_cnt,
> + u64 *run_fail_cnt,
> + u64 *poker_fail_cnt)
> +{
> + u64 status, csr_reg_off;
> + int test_idx;
unsigned int.
> +
> + if (!monobit_fail_cnt || !run_fail_cnt || !poker_fail_cnt)
> + return -EINVAL;
> +
> + for (test_idx = 0; test_idx < count; test_idx++) {
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_RAW_L_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, datal);
> +
> + if (add_half) {
> + if (test_idx < count - 1) {
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
Misaligned.
> + MLXBF_PKA_TRNG_RAW_H_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, datah);
> + }
> + } else {
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_RAW_H_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, datah);
> + }
The entire logic here should be rethought such that code is not
duplicated.
> +
> + /*
> + * Wait until the 'test_ready' bit in the TRNG_STATUS register becomes '1' again,
> + * signaling readiness for the next 64 bits of test data. At this point, the
> + * previous test data has been handled so the counter states can be inspected.
> + */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_STATUS_ADDR);
> + do {
> + status = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + } while (!(status & MLXBF_PKA_TRNG_STATUS_TEST_READY));
Infinite loop if HW misbehaves? Use read_poll_timeout() & handle errors.
> +
> + /* Check test status bits. */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_INTACK_ADDR);
> + if (status & MLXBF_PKA_TRNG_STATUS_MONOBIT_FAIL) {
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off,
> + MLXBF_PKA_TRNG_STATUS_MONOBIT_FAIL);
> + *monobit_fail_cnt += 1;
> + } else if (status & MLXBF_PKA_TRNG_STATUS_RUN_FAIL) {
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off,
> + MLXBF_PKA_TRNG_STATUS_RUN_FAIL);
> + *run_fail_cnt += 1;
> + } else if (status & MLXBF_PKA_TRNG_STATUS_POKER_FAIL) {
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off,
> + MLXBF_PKA_TRNG_STATUS_POKER_FAIL);
> + *poker_fail_cnt += 1;
> + }
Are these fails prioritized like this so it's should count just into one
counter if more than one FAIL is asserted?
> + }
> +
> + return (*monobit_fail_cnt || *poker_fail_cnt || *run_fail_cnt) ? -EIO : 0;
> +}
> +
> +static void mlxbf_pka_dev_trng_disable_test(void __iomem *csr_reg_ptr, u64 csr_reg_base)
> +{
> + u64 status, val, csr_reg_off;
Add an empty line.
> + /*
> + * When done, clear the 'test_known_noise' bit in the TRNG_TEST register (will immediately
> + * clear the 'test_ready' bit in the TRNG_STATUS register and reset the 'monobit test',
> + * 'run test' and 'poker test' circuits) and clear the 'test_mode' bit in the TRNG_CONTROL
> + * register.
> + */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_TEST_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_STATUS_ADDR);
> + status = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> +
> + if (status & MLXBF_PKA_TRNG_STATUS_TEST_READY)
> + pr_info("mlxbf_pka warning: test ready bit is still set\n");
> +
> + if (status &
> + (MLXBF_PKA_TRNG_STATUS_MONOBIT_FAIL |
> + MLXBF_PKA_TRNG_STATUS_RUN_FAIL |
> + MLXBF_PKA_TRNG_STATUS_POKER_FAIL))
Fix alignment.
Add define for this bit combination as you used it twice already.
> + pr_info("mlxbf_pka warning: test bits are still set, TRNG_STATUS:0x%llx\n", status);
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONTROL_ADDR);
> + val = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, (val & ~MLXBF_PKA_TRNG_STATUS_TEST_READY));
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_ALARMCNT_ADDR);
> + val = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + mlxbf_pka_dev_io_write(csr_reg_ptr,
> + csr_reg_off,
> + (val & ~MLXBF_PKA_TRNG_ALARMCNT_STALL_RUN_POKER));
> +}
> +
> +static int mlxbf_pka_dev_trng_test_known_answer_basic(void __iomem *csr_reg_ptr, u64 csr_reg_base)
> +{
> + u64 poker_cnt[MLXBF_PKA_TRNG_POKER_TEST_CNT];
> + u64 monobit_fail_cnt = 0;
> + u64 poker_fail_cnt = 0;
> + u64 run_fail_cnt = 0;
> + u64 monobit_cnt;
> + u64 csr_reg_off;
> + int cnt_idx;
> + int cnt_off;
> + int ret;
> +
> + pr_debug("mlxbf_pka: run known-answer test circuits\n");
> +
> + ret = mlxbf_pka_dev_trng_enable_test(csr_reg_ptr, csr_reg_base,
> + MLXBF_PKA_TRNG_TEST_KNOWN_NOISE);
> + if (ret)
> + return ret;
> +
> + ret = mlxbf_pka_dev_trng_test_circuits(csr_reg_ptr,
> + csr_reg_base,
> + MLXBF_PKA_TRNG_TEST_DATAL_BASIC_1,
> + MLXBF_PKA_TRNG_TEST_DATAH_BASIC_1,
> + MLXBF_PKA_TRNG_TEST_COUNT_BASIC_1,
> + MLXBF_PKA_TRNG_TEST_HALF_NO,
> + &monobit_fail_cnt,
> + &run_fail_cnt,
> + &poker_fail_cnt);
> +
> + ret |= mlxbf_pka_dev_trng_test_circuits(csr_reg_ptr,
> + csr_reg_base,
> + MLXBF_PKA_TRNG_TEST_DATAL_BASIC_2,
> + MLXBF_PKA_TRNG_TEST_DATAH_BASIC_2,
> + MLXBF_PKA_TRNG_TEST_COUNT_BASIC_2,
> + MLXBF_PKA_TRNG_TEST_HALF_ADD,
> + &monobit_fail_cnt,
> + &run_fail_cnt,
> + &poker_fail_cnt);
> +
> + pr_debug("mlxbf_pka: monobit_fail_cnt : 0x%llx\n", monobit_fail_cnt);
> + pr_debug("mlxbf_pka: poker_fail_cnt : 0x%llx\n", poker_fail_cnt);
> + pr_debug("mlxbf_pka: run_fail_cnt : 0x%llx\n", run_fail_cnt);
> +
> + for (cnt_idx = 0, cnt_off = 0;
> + cnt_idx < MLXBF_PKA_TRNG_POKER_TEST_CNT;
> + cnt_idx++, cnt_off += 8) {
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
Misaligned.
> + (MLXBF_PKA_TRNG_POKER_3_0_ADDR + cnt_off));
> + poker_cnt[cnt_idx] = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + }
> +
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
Misaligned.
> + MLXBF_PKA_TRNG_MONOBITCNT_ADDR);
> + monobit_cnt = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> +
> + if (!ret) {
Reverse logic + use goto.
> + if (memcmp(poker_cnt,
> + poker_test_exp_cnt,
> + sizeof(poker_test_exp_cnt))) {
> + pr_debug("mlxbf_pka: invalid poker counters!\n");
> + ret = -EIO;
> + }
> +
> + if (monobit_cnt != MLXBF_PKA_TRNG_MONOBITCNT_SUM) {
> + pr_debug("mlxbf_pka: invalid sum of squares!\n");
> + ret = -EIO;
> + }
> + }
> +
> + mlxbf_pka_dev_trng_disable_test(csr_reg_ptr, csr_reg_base);
> +
> + return ret;
> +}
> +
> +static int mlxbf_pka_dev_trng_test_known_answer_poker_fail(void __iomem *csr_reg_ptr,
> + u64 csr_reg_base)
> +{
> + u64 monobit_fail_cnt = 0;
> + u64 poker_fail_cnt = 0;
> + u64 run_fail_cnt = 0;
> +
> + pr_debug("mlxbf_pka: run known-answer test circuits (poker fail)\n");
> +
> + mlxbf_pka_dev_trng_enable_test(csr_reg_ptr, csr_reg_base, MLXBF_PKA_TRNG_TEST_KNOWN_NOISE);
> +
> + /*
> + * Ignore the return value here as it is expected that poker test should fail. Check failure
> + * counts thereafter to assert only poker test has failed.
> + */
> + mlxbf_pka_dev_trng_test_circuits(csr_reg_ptr,
> + csr_reg_base,
> + MLXBF_PKA_TRNG_TEST_DATAL_POKER,
> + MLXBF_PKA_TRNG_TEST_DATAH_POKER,
> + MLXBF_PKA_TRNG_TEST_COUNT_POKER,
> + MLXBF_PKA_TRNG_TEST_HALF_NO,
> + &monobit_fail_cnt,
> + &run_fail_cnt,
> + &poker_fail_cnt);
> +
> + pr_debug("mlxbf_pka: monobit_fail_cnt : 0x%llx\n", monobit_fail_cnt);
> + pr_debug("mlxbf_pka: poker_fail_cnt : 0x%llx\n", poker_fail_cnt);
> + pr_debug("mlxbf_pka: run_fail_cnt : 0x%llx\n", run_fail_cnt);
> +
> + mlxbf_pka_dev_trng_disable_test(csr_reg_ptr, csr_reg_base);
> +
> + return (poker_fail_cnt && !run_fail_cnt && !monobit_fail_cnt) ? 0 : -EIO;
> +}
> +
> +static int mlxbf_pka_dev_trng_test_unknown_answer(void __iomem *csr_reg_ptr, u64 csr_reg_base)
> +{
> + u64 datal = 0, datah = 0, csr_reg_off;
> + int ret = 0, test_idx;
> +
> + pr_debug("mlxbf_pka: run unknown-answer self test\n");
> +
> + /* First reset, the RAW registers. */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_RAW_L_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_RAW_H_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + /*
> + * There is a small probability for this test to fail. So run the test 10 times, if it
> + * succeeds once then assume that the test passed.
> + */
> + for (test_idx = 0; test_idx < 10; test_idx++) {
> + mlxbf_pka_dev_trng_enable_test(csr_reg_ptr, csr_reg_base,
> + MLXBF_PKA_TRNG_TEST_NOISE);
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_RAW_L_ADDR);
> + datal = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_RAW_H_ADDR);
> + datah = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> +
> + pr_debug("mlxbf_pka: datal=0x%llx\n", datal);
> + pr_debug("mlxbf_pka: datah=0x%llx\n", datah);
> +
> + mlxbf_pka_dev_trng_disable_test(csr_reg_ptr, csr_reg_base);
> +
> + if (!datah && !datal)
> + ret = -EIO;
> + else
> + return 0;
> + }
> + return ret;
> +}
> +
> +/* Test TRNG. */
> +static int mlxbf_pka_dev_test_trng(void __iomem *csr_reg_ptr, u64 csr_reg_base)
> +{
> + int ret;
> +
> + ret = mlxbf_pka_dev_trng_test_known_answer_basic(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + ret = mlxbf_pka_dev_trng_test_known_answer_poker_fail(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + ret = mlxbf_pka_dev_trng_test_unknown_answer(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + return ret;
> +}
> +
> +static void mlxbf_pka_dev_trng_write_ps_ai_str(void __iomem *csr_reg_ptr,
> + u64 csr_reg_base,
> + u32 input_str[])
> +{
> + u64 csr_reg_off;
> + int i;
unsigned int for loop variables that count from 0 upwards.
> +
> + for (i = 0; i < MLXBF_PKA_TRNG_PS_AI_REG_COUNT; i++) {
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
Please fix all these alignment issues.
> + MLXBF_PKA_TRNG_PS_AI_0_ADDR +
> + (i * MLXBF_PKA_TRNG_OUTPUT_REG_OFFSET));
Unnecessary ().
> +
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, input_str[i]);
> + }
> +}
> +
> +static void mlxbf_pka_dev_trng_drbg_generate(void __iomem *csr_reg_ptr, u64 csr_reg_base)
> +{
> + u64 csr_reg_off;
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONTROL_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, MLXBF_PKA_TRNG_CONTROL_REQ_DATA_VAL);
> +}
> +
> +static int mlxbf_pka_dev_test_trng_drbg(void __iomem *csr_reg_ptr, u64 csr_reg_base)
> +{
> + u64 csr_reg_off, csr_reg_val;
> + int i, ret;
> +
> + /* Make sure the engine is idle. */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONTROL_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + /* Enable DRBG, TRNG need not be enabled for this test. */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONTROL_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, MLXBF_PKA_TRNG_CONTROL_DRBG_ENABLE_VAL);
> +
> + /* Set 'test_sp_800_90' bit in the TRNG_TEST register. */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_TEST_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, MLXBF_PKA_TRNG_TEST_DRBG_VAL);
> +
> + /* Wait for 'test_ready' bit to be set. */
> + ret = mlxbf_pka_dev_trng_wait_test_ready(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + /* 'Instantiate' function. */
> + mlxbf_pka_dev_trng_write_ps_ai_str(csr_reg_ptr,
> + csr_reg_base,
> + mlxbf_pka_trng_drbg_test_ps_str);
> + ret = mlxbf_pka_dev_trng_wait_test_ready(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + /* 'Generate' function. */
> + mlxbf_pka_dev_trng_write_ps_ai_str(csr_reg_ptr,
> + csr_reg_base,
> + mlxbf_pka_trng_drbg_test_etpy_str1);
> + ret = mlxbf_pka_dev_trng_wait_test_ready(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + /*
> + * A standard NIST SP 800-90A DRBG known-answer test discards the result of the first
> + * 'Generate' function and only checks the result of the second 'Generate' function. Hence
> + * 'Generate' is performed again.
> + */
> +
> + /* 'Generate' function. */
> + mlxbf_pka_dev_trng_write_ps_ai_str(csr_reg_ptr,
> + csr_reg_base,
> + mlxbf_pka_trng_drbg_test_etpy_str2);
> + ret = mlxbf_pka_dev_trng_wait_test_ready(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + /* Check output registers. */
> + for (i = 0; i < MLXBF_PKA_TRNG_OUTPUT_CNT; i++) {
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_OUTPUT_0_ADDR +
> + (i * MLXBF_PKA_TRNG_OUTPUT_REG_OFFSET));
> +
> + csr_reg_val = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> +
> + if ((u32)csr_reg_val != mlxbf_pka_trng_drbg_test_output[i]) {
> + pr_debug
> + ("mlxbf_pka: DRBG known answer test failed: output register:%d, 0x%x\n",
> + i, (u32)csr_reg_val);
> + return 1;
> + }
> + }
> +
> + /* Clear 'test_sp_800_90' bit in the TRNG_TEST register. */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_TEST_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + return 0;
> +}
> +
> +/* Configure the TRNG. */
> +static int mlxbf_pka_dev_config_trng_drbg(struct mlxbf_pka_dev_res_t *aic_csr_ptr,
> + struct mlxbf_pka_dev_res_t *trng_csr_ptr)
> +{
> + u64 csr_reg_base, csr_reg_off;
> + void __iomem *csr_reg_ptr;
> + int ret;
> +
> + if (trng_csr_ptr->status != MLXBF_PKA_DEV_RES_STATUS_MAPPED ||
> + trng_csr_ptr->type != MLXBF_PKA_DEV_RES_TYPE_REG)
> + return -EPERM;
> +
> + pr_debug("mlxbf_pka: starting up the TRNG\n");
> +
> + ret = mlxbf_pka_dev_config_trng_clk(aic_csr_ptr);
> + if (ret)
> + return ret;
> +
> + csr_reg_base = trng_csr_ptr->base;
> + csr_reg_ptr = trng_csr_ptr->ioaddr;
> +
> + /*
> + * Perform NIST known-answer tests on the complete SP 800-90A DRBG without BC_DF
> + * functionality.
> + */
> + ret = mlxbf_pka_dev_test_trng_drbg(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + /* Starting up the TRNG with a DRBG. */
> +
> + /* Make sure the engine is idle. */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONTROL_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + /* Disable all FROs initially. */
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_FROENABLE_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_FRODETUNE_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + /*
> + * Write all configuration values in the TRNG_CONFIG and TRNG_ALARMCNT, write zeroes to the
> + * TRNG_ALARMMASK and TRNG_ALARMSTOP registers.
> + */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONFIG_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, MLXBF_PKA_TRNG_CONFIG_REG_VAL);
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_ALARMCNT_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, MLXBF_PKA_TRNG_ALARMCNT_REG_VAL);
> +
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_ALARMMASK_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_ALARMSTOP_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + /*
> + * Enable all FROs in the TRNG_FROENABLE register. Note that this can only be done after
> + * clearing the TRNG_ALARMSTOP register.
> + */
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_FROENABLE_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, MLXBF_PKA_TRNG_FROENABLE_REG_VAL);
> +
> + /*
> + * Optionally, write 'Personalization string' of up to 384 bits in TRNG_PS_AI_xxx registers.
> + * The contents of these registers will be XOR-ed into the output of the SHA-256
> + * 'Conditioning Function' to be used as seed value for the actual DRBG.
> + */
> + mlxbf_pka_dev_trng_write_ps_ai_str(csr_reg_ptr, csr_reg_base, mlxbf_pka_trng_drbg_ps_str);
> +
> + /*
> + * Run TRNG tests after configuring TRNG.
> + * NOTE: TRNG need not be enabled to carry out these tests.
> + */
> + ret = mlxbf_pka_dev_test_trng(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + /*
> + * Start the actual engine by setting the 'enable_trng' and 'drbg_en' bit in the
> + * TRNG_CONTROL register (also a nice point to set the interrupt mask bits).
> + */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONTROL_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, MLXBF_PKA_TRNG_CONTROL_DRBG_REG_VAL);
> +
> + /*
> + * The engine is now ready to handle the first 'Generate' request using the 'request_data'
> + * bit of the TRNG_CONTROL register. The first output for these requests will take a while,
> + * as Noise Source and Conditioning Function must first generate seed entropy for the DRBG.
> + *
> + * Optionally, when buffer RAM is configured: Set a data available interrupt threshold using
> + * the 'load_thresh' and 'blocks_thresh' fields of the TRNG_INTACK register. This allows
> + * delaying the data available interrupt until the indicated number of 128-bit words are
> + * available in the buffer RAM.
> + *
> + * Start the actual 'Generate' operation using the 'request_data' and 'data_blocks' fields
> + * of the TRNG_CONTROL register.
> + */
> + mlxbf_pka_dev_trng_drbg_generate(csr_reg_ptr, csr_reg_base);
> +
> + /* Delay 200 ms. */
> + mdelay(200);
> +
> + return 0;
> +}
> +
> +/*
> + * Initialize PKA IO block referred to as shim. It configures shim's parameters and prepare
> + * resources by mapping corresponding memory. The function also configures shim registers and load
> + * firmware to shim internal rams. The struct mlxbf_pka_dev_shim_s passed as input is also an
> + * output. It returns 0 on success, a negative error code on failure.
> + */
> +static int mlxbf_pka_dev_init_shim(struct mlxbf_pka_dev_shim_s *shim)
> +{
> + u32 data[MLXBF_PKA_TRNG_OUTPUT_CNT];
> + int ret;
> + u8 i;
> +
> + if (shim->status != MLXBF_PKA_SHIM_STATUS_CREATED) {
> + pr_err("mlxbf_pka error: PKA device must be created\n");
> + return -EPERM;
> + }
> +
> + /* Configure PKA Ring options control word. */
> + ret = mlxbf_pka_dev_config_ring_options(&shim->resources.buffer_ram,
> + shim->rings_num,
> + shim->ring_priority);
> + if (ret) {
> + pr_err("mlxbf_pka error: failed to configure ring options\n");
> + return ret;
> + }
> +
> + shim->trng_enabled = MLXBF_PKA_SHIM_TRNG_ENABLED;
> + shim->trng_err_cycle = 0;
> +
> + /* Configure the TRNG. */
> + ret = mlxbf_pka_dev_config_trng_drbg(&shim->resources.aic_csr, &shim->resources.trng_csr);
> +
> + /*
> + * Pull out data from the content of the TRNG buffer RAM and start the regeneration of new
> + * numbers; read and drop 512 words. The read must be done over the 4 TRNG_OUTPUT_X
> + * registers at a time.
> + */
> + for (i = 0; i < MLXBF_PKA_TRNG_NUM_OF_FOUR_WORD; i++)
> + mlxbf_pka_dev_trng_read(shim, data, sizeof(data));
> +
> + if (ret) {
> + /* Keep running without TRNG since it does not hurt, but notify users. */
> + pr_err("mlxbf_pka error: failed to configure TRNG\n");
> + shim->trng_enabled = MLXBF_PKA_SHIM_TRNG_DISABLED;
> + }
> +
> + mutex_init(&shim->mutex);
devm_mutex_init() + error handling
> + shim->busy_ring_num = 0;
> + shim->status = MLXBF_PKA_SHIM_STATUS_INITIALIZED;
> +
> + return ret;
> +}
> +
> +/* Release a given shim. */
> +static int mlxbf_pka_dev_release_shim(struct mlxbf_pka_dev_shim_s *shim)
> +{
> + u32 ring_idx;
> + int ret = 0;
> +
> + if (shim->status != MLXBF_PKA_SHIM_STATUS_INITIALIZED &&
> + shim->status != MLXBF_PKA_SHIM_STATUS_STOPPED) {
> + pr_err("mlxbf_pka error: PKA device must be initialized or stopped\n");
> + return -EPERM;
> + }
> +
> + /*
> + * Release rings which belong to the shim. The operating system might release ring devices
> + * before shim devices. The global configuration must be checked before proceeding to the
> + * release of ring devices.
> + */
> + if (mlxbf_pka_gbl_config.dev_rings_cnt) {
> + for (ring_idx = 0; ring_idx < shim->rings_num; ring_idx++) {
> + ret = mlxbf_pka_dev_release_ring(shim->rings[ring_idx]);
> + if (ret) {
> + pr_err("mlxbf_pka error: failed to release ring %d\n", ring_idx);
> + return ret;
> + }
> + }
> + }
> +
> + shim->busy_ring_num = 0;
> + shim->status = MLXBF_PKA_SHIM_STATUS_FINALIZED;
> +
> + return ret;
> +}
> +
> +/* Return the ring associated with the given identifier. */
> +struct mlxbf_pka_dev_ring_t *mlxbf_pka_dev_get_ring(u32 ring_id)
> +{
> + return mlxbf_pka_gbl_config.dev_rings[ring_id];
> +}
> +
> +/* Return the shim associated with the given identifier. */
> +struct mlxbf_pka_dev_shim_s *mlxbf_pka_dev_get_shim(u32 shim_id)
> +{
> + return mlxbf_pka_gbl_config.dev_shims[shim_id];
> +}
> +
> +struct mlxbf_pka_dev_ring_t *mlxbf_pka_dev_register_ring(struct device *dev,
> + u32 ring_id,
> + u32 shim_id)
> +{
> + struct mlxbf_pka_dev_shim_s *shim;
> + struct mlxbf_pka_dev_ring_t *ring;
> + int ret;
> +
> + shim = mlxbf_pka_dev_get_shim(shim_id);
> + if (!shim)
> + return NULL;
> +
> + ring = devm_kzalloc(dev, sizeof(*ring), GFP_KERNEL);
> + if (!ring)
> + return NULL;
> +
> + ring->status = MLXBF_PKA_DEV_RING_STATUS_UNDEFINED;
> +
> + /* Initialize ring. */
> + ret = mlxbf_pka_dev_init_ring(dev, ring, ring_id, shim);
> + if (ret) {
> + dev_err(dev, "failed to initialize ring %d\n", ring_id);
> + mlxbf_pka_dev_release_ring(ring);
> + return NULL;
> + }
> +
> + mlxbf_pka_gbl_config.dev_rings[ring->ring_id] = ring;
> + mlxbf_pka_gbl_config.dev_rings_cnt += 1;
> +
> + return ring;
> +}
> +
> +int mlxbf_pka_dev_unregister_ring(struct mlxbf_pka_dev_ring_t *ring)
> +{
> + if (!ring)
> + return -EINVAL;
> +
> + mlxbf_pka_gbl_config.dev_rings[ring->ring_id] = NULL;
> + mlxbf_pka_gbl_config.dev_rings_cnt -= 1;
> +
> + /* Release ring. */
> + return mlxbf_pka_dev_release_ring(ring);
> +}
> +
> +struct mlxbf_pka_dev_shim_s *mlxbf_pka_dev_register_shim(struct device *dev,
> + u32 shim_id,
> + struct mlxbf_pka_dev_mem_res *mem_res)
> +{
> + struct mlxbf_pka_dev_shim_s *shim;
> + u8 split;
> + int ret;
> +
> + dev_dbg(dev, "register shim id=%u\n", shim_id);
> +
> + shim = devm_kzalloc(dev, sizeof(*shim), GFP_KERNEL);
> + if (!shim)
> + return shim;
> +
> + /*
> + * Shim state MUST be set to undefined before calling 'mlxbf_pka_dev_create_shim' function.
> + */
> + shim->status = MLXBF_PKA_SHIM_STATUS_UNDEFINED;
> +
> + /* Set the Window RAM user mode. */
> + split = MLXBF_PKA_SPLIT_WINDOW_RAM_MODE;
> +
> + /* Create PKA shim. */
> + ret = mlxbf_pka_dev_create_shim(dev, shim, shim_id, split, mem_res);
> + if (ret) {
> + dev_err(dev, "failed to create shim %u\n", shim_id);
> + mlxbf_pka_dev_delete_shim(shim);
> + return NULL;
> + }
> +
> + /* Initialize PKA shim. */
> + ret = mlxbf_pka_dev_init_shim(shim);
> + if (ret) {
> + dev_err(dev, "failed to init shim %u\n", shim_id);
> + mlxbf_pka_dev_release_shim(shim);
> + mlxbf_pka_dev_delete_shim(shim);
> + return NULL;
Instead of duplicating rollbacks, please make a proper rollback path with
labels and gotos.
> + }
> +
> + mlxbf_pka_gbl_config.dev_shims[shim->shim_id] = shim;
> + mlxbf_pka_gbl_config.dev_shims_cnt += 1;
> +
> + return shim;
> +}
> +
> +int mlxbf_pka_dev_unregister_shim(struct mlxbf_pka_dev_shim_s *shim)
> +{
> + int ret;
> +
> + if (!shim)
> + return -EINVAL;
> +
> + mlxbf_pka_gbl_config.dev_shims[shim->shim_id] = NULL;
> + mlxbf_pka_gbl_config.dev_shims_cnt -= 1;
> +
> + /* Release shim. */
> + ret = mlxbf_pka_dev_release_shim(shim);
> + if (ret)
> + return ret;
> +
> + /* Delete shim. */
> + return mlxbf_pka_dev_delete_shim(shim);
> +}
> +
> +static bool mlxbf_pka_dev_trng_shutdown_oflo(struct mlxbf_pka_dev_res_t *trng_csr_ptr,
> + u64 *err_cycle)
> +{
> + u64 curr_cycle_cnt, fro_stopped_mask, fro_enabled_mask;
> + u64 csr_reg_base, csr_reg_off, csr_reg_value;
> + void __iomem *csr_reg_ptr;
> +
> + csr_reg_base = trng_csr_ptr->base;
> + csr_reg_ptr = trng_csr_ptr->ioaddr;
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_STATUS_ADDR);
> + csr_reg_value = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> +
> + if (csr_reg_value & MLXBF_PKA_TRNG_STATUS_SHUTDOWN_OFLO) {
> + curr_cycle_cnt = get_cycles();
> + /*
> + * See if any FROs were shut down. If they were, toggle bits in the FRO detune
> + * register and reenable the FROs.
> + */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_ALARMSTOP_ADDR);
> + fro_stopped_mask = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + if (fro_stopped_mask) {
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_FROENABLE_ADDR);
> + fro_enabled_mask = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_FRODETUNE_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, fro_stopped_mask);
> +
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_FROENABLE_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off,
> + fro_stopped_mask | fro_enabled_mask);
> + }
> +
> + /* Reset the error. */
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_ALARMMASK_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_ALARMSTOP_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, 0);
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_INTACK_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr,
> + csr_reg_off,
> + MLXBF_PKA_TRNG_STATUS_SHUTDOWN_OFLO);
> +
> + /*
> + * If this error occurs again within about a second, the hardware is malfunctioning.
> + * Disable the trng and return an error.
> + */
> + if (*err_cycle &&
> + (curr_cycle_cnt - *err_cycle < MLXBF_PKA_TRNG_TEST_ERR_CYCLE_MAX)) {
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_CONTROL_ADDR);
> + csr_reg_value = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + csr_reg_value &= ~MLXBF_PKA_TRNG_CONTROL_REG_VAL;
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, csr_reg_value);
> + return false;
> + }
> +
> + *err_cycle = curr_cycle_cnt;
> + }
> +
> + return true;
> +}
> +
> +static int mlxbf_pka_dev_trng_drbg_reseed(void __iomem *csr_reg_ptr, u64 csr_reg_base)
> +{
> + u64 csr_reg_off;
> + int ret;
> +
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_CONTROL_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off, MLXBF_PKA_TRNG_CONTROL_DRBG_RESEED);
> +
> + ret = mlxbf_pka_dev_trng_wait_test_ready(csr_reg_ptr, csr_reg_base);
> + if (ret)
> + return ret;
> +
> + /* Write personalization string. */
> + mlxbf_pka_dev_trng_write_ps_ai_str(csr_reg_ptr, csr_reg_base, mlxbf_pka_trng_drbg_ps_str);
> +
> + return ret;
> +}
> +
> +/* Read from DRBG enabled TRNG. */
> +int mlxbf_pka_dev_trng_read(struct mlxbf_pka_dev_shim_s *shim, u32 *data, u32 cnt)
> +{
> + u64 csr_reg_base, csr_reg_off, csr_reg_value, timer;
> + struct mlxbf_pka_dev_res_t *trng_csr_ptr;
> + u8 output_idx, trng_ready = 0;
> + u32 data_idx, word_cnt;
> + void __iomem *csr_reg_ptr;
> + int ret = 0;
> +
> + if (!shim || !data || (cnt % MLXBF_PKA_TRNG_OUTPUT_CNT != 0))
> + return -EINVAL;
> +
> + if (!cnt)
> + return ret;
> +
> + mutex_lock(&shim->mutex);
Please use guard() to reduce complexity of the error handling.
> +
> + trng_csr_ptr = &shim->resources.trng_csr;
> +
> + if (trng_csr_ptr->status != MLXBF_PKA_DEV_RES_STATUS_MAPPED ||
> + trng_csr_ptr->type != MLXBF_PKA_DEV_RES_TYPE_REG) {
> + ret = -EPERM;
> + goto exit;
> + }
> +
> + csr_reg_base = trng_csr_ptr->base;
> + csr_reg_ptr = trng_csr_ptr->ioaddr;
> +
> + if (!mlxbf_pka_dev_trng_shutdown_oflo(trng_csr_ptr, &shim->trng_err_cycle)) {
> + ret = -EWOULDBLOCK;
> + goto exit;
> + }
> +
> + /* Determine the number of 32-bit words. */
> + word_cnt = cnt >> 2;
word_cnt = cnt >> ilog2(sizeof(u32));
With that, you don't need the comment at all as the code explains itself.
> +
> + for (data_idx = 0; data_idx < word_cnt; data_idx++) {
> + output_idx = data_idx % MLXBF_PKA_TRNG_OUTPUT_CNT;
> +
> + /* Tell the hardware to advance. */
> + if (!output_idx) {
> + csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_INTACK_ADDR);
> + mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off,
> + MLXBF_PKA_TRNG_STATUS_READY);
> + trng_ready = 0;
> +
> + /*
> + * Check if 'data_blocks' field is zero in TRNG_CONTROL register. If it is
> + * zero, need to issue a 'Reseed and Generate' request for DRBG enabled
> + * TRNG.
> + */
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_CONTROL_ADDR);
> + csr_reg_value = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> +
> + if (!((u32)csr_reg_value & MLXBF_PKA_TRNG_DRBG_DATA_BLOCK_MASK)) {
> + /* Issue reseed. */
> + ret = mlxbf_pka_dev_trng_drbg_reseed(csr_reg_ptr, csr_reg_base);
> + if (ret) {
> + ret = -EBUSY;
> + goto exit;
> + }
> +
> + /* Issue generate request. */
> + mlxbf_pka_dev_trng_drbg_generate(csr_reg_ptr, csr_reg_base);
> + }
> + }
> +
> + /*
> + * Wait until a data word is available in the TRNG_OUTPUT_X registers, using the
> + * interrupt and/or 'ready' status bit in the TRNG_STATUS register. The only way
> + * this would hang is if the TRNG is never initialized. This function cannot be
> + * called if that happened.
> + */
> + timer = mlxbf_pka_dev_timer_start_msec(1000);
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base, MLXBF_PKA_TRNG_STATUS_ADDR);
> + while (!trng_ready) {
> + csr_reg_value = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + trng_ready = csr_reg_value & MLXBF_PKA_TRNG_STATUS_READY;
Eh, where's the closing brace?
> +
> + if (mlxbf_pka_dev_timer_done(timer)) {
> + pr_debug("mlxbf_pka: shim %u got error obtaining random number\n",
> + shim->shim_id);
> + ret = -EBUSY;
> + goto exit;
> + }
> + }
> +
> + /* Read the registers. */
> + csr_reg_off =
> + mlxbf_pka_dev_get_register_offset(csr_reg_base,
> + MLXBF_PKA_TRNG_OUTPUT_0_ADDR +
> + (output_idx * MLXBF_PKA_TRNG_OUTPUT_REG_OFFSET));
> + csr_reg_value = mlxbf_pka_dev_io_read(csr_reg_ptr, csr_reg_off);
> + data[data_idx] = (u32)csr_reg_value;
> + }
?????
> +
> +exit:
> + mutex_unlock(&shim->mutex);
> + return ret;
> +}
> +
> +bool mlxbf_pka_dev_has_trng(struct mlxbf_pka_dev_shim_s *shim)
> +{
> + if (!shim)
> + return false;
> +
> + return (shim->trng_enabled == MLXBF_PKA_SHIM_TRNG_ENABLED);
> +}
> +
> +/* Syscall to open ring. */
> +static int __mlxbf_pka_dev_open_ring(u32 ring_id)
> +{
> + struct mlxbf_pka_dev_shim_s *shim;
> + struct mlxbf_pka_dev_ring_t *ring;
> + int ret;
> +
> + if (!mlxbf_pka_gbl_config.dev_rings_cnt)
> + return -EPERM;
> +
> + ring = mlxbf_pka_dev_get_ring(ring_id);
> + if (!ring || !ring->shim)
> + return -ENXIO;
> +
> + shim = ring->shim;
> +
> + mutex_lock(&ring->mutex);
Use guard() so you can return instead of using gotos below.
> +
> + if (shim->status == MLXBF_PKA_SHIM_STATUS_UNDEFINED ||
> + shim->status == MLXBF_PKA_SHIM_STATUS_CREATED ||
> + shim->status == MLXBF_PKA_SHIM_STATUS_FINALIZED) {
> + ret = -EPERM;
> + goto unlock_return;
> + }
> +
> + if (ring->status == MLXBF_PKA_DEV_RING_STATUS_BUSY) {
> + ret = -EBUSY;
> + goto unlock_return;
> + }
> +
> + if (ring->status != MLXBF_PKA_DEV_RING_STATUS_INITIALIZED) {
> + ret = -EPERM;
> + goto unlock_return;
> + }
> +
> + /* Set ring information words. */
> + ret = mlxbf_pka_dev_set_ring_info(ring);
> + if (ret) {
> + pr_err("mlxbf_pka error: failed to set ring information\n");
> + ret = -EWOULDBLOCK;
> + goto unlock_return;
> + }
> +
> + if (!shim->busy_ring_num)
> + shim->status = MLXBF_PKA_SHIM_STATUS_RUNNING;
> +
> + ring->status = MLXBF_PKA_DEV_RING_STATUS_BUSY;
> + shim->busy_ring_num += 1;
> +
> +unlock_return:
> + mutex_unlock(&ring->mutex);
> + return ret;
> +}
> +
> +/* Open ring. */
> +int mlxbf_pka_dev_open_ring(struct mlxbf_pka_ring_info_t *ring_info)
> +{
> + return __mlxbf_pka_dev_open_ring(ring_info->ring_id);
> +}
> +
> +/* Syscall to close ring. */
> +static int __mlxbf_pka_dev_close_ring(u32 ring_id)
> +{
> + struct mlxbf_pka_dev_shim_s *shim;
> + struct mlxbf_pka_dev_ring_t *ring;
> + int ret = 0;
> +
> + if (!mlxbf_pka_gbl_config.dev_rings_cnt)
> + return -EPERM;
> +
> + ring = mlxbf_pka_dev_get_ring(ring_id);
> + if (!ring || !ring->shim)
> + return -ENXIO;
> +
> + shim = ring->shim;
> +
> + mutex_lock(&ring->mutex);
guard()
> +
> + if (shim->status != MLXBF_PKA_SHIM_STATUS_RUNNING &&
> + ring->status != MLXBF_PKA_DEV_RING_STATUS_BUSY) {
> + ret = -EPERM;
> + goto unlock_return;
> + }
> +
> + ring->status = MLXBF_PKA_DEV_RING_STATUS_INITIALIZED;
> + shim->busy_ring_num -= 1;
> +
> + if (!shim->busy_ring_num)
> + shim->status = MLXBF_PKA_SHIM_STATUS_STOPPED;
> +
> +unlock_return:
> + mutex_unlock(&ring->mutex);
> + return ret;
> +}
> +
> +/* Close ring. */
> +int mlxbf_pka_dev_close_ring(struct mlxbf_pka_ring_info_t *ring_info)
> +{
> + if (ring_info)
> + return __mlxbf_pka_dev_close_ring(ring_info->ring_id);
> +
> + return 0;
> +}
> diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
> new file mode 100644
> index 000000000000..3054476215bd
> --- /dev/null
> +++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_dev.h
> @@ -0,0 +1,657 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause */
> +/* SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION. All rights reserved. */
> +
> +#ifndef __MLXBF_PKA_DEV_H__
> +#define __MLXBF_PKA_DEV_H__
> +
> +/*
> + * @file
> + *
> + * API to handle the PKA EIP-154 I/O block (shim). It provides functions and data structures to
> + * initialize and configure the PKA shim. It's the "southband interface" for communication with PKA
> + * hardware resources.
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
Please order any includes alphabetically.
> +
> +/* PKA ring device related definitions. */
> +#define CMD_DESC_SIZE 64
> +
> +/*
> + * Describes the PKA command/result ring as used by the hardware. A pair of command and result rings
> + * in PKA window memory, and the data memory used by the commands.
> + */
> +struct mlxbf_pka_ring_desc_t {
> + u32 num_descs; /* Total number of descriptors in the ring. */
> +
> + u32 cmd_ring_base; /* Base address of the command ring. */
> + u32 cmd_idx; /* Index of the command in a ring. */
> +
> + u32 rslt_ring_base; /* Base address of the result ring. */
> + u32 rslt_idx; /* Index of the result in a ring. */
> +
> + u32 operands_base; /* Operands memory base address. */
> + u32 operands_end; /* End address of operands memory. */
> +
> + u32 desc_size; /* Size of each element in the ring. */
> +
> + u64 cmd_desc_mask; /* Bitmask of free(0)/in_use(1) cmd descriptors. */
> + u32 cmd_desc_cnt; /* Number of command descriptors currently in use. */
> + u32 rslt_desc_cnt; /* Number of result descriptors currently ready. */
Please move those comments into kerneldoc before the struct.
> +};
> +
> +/* This structure declares ring parameters which can be used by user interface. */
> +struct mlxbf_pka_ring_info_t {
> + int fd; /* File descriptor. */
> + int group; /* Iommu group. */
> + int container; /* VFIO cointainer. */
> +
> + u32 idx; /* Ring index. */
> + u32 ring_id; /* Hardware ring identifier. */
> +
> + u64 mem_off; /* Offset specific to window RAM region. */
> + u64 mem_addr; /* Window RAM region address. */
> + u64 mem_size; /* Window RAM region size. */
> +
> + u64 reg_off; /* Offset specific to count registers region. */
> + u64 reg_addr; /* Count registers region address. */
> + u64 reg_size; /* Count registers region size. */
> +
> + void *mem_ptr; /* Pointer to mapped memory region. */
> + void *reg_ptr; /* Pointer to mapped counters region. */
> +
> + struct mlxbf_pka_ring_desc_t ring_desc; /* Ring descriptor. */
> +
> + u8 big_endian; /* Big endian byte order when enabled. */
comments -> kerneldoc.
> +};
> +
> +/* PKA IOCTL related definitions. */
> +#define MLXBF_PKA_IOC_TYPE 0xB7
> +
> +/*
> + * MLXBF_PKA_RING_GET_REGION_INFO - _IORW(MLXBF_PKA_IOC_TYPE, 0x0, mlxbf_pka_dev_region_info_t).
> + *
> + * Retrieve information about a device region. This is intended to describe MMIO, I/O port, as well
> + * as bus specific regions (ex. PCI config space). Zero sized regions may be used to describe
> + * unimplemented regions.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct mlxbf_pka_dev_region_info_t {
> + u32 reg_index; /* Registers region index. */
> + u64 reg_size; /* Registers region size (bytes). */
> + u64 reg_offset; /* Registers region offset from start of device fd. */
> +
> + u32 mem_index; /* Memory region index. */
> + u64 mem_size; /* Memory region size (bytes). */
> + u64 mem_offset; /* Memory region offset from start of device fd. */
> +};
> +
> +#define MLXBF_PKA_RING_GET_REGION_INFO \
> + _IOWR(MLXBF_PKA_IOC_TYPE, 0x0, struct mlxbf_pka_dev_region_info_t)
> +
> +/*
> + * MLXBF_PKA_GET_RING_INFO - _IORW(MLXBF_PKA_IOC_TYPE, 0x1, mlxbf_pka_dev_ring_info_t).
> + *
> + * Retrieve information about a ring. This is intended to describe ring information words located in
> + * MLXBF_PKA_BUFFER_RAM. Ring information includes base addresses, size and statistics.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +
> +/* Bluefield specific ring information. */
> +struct mlxbf_pka_dev_hw_ring_info_t {
> + /* Base address of the command descriptor ring. */
> + u64 cmmd_base;
> +
> + /* Base address of the result descriptor ring. */
> + u64 rslt_base;
> +
> + /*
> + * Size of a command ring in number of descriptors, minus 1. Minimum value is 0 (for 1
> + * descriptor); maximum value is 65535 (for 64K descriptors).
> + */
> + u16 size;
> +
> + /*
> + * This field specifies the size (in 32-bit words) of the space that PKI command and result
> + * descriptor occupies on the Host.
> + */
> + u16 host_desc_size : 10;
> +
> + /*
> + * Indicates whether the result ring delivers results strictly in-order ('1') or that result
> + * descriptors are written to the result ring as soon as they become available, or out-of-
> + * order ('0').
> + */
> + u8 in_order : 1;
> +
> + /* Read pointer of the command descriptor ring. */
> + u16 cmmd_rd_ptr;
> +
> + /* Write pointer of the result descriptor ring. */
> + u16 rslt_wr_ptr;
> +
> + /* Read statistics of the command descriptor ring. */
> + u16 cmmd_rd_stats;
> +
> + /* Write statistics of the result descriptor ring. */
> + u16 rslt_wr_stats;
> +};
> +
> +/* ring_info related definitions. */
> +#define MLXBF_PKA_RING_INFO_IN_ORDER_MASK 0x0001
GENMASK() or BIT() + add #include.
> +#define MLXBF_PKA_RING_INFO_IN_ORDER_OFFSET 31
> +#define MLXBF_PKA_RING_INFO_HOST_DESC_SIZE_MASK 0x03FF
GENMASK()
> +#define MLXBF_PKA_RING_INFO_HOST_DESC_SIZE_OFFSET 18
Probably can be dropped after FIELD_GET()/PREP() conversion.
> +#define MLXBF_PKA_RING_NUM_CMD_DESC_MASK 0xFFFF
GENMASK()
> +
> +#define MLXBF_PKA_GET_RING_INFO _IOWR(MLXBF_PKA_IOC_TYPE, 0x1, struct mlxbf_pka_dev_hw_ring_info_t)
This not an uapi header?
> +
> +/* Ring option related definitions. */
> +#define MLXBF_PKA_RING_OPTIONS_RING_PRIORITY_MASK 0xFF
> +#define MLXBF_PKA_RING_OPTIONS_RING_NUM_OFFSET 8
> +#define MLXBF_PKA_RING_OPTIONS_RING_NUM_MASK 0xFF00
> +#define MLXBF_PKA_RING_OPTIONS_SIGNATURE_BYTE_OFFSET 24
> +#define MLXBF_PKA_RING_OPTIONS_SIGNATURE_BYTE_MASK 0xFF000000
> +
> +/*
> + * MLXBF_PKA_CLEAR_RING_COUNTERS - _IO(MLXBF_PKA_IOC_TYPE, 0x2).
> + *
> + * Clear counters. This is intended to reset all command and result counters.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +#define MLXBF_PKA_CLEAR_RING_COUNTERS _IO(MLXBF_PKA_IOC_TYPE, 0x2)
> +
> +/*
> + * MLXBF_PKA_GET_RANDOM_BYTES - _IOWR(MLXBF_PKA_IOC_TYPE, 0x3, mlxbf_pka_dev_trng_info_t).
> + *
> + * Get random bytes from True Random Number Generator(TRNG).
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +
> +/* TRNG information. */
> +struct mlxbf_pka_dev_trng_info_t {
> + /* Number of random bytes in the buffer or length of the buffer. */
> + u32 count;
> +
> + /* Data buffer to hold the random bytes. */
> + u8 *data;
> +};
> +
> +#define MLXBF_PKA_GET_RANDOM_BYTES _IOWR(MLXBF_PKA_IOC_TYPE, 0x3, struct mlxbf_pka_dev_trng_info_t)
> +
> +/* PKA address related definitions. */
> +
> +/*
> + * Global Control Space CSR addresses/offsets. These are accessed from the ARM as 8 byte reads/
> + * writes. However only the bottom 32 bits are implemented.
> + */
> +#define MLXBF_PKA_CLK_FORCE_ADDR 0x11C80
> +
> +/*
> + * Advanced Interrupt Controller CSR addresses/offsets. These are accessed from the ARM as 8 byte
> + * reads/writes. However only the bottom 32 bits are implemented.
> + */
> +#define MLXBF_PKA_AIC_POL_CTRL_ADDR 0x11E00
> +
> +/*
> + * The True Random Number Generator CSR addresses/offsets. These are accessed from the ARM as 8 byte
> + * reads/writes. However only the bottom 32 bits are implemented.
> + */
> +#define MLXBF_PKA_TRNG_OUTPUT_0_ADDR 0x12000
> +#define MLXBF_PKA_TRNG_STATUS_ADDR 0x12020
> +#define MLXBF_PKA_TRNG_INTACK_ADDR 0x12020
> +#define MLXBF_PKA_TRNG_CONTROL_ADDR 0x12028
> +#define MLXBF_PKA_TRNG_CONFIG_ADDR 0x12030
> +#define MLXBF_PKA_TRNG_ALARMCNT_ADDR 0x12038
> +#define MLXBF_PKA_TRNG_FROENABLE_ADDR 0x12040
> +#define MLXBF_PKA_TRNG_FRODETUNE_ADDR 0x12048
> +#define MLXBF_PKA_TRNG_ALARMMASK_ADDR 0x12050
> +#define MLXBF_PKA_TRNG_ALARMSTOP_ADDR 0x12058
> +#define MLXBF_PKA_TRNG_TEST_ADDR 0x120E0
> +#define MLXBF_PKA_TRNG_RAW_L_ADDR 0x12060
> +#define MLXBF_PKA_TRNG_RAW_H_ADDR 0x12068
> +#define MLXBF_PKA_TRNG_MONOBITCNT_ADDR 0x120B8
> +#define MLXBF_PKA_TRNG_POKER_3_0_ADDR 0x120C0
> +#define MLXBF_PKA_TRNG_PS_AI_0_ADDR 0x12080
> +
> +/*
> + * Control register address/offset. This is accessed from the ARM using 8 byte reads/writes. However
> + * only the bottom 32 bits are implemented.
> + */
> +#define MLXBF_PKA_MASTER_SEQ_CTRL_ADDR 0x27F90
> +
> +/*
> + * Ring CSRs: these are all accessed from the ARM using 8 byte reads/writes. However only the bottom
> + * 32 bits are implemented.
> + */
> +/* Ring 0 CSRS. */
> +#define MLXBF_PKA_COMMAND_COUNT_0_ADDR 0x80080
> +
> +/* MLXBF_PKA_BUFFER_RAM: 1024 x 64 - 8K bytes. */
> +#define MLXBF_PKA_BUFFER_RAM_BASE 0x00000
> +#define MLXBF_PKA_BUFFER_RAM_SIZE SZ_16K /* 0x00000...0x03FFF. */
The comment and define do not match???
If you derive size from something, it would be nice to use that formula
directly in the define itself.
Missing include for SZ_16K.
> +
> +/*
> + * PKA Buffer RAM offsets. These are NOT real CSR's but instead are specific offset/addresses within
> + * the EIP154 MLXBF_PKA_BUFFER_RAM.
> + */
> +
> +/* Ring 0. */
> +#define MLXBF_PKA_RING_CMMD_BASE_0_ADDR 0x00000
> +#define MLXBF_PKA_RING_RSLT_BASE_0_ADDR 0x00010
> +#define MLXBF_PKA_RING_SIZE_TYPE_0_ADDR 0x00020
> +#define MLXBF_PKA_RING_RW_PTRS_0_ADDR 0x00028
> +#define MLXBF_PKA_RING_RW_STAT_0_ADDR 0x00030
> +
> +/* Ring Options. */
> +#define MLXBF_PKA_RING_OPTIONS_ADDR 0x07FF8
> +
> +/* Alternate Window RAM size. */
> +#define MLXBF_PKA_WINDOW_RAM_REGION_SIZE SZ_16K
> +
> +/* PKA configuration related definitions. */
> +
> +/* The maximum number of PKA shims referred to as IO blocks. */
> +#define MLXBF_PKA_MAX_NUM_IO_BLOCKS 24
> +/* The maximum number of rings supported by the IO block (shim). */
> +#define MLXBF_PKA_MAX_NUM_IO_BLOCK_RINGS 4
> +
> +#define MLXBF_PKA_MAX_NUM_RINGS (MLXBF_PKA_MAX_NUM_IO_BLOCK_RINGS * MLXBF_PKA_MAX_NUM_IO_BLOCKS)
> +/*
> + * Resources are regions which include info control/status words, count registers and host window
> + * RAM.
> + */
> +#define MLXBF_PKA_MAX_NUM_RING_RESOURCES 3
> +
> +/*
> + * PKA Ring resources.
> + * Define Ring resources parameters including base address, size (in bytes) and ring spacing.
> + */
> +#define MLXBF_PKA_RING_WORDS_ADDR MLXBF_PKA_BUFFER_RAM_BASE
> +#define MLXBF_PKA_RING_CNTRS_ADDR MLXBF_PKA_COMMAND_COUNT_0_ADDR
> +
> +#define MLXBF_PKA_RING_WORDS_SIZE SZ_64
> +#define MLXBF_PKA_RING_CNTRS_SIZE SZ_32
> +#define MLXBF_PKA_RING_MEM_SIZE SZ_16K
> +
> +#define MLXBF_PKA_RING_WORDS_SPACING SZ_64
> +#define MLXBF_PKA_RING_CNTRS_SPACING SZ_64K
> +#define MLXBF_PKA_RING_MEM_0_SPACING SZ_16K
> +#define MLXBF_PKA_RING_MEM_1_SPACING SZ_64K
> +
> +/*
> + * PKA Window RAM parameters.
> + * Define whether to split window RAM during PKA device creation phase.
> + */
> +#define MLXBF_PKA_SPLIT_WINDOW_RAM_MODE 0
> +
> +/* Defines for window RAM partition, valid for 16K memory. */
> +#define MLXBF_PKA_WINDOW_RAM_RING_MEM_SIZE SZ_2K
> +#define MLXBF_PKA_WINDOW_RAM_RING_MEM_DIV 2 /* Divide into halves. */
> +#define MLXBF_PKA_WINDOW_RAM_DATA_MEM_SIZE 0x3800 /* 14KB. */
> +#define MLXBF_PKA_WINDOW_RAM_RING_ADDR_MASK 0xFFFF
> +#define MLXBF_PKA_WINDOW_RAM_RING_SIZE_MASK 0xF0000
GENMASK() x2
> +#define MLXBF_PKA_WINDOW_RAM_RING_SIZE_SHIFT 2
Now I'm lost what this is about, you've (xx & 0xF0000) >> 2 in the code
when using MLXBF_PKA_WINDOW_RAM_RING_SIZE_* which looks weird black
magic.
> +
> +/* Window RAM/Alternate window RAM offset mask for BF1 and BF2. */
> +#define MLXBF_PKA_WINDOW_RAM_OFFSET_MASK1 0x730000
GENMASK()
> +
> +/* Window RAM/Alternate window RAM offset mask for BF3. */
> +#define MLXBF_PKA_WINDOW_RAM_OFFSET_MASK2 0x70000
GENMASK()
Why isn't BF1 & BF3 in the name of the define?
> +
> +/*
> + * PKA Master Sequencer Control/Status Register.
> + * Writing '1' to bit [31] puts the Master controller Sequencer in a reset state. Resetting the
> + * Sequencer (in order to load other firmware) should only be done when the EIP-154 is not
> + * performing any operations.
> + */
> +#define MLXBF_PKA_MASTER_SEQ_CTRL_RESET_VAL BIT(31)
Missing include.
> +/*
> + * Writing '1' to bit [30] will reset all Command and Result counters. This bit is write-only and
> + * self clearing and can only be set if the 'Reset' bit [31] is '1'.
> + */
> +#define MLXBF_PKA_MASTER_SEQ_CTRL_CLEAR_COUNTERS_VAL BIT(30)
> +/*
> + * MLXBF_PKA_RING_OPTIONS field to specify the priority in which rings are handled:
> + * '00' = full rotating priority,
> + * '01' = fixed priority (ring 0 lowest),
> + * '10' = ring 0 has the highest priority and the remaining rings have rotating priority,
> + * '11' = reserved, do not use.
> + */
> +#define MLXBF_PKA_RING_OPTIONS_PRIORITY 0x0
The comment is extremely odd given the define. 'priority' means 'full
rotation priority' and not e.g. 'fixed priority'?!? :-/
> +
> +/*
> + * 'Signature' byte used because the ring options are transferred through RAM which does not have a
> + * defined reset value. The EIP-154 master controller keeps reading the MLXBF_PKA_RING_OPTIONS word
> + * at start-up until the 'Signature' byte contains 0x46 and the 'Reserved' field contains zero.
> + */
> +#define MLXBF_PKA_RING_OPTIONS_SIGNATURE_BYTE 0x46
> +
> +/*
> + * Order of the result reporting: two schemas are available:
> + * InOrder - the results will be reported in the same order as the commands were provided.
> + * OutOfOrder - the results are reported as soon as they are available.
> + * Note: only the OutOfOrder schema is used in this implementation.
> + */
> +#define MLXBF_PKA_RING_TYPE_OUT_OF_ORDER_BIT 0
> +#define MLXBF_PKA_RING_TYPE_IN_ORDER MLXBF_PKA_RING_TYPE_OUT_OF_ORDER_BIT
> +
> +/*
> + * Byte order of the data written/read to/from Rings.
> + * Little Endian (LE) - the least significant bytes have the lowest address.
> + * Big Endian (BE) - the most significant bytes come first.
> + * Note: only the little endian is used in this implementation.
> + */
> +#define MLXBF_PKA_RING_BYTE_ORDER_LE 0
> +#define MLXBF_PKA_RING_BYTE_ORDER MLXBF_PKA_RING_BYTE_ORDER_LE
Both seem unused.
> +/*
> + * 'trng_clk_on' mask for PKA Clock Switch Forcing Register. Turn on the TRNG clock. When the TRNG
> + * is controlled via the host slave interface, this engine needs to be turned on by setting bit 11.
> + */
> +#define MLXBF_PKA_CLK_FORCE_TRNG_ON 0x800
BIT() ?
> +
> +/* Number of TRNG output registers. */
> +#define MLXBF_PKA_TRNG_OUTPUT_CNT 4
> +
> +/* Number of TRNG poker test counts. */
> +#define MLXBF_PKA_TRNG_POKER_TEST_CNT 4
> +
> +/* TRNG configuration. */
> +#define MLXBF_PKA_TRNG_CONFIG_REG_VAL 0x00020008
> +/* TRNG Alarm Counter Register value. */
> +#define MLXBF_PKA_TRNG_ALARMCNT_REG_VAL 0x000200FF
> +/* TRNG FRO Enable Register value. */
> +#define MLXBF_PKA_TRNG_FROENABLE_REG_VAL 0x00FFFFFF
IMO, "_REG_VAL" suffixes just makes your code lines longer and adds little
value. The same goes for _MASK btw.
> +/*
> + * TRNG Control Register value. Set bit 10 to start the EIP-76 (i.e. TRNG engine), gathering entropy
> + * from the FROs.
What FRO stands for? Perhaps it would be possible to have it mentioned in
the changelog with the opened form.
> + */
> +#define MLXBF_PKA_TRNG_CONTROL_REG_VAL 0x00000400
> +
> +/* TRNG Control bit. */
> +#define MLXBF_PKA_TRNG_CONTROL_TEST_MODE 0x100
> +
> +/*
> + * TRNG Control Register value. Set bit 10 and 12 to start the EIP-76 (i.e. TRNG engine) with DRBG
> + * enabled, gathering entropy from the FROs.
> + */
> +#define MLXBF_PKA_TRNG_CONTROL_DRBG_REG_VAL 0x00001400
> +
> +/*
> + * DRBG enabled TRNG 'request_data' value. REQ_DATA_VAL (in accordance with DATA_BLOCK_MASK)
> + * requests 256 blocks of 128-bit random output. 4095 blocks is the maximum number that can be
> + * requested for the TRNG (with DRBG) configuration on Bluefield platforms.
> + */
> +#define MLXBF_PKA_TRNG_CONTROL_REQ_DATA_VAL 0x10010000
Maybe drop _VAL ?
> +
> +/* Mask for 'Data Block' in TRNG Control Register. */
> +#define MLXBF_PKA_TRNG_DRBG_DATA_BLOCK_MASK 0xfff00000
> +
> +/* Set bit 12 of TRNG Control Register to enable DRBG functionality. */
> +#define MLXBF_PKA_TRNG_CONTROL_DRBG_ENABLE_VAL BIT(12)
> +
> +/* Set bit 7 (i.e. 'test_sp_800_90 DRBG' bit) in the TRNG Test Register. */
> +#define MLXBF_PKA_TRNG_TEST_DRBG_VAL BIT(7)
> +
> +/* Number of Personalization String/Additional Input Registers. */
> +#define MLXBF_PKA_TRNG_PS_AI_REG_COUNT 12
> +
> +/* Offset bytes of Personalization String/Additional Input Registers. */
> +#define MLXBF_PKA_TRNG_OUTPUT_REG_OFFSET 0x8
> +
> +/* Maximum TRNG test error cycle, about one second. */
> +#define MLXBF_PKA_TRNG_TEST_ERR_CYCLE_MAX (1000 * 1000 * 1000)
> +
> +/* DRBG Reseed enable. */
> +#define MLXBF_PKA_TRNG_CONTROL_DRBG_RESEED BIT(15)
> +
> +/* TRNG Status bits. */
> +#define MLXBF_PKA_TRNG_STATUS_READY BIT(0)
> +#define MLXBF_PKA_TRNG_STATUS_SHUTDOWN_OFLO BIT(1)
> +#define MLXBF_PKA_TRNG_STATUS_TEST_READY BIT(8)
> +#define MLXBF_PKA_TRNG_STATUS_MONOBIT_FAIL BIT(7)
> +#define MLXBF_PKA_TRNG_STATUS_RUN_FAIL BIT(4)
> +#define MLXBF_PKA_TRNG_STATUS_POKER_FAIL BIT(6)
For any group of defines, please align the values with tabs
> +
> +/* TRNG Alarm Counter bits. */
> +#define MLXBF_PKA_TRNG_ALARMCNT_STALL_RUN_POKER BIT(15)
> +
> +/* TRNG Test bits. */
> +#define MLXBF_PKA_TRNG_TEST_KNOWN_NOISE BIT(5)
> +#define MLXBF_PKA_TRNG_TEST_NOISE BIT(13)
> +
> +/* TRNG Test constants*/
> +#define MLXBF_PKA_TRNG_MONOBITCNT_SUM 9978
> +
> +#define MLXBF_PKA_TRNG_TEST_HALF_ADD 1
> +#define MLXBF_PKA_TRNG_TEST_HALF_NO 0
> +
> +#define MLXBF_PKA_TRNG_TEST_DATAL_BASIC_1 0x11111333
> +#define MLXBF_PKA_TRNG_TEST_DATAH_BASIC_1 0x3555779f
> +#define MLXBF_PKA_TRNG_TEST_COUNT_BASIC_1 11
> +
> +#define MLXBF_PKA_TRNG_TEST_DATAL_BASIC_2 0x01234567
> +#define MLXBF_PKA_TRNG_TEST_DATAH_BASIC_2 0x89abcdef
> +#define MLXBF_PKA_TRNG_TEST_COUNT_BASIC_2 302
> +
> +#define MLXBF_PKA_TRNG_TEST_DATAL_POKER 0xffffffff
> +#define MLXBF_PKA_TRNG_TEST_DATAH_POKER 0xffffffff
> +#define MLXBF_PKA_TRNG_TEST_COUNT_POKER 11
> +
> +#define MLXBF_PKA_TRNG_NUM_OF_FOUR_WORD 128
> +
> +/* PKA device related definitions. */
> +#define MLXBF_PKA_DEVFS_RING_DEVICES "mlxbf_pka/%d"
> +
> +/* Device resource structure. */
> +struct mlxbf_pka_dev_res_t {
> + void __iomem *ioaddr; /* The (iore)mapped version of addr, driver internal use. */
> + u64 base; /* Base address of the device's resource. */
> + u64 size; /* Size of IO. */
> + u8 type; /* Type of resource addr points to. */
> + s8 status; /* Status of the resource. */
> + char *name; /* Name of the resource. */
Please put the per field comments into kerneldoc.
> +};
> +
> +/* Defines for mlxbf_pka_dev_res->type. */
> +#define MLXBF_PKA_DEV_RES_TYPE_MEM 1 /* Resource type is memory. */
> +#define MLXBF_PKA_DEV_RES_TYPE_REG 2 /* Resource type is register. */
> +
> +/* Defines for mlxbf_pka_dev_res->status. */
> +#define MLXBF_PKA_DEV_RES_STATUS_MAPPED 1 /* The resource is (iore)mapped. */
> +#define MLXBF_PKA_DEV_RES_STATUS_UNMAPPED -1 /* The resource is unmapped. */
> +
> +/* PKA Ring resources structure. */
> +struct mlxbf_pka_dev_ring_res_t {
> + struct mlxbf_pka_dev_res_t info_words; /* Ring information words. */
> + struct mlxbf_pka_dev_res_t counters; /* Ring counters. */
> + struct mlxbf_pka_dev_res_t window_ram; /* Window RAM. */
> +};
> +
> +/* PKA Ring structure. */
> +struct mlxbf_pka_dev_ring_t {
> + u32 ring_id; /* Ring identifier. */
> + struct mlxbf_pka_dev_shim_s *shim; /* Pointer to the shim associated to the ring. */
> + u32 resources_num; /* Number of ring resources. */
> + struct mlxbf_pka_dev_ring_res_t resources; /* Ring resources. */
> + struct mlxbf_pka_dev_hw_ring_info_t *ring_info; /* Ring information. */
> + u32 num_cmd_desc; /* Number of command descriptors. */
> + s8 status; /* Status of the ring. */
> + struct mutex mutex; /* Mutex lock for sharing ring device. */
Ditto. Please do this for all structs here.
> +};
> +
> +/* Defines for mlxbf_pka_dev_ring->status. */
> +#define MLXBF_PKA_DEV_RING_STATUS_UNDEFINED -1
> +#define MLXBF_PKA_DEV_RING_STATUS_INITIALIZED 1
> +#define MLXBF_PKA_DEV_RING_STATUS_READY 2
> +#define MLXBF_PKA_DEV_RING_STATUS_BUSY 3
> +
> +/* PKA Shim resources structure. */
> +struct mlxbf_pka_dev_shim_res_t {
> + struct mlxbf_pka_dev_res_t buffer_ram; /* Buffer RAM. */
> + struct mlxbf_pka_dev_res_t master_seq_ctrl; /* Master sequencer controller CSR. */
> + struct mlxbf_pka_dev_res_t aic_csr; /* Interrupt controller CSRs. */
> + struct mlxbf_pka_dev_res_t trng_csr; /* TRNG module CSRs. */
> +};
> +
> +/* Number of PKA device resources. */
> +#define MLXBF_PKA_DEV_SHIM_RES_CNT 6
> +
> +/* Platform global shim resource information. */
> +struct mlxbf_pka_dev_gbl_shim_res_info_t {
> + struct mlxbf_pka_dev_res_t *res_tbl[MLXBF_PKA_DEV_SHIM_RES_CNT];
> + u8 res_cnt;
> +};
> +
> +struct mlxbf_pka_dev_mem_res {
> + u64 eip154_base; /* Base address for EIP154 mmio registers. */
> + u64 eip154_size; /* EIP154 mmio register region size. */
> +
> + u64 wndw_ram_off_mask; /* Common offset mask for alt window RAM and window RAM. */
> + u64 wndw_ram_base; /* Base address for window RAM. */
> + u64 wndw_ram_size; /* Window RAM region size. */
> +
> + u64 alt_wndw_ram_0_base; /* Base address for alternate window RAM 0. */
> + u64 alt_wndw_ram_1_base; /* Base address for alternate window RAM 1. */
> + u64 alt_wndw_ram_2_base; /* Base address for alternate window RAM 2. */
> + u64 alt_wndw_ram_3_base; /* Base address for alternate window RAM 3. */
> + u64 alt_wndw_ram_size; /* Alternate window RAM regions size. */
> +
> + u64 csr_base; /* Base address for CSR registers. */
> + u64 csr_size; /* CSR area size. */
> +};
> +
> +/* PKA Shim structure. */
> +struct mlxbf_pka_dev_shim_s {
> + struct mlxbf_pka_dev_mem_res mem_res;
> + u64 trng_err_cycle; /* TRNG error cycle. */
> + u32 shim_id; /* Shim identifier. */
> + u32 rings_num; /* Number of supported rings (hardware specific). */
> + struct mlxbf_pka_dev_ring_t **rings; /* Pointer to rings which belong to the shim. */
> + u8 ring_priority; /* Specify the priority in which rings are handled. */
> + u8 ring_type; /*Indicates whether the result ring delivers results strictly in-order. */
> + struct mlxbf_pka_dev_shim_res_t resources; /* Shim resources. */
> + u8 window_ram_split; /* If non-zero, the split window RAM scheme is used. */
> + u32 busy_ring_num; /* Number of active rings (rings in busy state). */
> + u8 trng_enabled; /* Whether the TRNG engine is enabled. */
> + s8 status; /* Status of the shim. */
> + struct mutex mutex; /* Mutex lock for sharing shim. */
> +};
> +
> +/* Defines for mlxbf_pka_dev_shim->status. */
> +#define MLXBF_PKA_SHIM_STATUS_UNDEFINED -1
> +#define MLXBF_PKA_SHIM_STATUS_CREATED 1
> +#define MLXBF_PKA_SHIM_STATUS_INITIALIZED 2
> +#define MLXBF_PKA_SHIM_STATUS_RUNNING 3
> +#define MLXBF_PKA_SHIM_STATUS_STOPPED 4
> +#define MLXBF_PKA_SHIM_STATUS_FINALIZED 5
> +
> +/* Defines for mlxbf_pka_dev_shim->window_ram_split. */
> +
> +/* Window RAM is split into 4 * 16KB blocks. */
> +#define MLXBF_PKA_SHIM_WINDOW_RAM_SPLIT_ENABLED 1
> +/* Window RAM is not split and occupies 64KB. */
> +#define MLXBF_PKA_SHIM_WINDOW_RAM_SPLIT_DISABLED 2
> +
> +/* Defines for mlxbf_pka_dev_shim->trng_enabled. */
> +#define MLXBF_PKA_SHIM_TRNG_ENABLED 1
> +#define MLXBF_PKA_SHIM_TRNG_DISABLED 0
> +
> +/* Platform global configuration structure. */
> +struct mlxbf_pka_dev_gbl_config_t {
> + u32 dev_shims_cnt; /* Number of registered PKA shims. */
> + u32 dev_rings_cnt; /* Number of registered Rings. */
> +
> + /* Table of registered PKA shims. */
> + struct mlxbf_pka_dev_shim_s *dev_shims[MLXBF_PKA_MAX_NUM_IO_BLOCKS];
> +
> + /* Table of registered Rings. */
> + struct mlxbf_pka_dev_ring_t *dev_rings[MLXBF_PKA_MAX_NUM_RINGS];
> +};
> +
> +extern struct mlxbf_pka_dev_gbl_config_t mlxbf_pka_gbl_config;
> +
> +/* Processor speed in hertz, used in routines which might be called very early in boot. */
> +static inline u64 mlxbf_pka_early_cpu_speed(void)
> +{
> + /*
> + * Initial guess at our CPU speed. We set this to be larger than any possible real speed,
> + * so that any calculated delays will be too long, rather than too short.
> + *
> + * CPU Freq for High/Bin Chip - 1.255GHz.
> + */
> + return 1255 * 1000 * 1000;
Please use linux/units.h instead of literal 1000s.
> +}
> +
> +/*
> + * Ring getter for mlxbf_pka_dev_gbl_config_t structure which holds all system global configuration.
> + * This configuration is shared and common to kernel device driver associated with PKA hardware.
> + */
> +struct mlxbf_pka_dev_ring_t *mlxbf_pka_dev_get_ring(u32 ring_id);
> +
> +/*
> + * Shim getter for mlxbf_pka_dev_gbl_config_t structure which holds all system global configuration.
> + * This configuration is shared and common to kernel device driver associated with PKA hardware.
> + */
> +struct mlxbf_pka_dev_shim_s *mlxbf_pka_dev_get_shim(u32 shim_id);
> +
> +/*
> + * Register a ring. This function initializes a Ring and configures its related resources, and
> + * returns a pointer to that ring.
> + */
> +struct mlxbf_pka_dev_ring_t *mlxbf_pka_dev_register_ring(struct device *dev,
> + u32 ring_id,
> + u32 shim_id);
> +
> +/* Unregister a Ring. */
> +int mlxbf_pka_dev_unregister_ring(struct mlxbf_pka_dev_ring_t *ring);
> +
> +/*
> + * Register PKA IO block. This function initializes a shim and configures its related resources, and
> + * returns a pointer to that ring.
> + */
> +struct mlxbf_pka_dev_shim_s *mlxbf_pka_dev_register_shim(struct device *dev,
> + u32 shim_id,
> + struct mlxbf_pka_dev_mem_res *mem_res);
> +
> +/* Unregister PKA IO block. */
> +int mlxbf_pka_dev_unregister_shim(struct mlxbf_pka_dev_shim_s *shim);
> +
> +/* Reset a Ring. */
> +int mlxbf_pka_dev_reset_ring(struct mlxbf_pka_dev_ring_t *ring);
> +
> +/*
> + * Clear ring counters. This function resets the master sequencer controller to clear the command
> + * and result counters.
> + */
> +int mlxbf_pka_dev_clear_ring_counters(struct mlxbf_pka_dev_ring_t *ring);
> +
> +/*
> + * Read data from the TRNG. Drivers can fill up to 'cnt' bytes of data into the buffer 'data'. The
> + * buffer 'data' is aligned for any type and 'cnt' is a multiple of 4.
> + */
> +int mlxbf_pka_dev_trng_read(struct mlxbf_pka_dev_shim_s *shim, u32 *data, u32 cnt);
> +
> +/* Return true if the TRNG engine is enabled, false if not. */
> +bool mlxbf_pka_dev_has_trng(struct mlxbf_pka_dev_shim_s *shim);
> +
> +/*
> + * Open the file descriptor associated with ring. It returns an integer value, which is used to
> + * refer to the file. If not successful, it returns a negative error.
> + */
> +int mlxbf_pka_dev_open_ring(struct mlxbf_pka_ring_info_t *ring_info);
> +
> +/*
> + * Close the file descriptor associated with ring. The function returns 0 if successful, negative
> + * value to indicate an error.
> + */
> +int mlxbf_pka_dev_close_ring(struct mlxbf_pka_ring_info_t *ring_info);
> +
> +#endif /* __MLXBF_PKA_DEV_H__ */
> diff --git a/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
> new file mode 100644
> index 000000000000..a8fe0ac1df78
> --- /dev/null
> +++ b/drivers/platform/mellanox/mlxbf_pka/mlxbf_pka_drv.c
> @@ -0,0 +1,1066 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause
> +// SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION. All rights reserved.
> +
> +#include <linux/acpi.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/hw_random.h>
> +#include <linux/interrupt.h>
> +#include <linux/iommu.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#include "mlxbf_pka_dev.h"
> +
> +#define MLXBF_PKA_DRIVER_DESCRIPTION "BlueField PKA driver"
> +
> +#define MLXBF_PKA_DEVICE_ACPIHID_BF1 "MLNXBF10"
> +#define MLXBF_PKA_RING_DEVICE_ACPIHID_BF1 "MLNXBF11"
> +
> +#define MLXBF_PKA_DEVICE_ACPIHID_BF2 "MLNXBF20"
> +#define MLXBF_PKA_RING_DEVICE_ACPIHID_BF2 "MLNXBF21"
> +
> +#define MLXBF_PKA_DEVICE_ACPIHID_BF3 "MLNXBF51"
> +#define MLXBF_PKA_RING_DEVICE_ACPIHID_BF3 "MLNXBF52"
> +
> +#define MLXBF_PKA_DEVICE_ACCESS_MODE 0666
> +
> +#define MLXBF_PKA_DEVICE_RES_CNT 7
> +
> +#define MLXBF_PKA_DEVICE_NAME_MAX 14
> +
> +enum mlxbf_pka_mem_res_idx {
> + MLXBF_PKA_ACPI_EIP154_IDX = 0,
> + MLXBF_PKA_ACPI_WNDW_RAM_IDX,
> + MLXBF_PKA_ACPI_ALT_WNDW_RAM_0_IDX,
> + MLXBF_PKA_ACPI_ALT_WNDW_RAM_1_IDX,
> + MLXBF_PKA_ACPI_ALT_WNDW_RAM_2_IDX,
> + MLXBF_PKA_ACPI_ALT_WNDW_RAM_3_IDX,
> + MLXBF_PKA_ACPI_CSR_IDX
> +};
> +
> +enum mlxbf_pka_plat_type {
> + MLXBF_PKA_PLAT_TYPE_BF1 = 0, /* Platform type Bluefield-1. */
> + MLXBF_PKA_PLAT_TYPE_BF2, /* Platform type Bluefield-2. */
> + MLXBF_PKA_PLAT_TYPE_BF3 /* Platform type Bluefield-3. */
> +};
These don't look necessary. Variations should be put into driver_data.
> +
> +static DEFINE_MUTEX(mlxbf_pka_drv_lock);
> +
> +static u32 mlxbf_pka_device_cnt;
> +static u32 mlxbf_pka_ring_device_cnt;
> +
> +static const char mlxbf_pka_acpihid_bf1[] = MLXBF_PKA_DEVICE_ACPIHID_BF1;
> +static const char mlxbf_pka_ring_acpihid_bf1[] = MLXBF_PKA_RING_DEVICE_ACPIHID_BF1;
> +
> +static const char mlxbf_pka_acpihid_bf2[] = MLXBF_PKA_DEVICE_ACPIHID_BF2;
> +static const char mlxbf_pka_ring_acpihid_bf2[] = MLXBF_PKA_RING_DEVICE_ACPIHID_BF2;
> +
> +static const char mlxbf_pka_acpihid_bf3[] = MLXBF_PKA_DEVICE_ACPIHID_BF3;
> +static const char mlxbf_pka_ring_acpihid_bf3[] = MLXBF_PKA_RING_DEVICE_ACPIHID_BF3;
> +
> +struct mlxbf_pka_drv_plat_info {
> + enum mlxbf_pka_plat_type type;
> +};
> +
> +static struct mlxbf_pka_drv_plat_info mlxbf_pka_drv_plat[] = {
> + [MLXBF_PKA_PLAT_TYPE_BF1] = {.type = MLXBF_PKA_PLAT_TYPE_BF1,},
> + [MLXBF_PKA_PLAT_TYPE_BF2] = {.type = MLXBF_PKA_PLAT_TYPE_BF2,},
> + [MLXBF_PKA_PLAT_TYPE_BF3] = {.type = MLXBF_PKA_PLAT_TYPE_BF3,}
Add spaces inside the braces.
Actually, now that I looked more into how it's used. The entire array
looks unnecessary, just make them independent:
static const struct mlxbf_pka_drv_plat_info mlxbf_pka_bf1_info = {
...
};
static const struct mlxbf_pka_drv_plat_info mlxbf_pka_bf2_info = {
...
> +};
> +
> +static const struct acpi_device_id mlxbf_pka_drv_acpi_ids[] = {
> + { MLXBF_PKA_DEVICE_ACPIHID_BF1,
> + (kernel_ulong_t)&mlxbf_pka_drv_plat[MLXBF_PKA_PLAT_TYPE_BF1],
> + 0,
> + 0 },
Put zeros on a single line.
> + { MLXBF_PKA_RING_DEVICE_ACPIHID_BF1, 0, 0, 0 },
> + { MLXBF_PKA_DEVICE_ACPIHID_BF2,
> + (kernel_ulong_t)&mlxbf_pka_drv_plat[MLXBF_PKA_PLAT_TYPE_BF2],
> + 0,
> + 0 },
> + { MLXBF_PKA_RING_DEVICE_ACPIHID_BF2, 0, 0, 0 },
> + { MLXBF_PKA_DEVICE_ACPIHID_BF3,
> + (kernel_ulong_t)&mlxbf_pka_drv_plat[MLXBF_PKA_PLAT_TYPE_BF3],
> + 0,
> + 0 },
> + { MLXBF_PKA_RING_DEVICE_ACPIHID_BF3, 0, 0, 0 },
> + {},
> +};
> +
> +static struct pka {
> + struct idr ring_idr;
> + struct mutex idr_lock; /* PKA ring device IDR lock mutex. */
> +} pka;
> +
> +struct mlxbf_pka_info {
> + struct device *dev; /* The device this info struct belongs to. */
> + const char *name; /* Device name. */
> + const char *acpihid;
> + u8 flag;
> + struct module *module;
> + void *priv; /* Optional private data. */
> +};
> +
> +/* Defines for mlxbf_pka_info->flags. */
> +#define MLXBF_PKA_DRIVER_FLAG_RING_DEVICE 1
> +#define MLXBF_PKA_DRIVER_FLAG_DEVICE 2
> +
> +struct mlxbf_pka_platdata {
> + struct platform_device *pdev;
> + struct mlxbf_pka_info *info;
> + spinlock_t lock; /* Generic spinlock. */
> + unsigned long irq_flags;
> +};
> +
> +/* Bits in mlxbf_pka_platdata.irq_flags. */
> +enum {
> + MLXBF_PKA_IRQ_DISABLED = 0,
> +};
> +
> +struct mlxbf_pka_ring_region {
> + u64 off;
> + u64 addr;
> + resource_size_t size;
> + u32 flags;
> + u32 type;
> +};
> +
> +/* Defines for mlxbf_pka_ring_region->flags. */
> +#define MLXBF_PKA_RING_REGION_FLAG_READ BIT(0) /* Region supports read. */
> +#define MLXBF_PKA_RING_REGION_FLAG_WRITE BIT(1) /* Region supports write. */
> +#define MLXBF_PKA_RING_REGION_FLAG_MMAP BIT(2) /* Region supports mmap. */
> +
> +/* Defines for mlxbf_pka_ring_region->type. */
> +#define MLXBF_PKA_RING_RES_TYPE_WORDS 1 /* Info control/status words. */
> +#define MLXBF_PKA_RING_RES_TYPE_CNTRS 2 /* Count registers. */
> +#define MLXBF_PKA_RING_RES_TYPE_MEM 4 /* Window RAM region. */
> +
> +#define MLXBF_PKA_DRIVER_RING_DEV_MAX MLXBF_PKA_MAX_NUM_RINGS
> +
> +struct mlxbf_pka_ring_device {
> + struct mlxbf_pka_info *info;
> + struct device *device;
> + u32 device_id;
> + u32 parent_device_id;
> + struct mutex mutex; /* PKA ring device mutex. */
> + struct mlxbf_pka_dev_ring_t *ring;
> + u32 num_regions;
> + struct mlxbf_pka_ring_region *regions;
> + struct miscdevice misc;
> +};
> +
> +#define MLXBF_PKA_DRIVER_DEV_MAX MLXBF_PKA_MAX_NUM_IO_BLOCKS
> +
> +/* Defines for region index. */
> +#define MLXBF_PKA_RING_REGION_WORDS_IDX 0
> +#define MLXBF_PKA_RING_REGION_CNTRS_IDX 1
> +#define MLXBF_PKA_RING_REGION_MEM_IDX 2
> +#define MLXBF_PKA_RING_REGION_OFFSET_SHIFT 40
> +#define MLXBF_PKA_RING_REGION_INDEX_TO_OFFSET(index) \
> + ((u64)(index) << MLXBF_PKA_RING_REGION_OFFSET_SHIFT)
Add the field as GENMASK(), use FIELD_PREP() and remove the _SHIFT define?
> +
> +struct mlxbf_pka_device {
> + struct mlxbf_pka_info *info;
> + struct device *device;
> + u32 device_id;
> + struct resource *resource[MLXBF_PKA_DEVICE_RES_CNT];
> + struct mlxbf_pka_dev_shim_s *shim;
> + long irq; /* Interrupt number. */
The comment is dead obvious and can be dropped.
Why is this long?
> + struct hwrng rng;
> +};
> +
> +/* Defines for mlxbf_pka_device->irq. */
> +#define MLXBF_PKA_IRQ_CUSTOM -1
> +#define MLXBF_PKA_IRQ_NONE 0
> +
> +/* Hardware interrupt handler. */
> +static irqreturn_t mlxbf_pka_drv_irq_handler(int irq, void *device)
> +{
> + struct mlxbf_pka_device *mlxbf_pka_dev = (struct mlxbf_pka_device *)device;
> + struct platform_device *pdev = to_platform_device(mlxbf_pka_dev->device);
> + struct mlxbf_pka_platdata *priv = platform_get_drvdata(pdev);
> +
> + dev_dbg(&pdev->dev, "handle irq in device\n");
> +
> + /* Just disable the interrupt in the interrupt controller. */
> + spin_lock(&priv->lock);
> + if (!__test_and_set_bit(MLXBF_PKA_IRQ_DISABLED, &priv->irq_flags))
> + disable_irq_nosync(irq);
> +
> + spin_unlock(&priv->lock);
> +
> + return IRQ_HANDLED;
This always returns IRQ_HANDLED, yet you've given IRQF_SHARED flag? If
the IRQ is shared, you must distinguish whether IRQ is for you or not, if
it's not for you, IRQ_NONE should be returned.
> +}
> +
> +static int mlxbf_pka_drv_register_irq(struct mlxbf_pka_device *mlxbf_pka_dev)
> +{
> + if (mlxbf_pka_dev->irq && mlxbf_pka_dev->irq != MLXBF_PKA_IRQ_CUSTOM) {
> + /* Allow sharing the irq among several devices (child devices so far). */
> + return request_irq(mlxbf_pka_dev->irq,
> + (irq_handler_t)mlxbf_pka_drv_irq_handler,
> + IRQF_SHARED, mlxbf_pka_dev->info->name,
> + mlxbf_pka_dev);
> + }
> +
> + return -ENXIO;
> +}
> +
> +static int mlxbf_pka_drv_ring_regions_init(struct mlxbf_pka_ring_device *ring_dev)
> +{
> + struct mlxbf_pka_ring_region *region;
> + struct mlxbf_pka_dev_ring_t *ring;
> + struct mlxbf_pka_dev_res_t *res;
> + u32 num_regions;
> +
> + ring = ring_dev->ring;
> + if (!ring || !ring->shim)
> + return -ENXIO;
> +
> + num_regions = ring->resources_num;
> + ring_dev->num_regions = num_regions;
> + ring_dev->regions = devm_kzalloc(ring_dev->device,
> + num_regions * sizeof(struct mlxbf_pka_ring_region),
devm_kcalloc()
> + GFP_KERNEL);
> + if (!ring_dev->regions)
> + return -ENOMEM;
> +
> + /* Information words region. */
> + res = &ring->resources.info_words;
> + region = &ring_dev->regions[MLXBF_PKA_RING_REGION_WORDS_IDX];
> + /* Map offset to the physical address. */
> + region->off = MLXBF_PKA_RING_REGION_INDEX_TO_OFFSET(MLXBF_PKA_RING_REGION_WORDS_IDX);
> + region->addr = res->base;
> + region->size = res->size;
> + region->type = MLXBF_PKA_RING_RES_TYPE_WORDS;
> + region->flags |= (MLXBF_PKA_RING_REGION_FLAG_MMAP |
> + MLXBF_PKA_RING_REGION_FLAG_READ |
> + MLXBF_PKA_RING_REGION_FLAG_WRITE);
> +
> + /* Counters registers region. */
> + res = &ring->resources.counters;
> + region = &ring_dev->regions[MLXBF_PKA_RING_REGION_CNTRS_IDX];
> + /* Map offset to the physical address. */
> + region->off = MLXBF_PKA_RING_REGION_INDEX_TO_OFFSET(MLXBF_PKA_RING_REGION_CNTRS_IDX);
> + region->addr = res->base;
> + region->size = res->size;
> + region->type = MLXBF_PKA_RING_RES_TYPE_CNTRS;
> + region->flags |= (MLXBF_PKA_RING_REGION_FLAG_MMAP |
> + MLXBF_PKA_RING_REGION_FLAG_READ |
> + MLXBF_PKA_RING_REGION_FLAG_WRITE);
Unnecessary parenthesis.
> +
> + /* Window RAM region. */
> + res = &ring->resources.window_ram;
> + region = &ring_dev->regions[MLXBF_PKA_RING_REGION_MEM_IDX];
> + /* Map offset to the physical address. */
> + region->off = MLXBF_PKA_RING_REGION_INDEX_TO_OFFSET(MLXBF_PKA_RING_REGION_MEM_IDX);
> + region->addr = res->base;
> + region->size = res->size;
> + region->type = MLXBF_PKA_RING_RES_TYPE_MEM;
> + region->flags |= (MLXBF_PKA_RING_REGION_FLAG_MMAP |
> + MLXBF_PKA_RING_REGION_FLAG_READ |
> + MLXBF_PKA_RING_REGION_FLAG_WRITE);
> +
> + return 0;
> +}
> +
> +static void mlxbf_pka_drv_ring_regions_cleanup(struct mlxbf_pka_ring_device *ring_dev)
> +{
> + /* Clear PKA ring device regions. */
> + ring_dev->num_regions = 0;
> +}
> +
> +static int mlxbf_pka_drv_ring_open(void *device_data)
> +{
> + struct mlxbf_pka_ring_device *ring_dev = device_data;
> + struct mlxbf_pka_info *info = ring_dev->info;
> + struct mlxbf_pka_ring_info_t ring_info;
> + int ret;
> +
> + dev_dbg(ring_dev->device, "open ring device (device_data:%p)\n", ring_dev);
> +
> + if (!try_module_get(info->module))
> + return -ENODEV;
> +
> + ring_info.ring_id = ring_dev->device_id;
> + ret = mlxbf_pka_dev_open_ring(&ring_info);
> + if (ret) {
> + dev_dbg(ring_dev->device, "failed to open ring\n");
> + module_put(info->module);
> + return ret;
> + }
> +
> + /* Initialize regions. */
> + ret = mlxbf_pka_drv_ring_regions_init(ring_dev);
> + if (ret) {
> + dev_dbg(ring_dev->device, "failed to initialize regions\n");
> + mlxbf_pka_dev_close_ring(&ring_info);
> + module_put(info->module);
Please convert this to proper rollback path.
> + return ret;
> + }
> +
> + return ret;
return 0; , followed by the rollback path.
> +}
> +
> +static void mlxbf_pka_drv_ring_release(void *device_data)
> +{
> + struct mlxbf_pka_ring_device *ring_dev = device_data;
> + struct mlxbf_pka_info *info = ring_dev->info;
> + struct mlxbf_pka_ring_info_t ring_info;
> + int ret;
> +
> + dev_dbg(ring_dev->device, "release ring device (device_data:%p)\n", ring_dev);
> +
> + mlxbf_pka_drv_ring_regions_cleanup(ring_dev);
> +
> + ring_info.ring_id = ring_dev->device_id;
> + ret = mlxbf_pka_dev_close_ring(&ring_info);
> + if (ret)
> + dev_dbg(ring_dev->device, "failed to close ring\n");
> +
> + module_put(info->module);
> +}
> +
> +static int mlxbf_pka_drv_ring_mmap_region(struct mlxbf_pka_ring_region region,
> + struct vm_area_struct *vma)
> +{
> + u64 req_len, pgoff, req_start;
> +
> + req_len = vma->vm_end - vma->vm_start;
> + pgoff = vma->vm_pgoff & ((1U << (MLXBF_PKA_RING_REGION_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> + req_start = pgoff << PAGE_SHIFT;
> +
> + region.size = roundup(region.size, PAGE_SIZE);
> +
> + if (req_start + req_len > region.size)
> + return -EINVAL;
> +
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
> +
> + return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, req_len, vma->vm_page_prot);
> +}
> +
> +static int mlxbf_pka_drv_ring_mmap(void *device_data, struct vm_area_struct *vma)
> +{
> + struct mlxbf_pka_ring_device *ring_dev = device_data;
> + struct mlxbf_pka_ring_region *region;
> + unsigned int index;
> +
> + dev_dbg(ring_dev->device, "mmap device\n");
> +
> + index = vma->vm_pgoff >> (MLXBF_PKA_RING_REGION_OFFSET_SHIFT - PAGE_SHIFT);
> +
> + if (vma->vm_end < vma->vm_start)
> + return -EINVAL;
> + if (!(vma->vm_flags & VM_SHARED))
> + return -EINVAL;
> + if (index >= ring_dev->num_regions)
> + return -EINVAL;
> + if (vma->vm_start & ~PAGE_MASK)
> + return -EINVAL;
> + if (vma->vm_end & ~PAGE_MASK)
> + return -EINVAL;
> +
> + region = &ring_dev->regions[index];
> +
> + if (!(region->flags & MLXBF_PKA_RING_REGION_FLAG_MMAP))
> + return -EINVAL;
> +
> + if (!(region->flags & MLXBF_PKA_RING_REGION_FLAG_READ) && (vma->vm_flags & VM_READ))
> + return -EINVAL;
> +
> + if (!(region->flags & MLXBF_PKA_RING_REGION_FLAG_WRITE) && (vma->vm_flags & VM_WRITE))
> + return -EINVAL;
> +
> + vma->vm_private_data = ring_dev;
> +
> + if (region->type & MLXBF_PKA_RING_RES_TYPE_CNTRS ||
> + region->type & MLXBF_PKA_RING_RES_TYPE_MEM)
> + return mlxbf_pka_drv_ring_mmap_region(ring_dev->regions[index], vma);
> +
> + if (region->type & MLXBF_PKA_RING_RES_TYPE_WORDS)
> + /* Currently user space is not allowed to access this region. */
> + return -EINVAL;
> +
> + return -EINVAL;
> +}
> +
> +static long mlxbf_pka_drv_ring_ioctl(void *device_data, unsigned int cmd, unsigned long arg)
> +{
> + struct mlxbf_pka_ring_device *ring_dev = device_data;
> +
> + if (cmd == MLXBF_PKA_RING_GET_REGION_INFO) {
> + struct mlxbf_pka_dev_region_info_t info;
> +
> + info.mem_index = MLXBF_PKA_RING_REGION_MEM_IDX;
> + info.mem_offset = ring_dev->regions[info.mem_index].off;
> + info.mem_size = ring_dev->regions[info.mem_index].size;
> +
> + info.reg_index = MLXBF_PKA_RING_REGION_CNTRS_IDX;
> + info.reg_offset = ring_dev->regions[info.reg_index].off;
> + info.reg_size = ring_dev->regions[info.reg_index].size;
> +
> + return copy_to_user((void __user *)arg, &info, sizeof(info)) ? -EFAULT : 0;
> +
> + } else if (cmd == MLXBF_PKA_GET_RING_INFO) {
> + struct mlxbf_pka_dev_hw_ring_info_t *this_ring_info;
> + struct mlxbf_pka_dev_hw_ring_info_t hw_ring_info;
> +
> + this_ring_info = ring_dev->ring->ring_info;
> +
> + hw_ring_info.cmmd_base = this_ring_info->cmmd_base;
> + hw_ring_info.rslt_base = this_ring_info->rslt_base;
> + hw_ring_info.size = this_ring_info->size;
> + hw_ring_info.host_desc_size = this_ring_info->host_desc_size;
> + hw_ring_info.in_order = this_ring_info->in_order;
> + hw_ring_info.cmmd_rd_ptr = this_ring_info->cmmd_rd_ptr;
> + hw_ring_info.rslt_wr_ptr = this_ring_info->rslt_wr_ptr;
> + hw_ring_info.cmmd_rd_stats = this_ring_info->cmmd_rd_ptr;
> + hw_ring_info.rslt_wr_stats = this_ring_info->rslt_wr_stats;
> +
> + return copy_to_user((void __user *)arg,
> + &hw_ring_info,
> + sizeof(hw_ring_info)) ? -EFAULT : 0;
Please split this return statement to smaller components.
> + } else if (cmd == MLXBF_PKA_CLEAR_RING_COUNTERS) {
> + return mlxbf_pka_dev_clear_ring_counters(ring_dev->ring);
> + } else if (cmd == MLXBF_PKA_GET_RANDOM_BYTES) {
> + struct mlxbf_pka_dev_trng_info_t trng_data;
> + struct mlxbf_pka_dev_shim_s *shim;
> + bool trng_present;
> + u32 byte_cnt;
> + u32 *data;
> + int ret;
> +
> + shim = ring_dev->ring->shim;
> + ret = copy_from_user(&trng_data,
> + (void __user *)(arg),
> + sizeof(struct mlxbf_pka_dev_trng_info_t));
> + if (ret) {
> + dev_dbg(ring_dev->device, "failed to copy user request.\n");
> + return -EFAULT;
> + }
> +
> + /*
> + * Need byte count which is multiple of 4 as required by the
> + * mlxbf_pka_dev_trng_read() interface.
> + */
> + byte_cnt = round_up(trng_data.count, MLXBF_PKA_TRNG_OUTPUT_CNT);
> +
> + data = kzalloc(byte_cnt, GFP_KERNEL);
Use __free() to handle the release of data.
> + if (!data)
> + return -ENOMEM;
> +
> + trng_present = mlxbf_pka_dev_has_trng(shim);
> + if (!trng_present) {
> + kfree(data);
> + return -EAGAIN;
> + }
> +
> + ret = mlxbf_pka_dev_trng_read(shim, data, byte_cnt);
> + if (ret) {
> + dev_dbg(ring_dev->device, "TRNG failed %d\n", ret);
> + kfree(data);
> + return ret;
> + }
> +
> + ret = copy_to_user((void __user *)(trng_data.data), data, trng_data.count);
> + kfree(data);
> + return ret ? -EFAULT : 0;
> + }
> +
> + return -ENOTTY;
> +}
> +
> +static int mlxbf_pka_drv_open(struct inode *inode, struct file *filep)
> +{
> + struct mlxbf_pka_ring_device *ring_dev;
> + int ret;
> +
> + mutex_lock(&pka.idr_lock);
> + ring_dev = idr_find(&pka.ring_idr, iminor(inode));
> + mutex_unlock(&pka.idr_lock);
> + if (!ring_dev) {
> + pr_err("mlxbf_pka error: failed to find idr for device\n");
> + return -ENODEV;
> + }
> +
> + ret = mlxbf_pka_drv_ring_open(ring_dev);
> + if (ret)
> + return ret;
> +
> + filep->private_data = ring_dev;
> + return ret;
> +}
> +
> +static int mlxbf_pka_drv_release(struct inode *inode, struct file *filep)
> +{
> + struct mlxbf_pka_ring_device *ring_dev = filep->private_data;
> +
> + filep->private_data = NULL;
> + mlxbf_pka_drv_ring_release(ring_dev);
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_drv_mmap(struct file *filep, struct vm_area_struct *vma)
> +{
> + return mlxbf_pka_drv_ring_mmap(filep->private_data, vma);
> +}
> +
> +static long mlxbf_pka_drv_unlocked_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> +{
> + return mlxbf_pka_drv_ring_ioctl(filep->private_data, cmd, arg);
> +}
> +
> +static const struct file_operations mlxbf_pka_ring_fops = {
> + .owner = THIS_MODULE,
> + .open = mlxbf_pka_drv_open,
> + .release = mlxbf_pka_drv_release,
> + .unlocked_ioctl = mlxbf_pka_drv_unlocked_ioctl,
> + .mmap = mlxbf_pka_drv_mmap,
> +};
> +
> +static int mlxbf_pka_drv_add_ring_device(struct mlxbf_pka_ring_device *ring_dev)
> +{
> + struct device *dev = ring_dev->device;
> + char name[MLXBF_PKA_DEVICE_NAME_MAX];
> + int minor_number;
> + int ret;
> +
> + ret = snprintf(name, sizeof(name), MLXBF_PKA_DEVFS_RING_DEVICES, ring_dev->device_id);
scnprintf().
> + if (ret < 0) {
> + dev_err(dev, " -- snprintf failed: ret=%d\n", ret);
No you don't want to print this.
And I don't think the s*printf() even return errors so the entire check
is pointless.
> + return ret;
> + }
> +
> + ring_dev->misc.minor = MISC_DYNAMIC_MINOR;
> + ring_dev->misc.name = &name[0];
> + ring_dev->misc.mode = MLXBF_PKA_DEVICE_ACCESS_MODE;
> + ring_dev->misc.fops = &mlxbf_pka_ring_fops;
> +
> + ret = misc_register(&ring_dev->misc);
> + if (ret) {
> + dev_err(dev, " -- misc_register failed: ret=%d\n", ret);
Please try to avoid C code references in user visible error message, write
the explanation of the problem in English instead.
> + return ret;
> + }
> +
> + mutex_lock(&pka.idr_lock);
> + minor_number = idr_alloc(&pka.ring_idr,
> + ring_dev,
> + ring_dev->misc.minor,
> + MINORMASK + 1,
> + GFP_KERNEL);
Use less lines.
Missing #include ?
> + mutex_unlock(&pka.idr_lock);
> + if (minor_number != ring_dev->misc.minor) {
Why you're not checking idr_alloc()'s return value as per the usual < 0 ??
Are you using idr only for reverse mapping these?
> + dev_err(dev, " -- idr_alloc failed with minor number %d\n", ring_dev->misc.minor);
Remove "--".
> + return ring_dev->misc.minor;
> + }
> +
> + dev_info(dev, "ring device minor:%d\n", ring_dev->misc.minor);
dev_dbg() at most, if at all.
> +
> + return ret;
> +}
> +
> +static struct mlxbf_pka_ring_device *mlxbf_pka_drv_del_ring_device(struct device *dev)
> +{
> + struct platform_device *pdev = container_of(dev, struct platform_device, dev);
> + struct mlxbf_pka_platdata *priv = platform_get_drvdata(pdev);
> + struct mlxbf_pka_info *info = priv->info;
> + struct mlxbf_pka_ring_device *ring_dev = info->priv;
> +
> + if (ring_dev) {
> + mutex_lock(&pka.idr_lock);
> + idr_remove(&pka.ring_idr, ring_dev->misc.minor);
> + mutex_unlock(&pka.idr_lock);
> + misc_deregister(&ring_dev->misc);
> + }
> +
> + return ring_dev;
> +}
> +
> +static void mlxbf_pka_drv_get_mem_res(struct mlxbf_pka_device *mlxbf_pka_dev,
> + struct mlxbf_pka_dev_mem_res *mem_res,
> + u64 wndw_ram_off_mask)
> +{
> + enum mlxbf_pka_mem_res_idx acpi_mem_idx;
> +
> + acpi_mem_idx = MLXBF_PKA_ACPI_EIP154_IDX;
> + mem_res->wndw_ram_off_mask = wndw_ram_off_mask;
> +
> + /* PKA EIP154 MMIO base address. */
> + mem_res->eip154_base = mlxbf_pka_dev->resource[acpi_mem_idx]->start;
> + mem_res->eip154_size = mlxbf_pka_dev->resource[acpi_mem_idx]->end -
> + mem_res->eip154_base + 1;
resource_size(), please change all of them.
> + acpi_mem_idx++;
> +
> + /* PKA window RAM base address. */
> + mem_res->wndw_ram_base = mlxbf_pka_dev->resource[acpi_mem_idx]->start;
> + mem_res->wndw_ram_size = mlxbf_pka_dev->resource[acpi_mem_idx]->end -
> + mem_res->wndw_ram_base + 1;
> + acpi_mem_idx++;
> +
> + /*
> + * PKA alternate window RAM base address.
> + * Note: the size of all the alt window RAM is the same, depicted by 'alt_wndw_ram_size'
> + * variable. All alt window RAM resources are read here even though not all of them are used
> + * currently.
> + */
> + mem_res->alt_wndw_ram_0_base = mlxbf_pka_dev->resource[acpi_mem_idx]->start;
> + mem_res->alt_wndw_ram_size = mlxbf_pka_dev->resource[acpi_mem_idx]->end -
> + mem_res->alt_wndw_ram_0_base + 1;
> +
> + if (mem_res->alt_wndw_ram_size != MLXBF_PKA_WINDOW_RAM_REGION_SIZE)
> + dev_err(mlxbf_pka_dev->device, "alternate Window RAM size from ACPI is wrong.\n");
Should this return error and result in failing to register the device?
> + acpi_mem_idx++;
> +
> + mem_res->alt_wndw_ram_1_base = mlxbf_pka_dev->resource[acpi_mem_idx]->start;
> + acpi_mem_idx++;
> +
> + mem_res->alt_wndw_ram_2_base = mlxbf_pka_dev->resource[acpi_mem_idx]->start;
> + acpi_mem_idx++;
> +
> + mem_res->alt_wndw_ram_3_base = mlxbf_pka_dev->resource[acpi_mem_idx]->start;
> + acpi_mem_idx++;
> +
> + /* PKA CSR base address. */
> + mem_res->csr_base = mlxbf_pka_dev->resource[acpi_mem_idx]->start;
> + mem_res->csr_size = mlxbf_pka_dev->resource[acpi_mem_idx]->end - mem_res->csr_base + 1;
> +}
> +
> +/*
> + * Note: this function must be serialized because it calls 'mlxbf_pka_dev_register_shim' which
Please assert the condition for serialization with lockdep so it is
properly enforced.
> + * manipulates common counters for the PKA devices.
> + */
> +static int mlxbf_pka_drv_register_device(struct mlxbf_pka_device *mlxbf_pka_dev,
> + u64 wndw_ram_off_mask)
> +{
> + struct mlxbf_pka_dev_mem_res mem_res;
> + u32 mlxbf_pka_shim_id;
> +
> + /* Register shim. */
Obvious comment.
> + mlxbf_pka_shim_id = mlxbf_pka_dev->device_id;
> +
> + mlxbf_pka_drv_get_mem_res(mlxbf_pka_dev, &mem_res, wndw_ram_off_mask);
> +
> + mlxbf_pka_dev->shim = mlxbf_pka_dev_register_shim(mlxbf_pka_dev->device,
> + mlxbf_pka_shim_id,
> + &mem_res);
> + if (!mlxbf_pka_dev->shim) {
> + dev_dbg(mlxbf_pka_dev->device, "failed to register shim\n");
> + return -EFAULT;
No, please shadow errnos like this (-EFAULT is wrong errno anyway, at
least most of the error I can see in that call chain). Change
mlxbf_pka_dev_register_shim() to return correct errors and pass it on
here. You need to fix the entire call chain as some functions
mlxbf_pka_dev_register_shim() calls to also return errnos which it doesn't
properly propagate here.
> + }
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_drv_unregister_device(struct mlxbf_pka_device *mlxbf_pka_dev)
> +{
> + if (!mlxbf_pka_dev)
> + return -EINVAL;
> +
> + if (mlxbf_pka_dev->shim) {
Can this be false as register side returned error?
If it can (which I'm skeptical about), reverse the logic and return early
instead.
> + dev_dbg(mlxbf_pka_dev->device, "unregister device shim\n");
> + return mlxbf_pka_dev_unregister_shim(mlxbf_pka_dev->shim);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Note: this function must be serialized because it calls 'mlxbf_pka_dev_register_ring' which
> + * manipulates common counters for the PKA ring devices.
> + */
> +static int mlxbf_pka_drv_register_ring_device(struct mlxbf_pka_ring_device *ring_dev)
> +{
> + u32 shim_id = ring_dev->parent_device_id;
> + u32 ring_id = ring_dev->device_id;
> +
> + ring_dev->ring = mlxbf_pka_dev_register_ring(ring_dev->device, ring_id, shim_id);
> + if (!ring_dev->ring) {
> + dev_dbg(ring_dev->device, "failed to register ring device\n");
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_drv_unregister_ring_device(struct mlxbf_pka_ring_device *ring_dev)
> +{
> + if (!ring_dev)
> + return -EINVAL;
> +
> + if (ring_dev->ring) {
> + dev_dbg(ring_dev->device, "unregister ring device\n");
> + return mlxbf_pka_dev_unregister_ring(ring_dev->ring);
> + }
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_drv_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> +{
> + struct mlxbf_pka_device *mlxbf_pka_dev = container_of(rng, struct mlxbf_pka_device, rng);
> + u32 *buffer = data;
> + int ret;
> +
> + ret = mlxbf_pka_dev_trng_read(mlxbf_pka_dev->shim, buffer, max);
> + if (ret) {
> + dev_dbg(mlxbf_pka_dev->device,
> + "%s: failed to read random bytes ret=%d",
> + rng->name, ret);
> + return 0;
> + }
> +
> + return max;
> +}
> +
> +static int mlxbf_pka_drv_probe_device(struct mlxbf_pka_info *info)
> +{
> + struct mlxbf_pka_drv_plat_info *plat_info;
> + enum mlxbf_pka_mem_res_idx acpi_mem_idx;
> + struct mlxbf_pka_device *mlxbf_pka_dev;
> + const struct acpi_device_id *aid;
> + const char *bootup_status;
> + u64 wndw_ram_off_mask;
> + struct hwrng *trng;
> + int ret;
> +
Please don't lave empty lines into variable declaration block.
> + struct device *dev = info->dev;
> + struct device_node *of_node = dev->of_node;
> + struct platform_device *pdev = to_platform_device(dev);
> +
> + if (!info)
Can this happen? Besides, you already dereferenced it above.
> + return -EINVAL;
> +
> + mlxbf_pka_dev = devm_kzalloc(dev, sizeof(*mlxbf_pka_dev), GFP_KERNEL);
> + if (!mlxbf_pka_dev)
> + return -ENOMEM;
> +
> + mutex_lock(&mlxbf_pka_drv_lock);
scoped_guard()
> + mlxbf_pka_device_cnt += 1;
> + if (mlxbf_pka_device_cnt > MLXBF_PKA_DRIVER_DEV_MAX) {
> + dev_dbg(dev, "cannot support %u devices\n", mlxbf_pka_device_cnt);
> + mutex_unlock(&mlxbf_pka_drv_lock);
> + return -EPERM;
Wrong error code.
> + }
> + mlxbf_pka_dev->device_id = mlxbf_pka_device_cnt - 1;
> + mutex_unlock(&mlxbf_pka_drv_lock);
> +
> + mlxbf_pka_dev->info = info;
> + mlxbf_pka_dev->device = dev;
> + info->flag = MLXBF_PKA_DRIVER_FLAG_DEVICE;
> +
> + for (acpi_mem_idx = MLXBF_PKA_ACPI_EIP154_IDX;
> + acpi_mem_idx < MLXBF_PKA_DEVICE_RES_CNT; acpi_mem_idx++) {
Misaligned.
> + mlxbf_pka_dev->resource[acpi_mem_idx] = platform_get_resource(pdev,
> + IORESOURCE_MEM,
> + acpi_mem_idx);
> + }
> +
> + /* Verify PKA bootup status. */
> + ret = device_property_read_string(dev, "bootup_done", &bootup_status);
> + if (ret < 0 || strcmp(bootup_status, "true")) {
For ret < 0 case, you should return the original error code.
> + dev_err(dev, "device bootup required\n");
> + return -EPERM;
Likely wrong error code.
> + }
> +
> + /* Window RAM offset mask is platform dependent. */
> + aid = acpi_match_device(mlxbf_pka_drv_acpi_ids, dev);
> + if (!aid)
> + return -ENODEV;
> +
> + plat_info = (struct mlxbf_pka_drv_plat_info *)aid->driver_data;
> + if (plat_info->type <= MLXBF_PKA_PLAT_TYPE_BF2) {
> + wndw_ram_off_mask = MLXBF_PKA_WINDOW_RAM_OFFSET_MASK1;
> + } else if (plat_info->type <= MLXBF_PKA_PLAT_TYPE_BF3) {
> + wndw_ram_off_mask = MLXBF_PKA_WINDOW_RAM_OFFSET_MASK2;
Put this mask into a field in driver_data.
> + } else {
Can this happen?
> + dev_err(dev, "invalid platform type: %d\n", (int)plat_info->type);
> + return -EINVAL;
> + }
> +
> + /* Set interrupts. */
> + ret = platform_get_irq(pdev, 0);
> + mlxbf_pka_dev->irq = ret;
Assign directly to mlxbf_pka_dev->irq ?
> + if (ret == -ENXIO && of_node) {
> + mlxbf_pka_dev->irq = MLXBF_PKA_IRQ_NONE;
> + } else if (ret < 0) {
> + dev_err(dev, "failed to get device IRQ\n");
> + return ret;
> + }
> +
> + /* Register IRQ. */
> + ret = mlxbf_pka_drv_register_irq(mlxbf_pka_dev);
> + if (ret) {
> + dev_err(dev, "failed to register device IRQ\n");
> + return ret;
> + }
> +
> + mutex_lock(&mlxbf_pka_drv_lock);
scoped_guard()
> + ret = mlxbf_pka_drv_register_device(mlxbf_pka_dev, wndw_ram_off_mask);
> + if (ret) {
> + dev_dbg(dev, "failed to register shim\n");
> + mutex_unlock(&mlxbf_pka_drv_lock);
> + return ret;
> + }
> + mutex_unlock(&mlxbf_pka_drv_lock);
> +
> + /* Setup the TRNG if needed. */
> + if (mlxbf_pka_dev_has_trng(mlxbf_pka_dev->shim)) {
> + trng = &mlxbf_pka_dev->rng;
> + trng->name = pdev->name;
> + trng->read = mlxbf_pka_drv_rng_read;
> +
> + ret = hwrng_register(&mlxbf_pka_dev->rng);
> + if (ret) {
> + dev_err(dev, "failed to register trng\n");
> + return ret;
> + }
> + }
> +
> + info->priv = mlxbf_pka_dev;
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_drv_remove_device(struct platform_device *pdev)
> +{
> + struct mlxbf_pka_platdata *priv = platform_get_drvdata(pdev);
> + struct mlxbf_pka_info *info = priv->info;
> + struct mlxbf_pka_device *mlxbf_pka_dev = (struct mlxbf_pka_device *)info->priv;
> +
> + if (!mlxbf_pka_dev) {
> + pr_err("mlxbf_pka error: failed to unregister device\n");
Remove.
> + return -EINVAL;
> + }
> +
> + if (mlxbf_pka_dev_has_trng(mlxbf_pka_dev->shim))
> + hwrng_unregister(&mlxbf_pka_dev->rng);
> +
> + if (mlxbf_pka_drv_unregister_device(mlxbf_pka_dev))
> + dev_err(&pdev->dev, "failed to unregister device\n");
Yeah, just as I thought, nothing useful can be done on this error. Just
remove the error print and convert the unregister chain into void.
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_drv_probe_ring_device(struct mlxbf_pka_info *info)
> +{
> + struct mlxbf_pka_ring_device *ring_dev;
> + struct device *dev = info->dev;
> + const char *bootup_status;
> + int ret;
> +
> + if (!info)
> + return -EINVAL;
> +
> + ring_dev = devm_kzalloc(dev, sizeof(*ring_dev), GFP_KERNEL);
> + if (!ring_dev)
> + return -ENOMEM;
> +
> + if (!mlxbf_pka_ring_device_cnt) {
> + mutex_init(&pka.idr_lock);
What protects these two lines? I don't see any locks here or
lockdep asserts.
devm_mutex_init() + error handling.
> + mutex_lock(&pka.idr_lock);
> + /* Only initialize IDR if there is no ring device registered. */
> + idr_init(&pka.ring_idr);
> + mutex_unlock(&pka.idr_lock);
> + }
> +
> + mutex_lock(&mlxbf_pka_drv_lock);
scoped_guard()
> + mlxbf_pka_ring_device_cnt += 1;
> + if (mlxbf_pka_ring_device_cnt > MLXBF_PKA_DRIVER_RING_DEV_MAX) {
> + dev_dbg(dev, "cannot support %u ring devices\n", mlxbf_pka_ring_device_cnt);
> + mutex_unlock(&mlxbf_pka_drv_lock);
> + return -EPERM;
Wrong error code.
> + }
> + ring_dev->device_id = mlxbf_pka_ring_device_cnt - 1;
> + ring_dev->parent_device_id = mlxbf_pka_device_cnt - 1;
> + mutex_unlock(&mlxbf_pka_drv_lock);
> +
> + ring_dev->info = info;
> + ring_dev->device = dev;
> + info->flag = MLXBF_PKA_DRIVER_FLAG_RING_DEVICE;
> + mutex_init(&ring_dev->mutex);
> +
> + /* Verify PKA bootup status. */
> + ret = device_property_read_string(dev, "bootup_done", &bootup_status);
> + if (ret < 0 || strcmp(bootup_status, "true")) {
> + dev_err(dev, "device bootup required\n");
> + return -EPERM;
Duplicated code should be moved into a helper (I didn't check the full
extent of it but I got a dejavu here).
> + }
> +
> + mutex_lock(&mlxbf_pka_drv_lock);
scoped_guard() ?
> + /* Add PKA ring device. */
> + ret = mlxbf_pka_drv_add_ring_device(ring_dev);
> + if (ret) {
> + dev_dbg(dev, "failed to add ring device %u\n", ring_dev->device_id);
> + mutex_unlock(&mlxbf_pka_drv_lock);
> + return ret;
> + }
> +
> + /* Register PKA ring device. */
> + ret = mlxbf_pka_drv_register_ring_device(ring_dev);
> + if (ret) {
> + dev_dbg(dev, "failed to register ring device\n");
> + mutex_unlock(&mlxbf_pka_drv_lock);
> + goto err_register_ring;
> + }
> + mutex_unlock(&mlxbf_pka_drv_lock);
> +
> + info->priv = ring_dev;
> +
> + return 0;
> +
> + err_register_ring:
> + mlxbf_pka_drv_del_ring_device(dev);
> + return ret;
> +}
> +
> +static int mlxbf_pka_drv_remove_ring_device(struct platform_device *pdev)
> +{
> + struct mlxbf_pka_ring_device *ring_dev;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + ring_dev = mlxbf_pka_drv_del_ring_device(dev);
> + if (ring_dev) {
> + ret = mlxbf_pka_drv_unregister_ring_device(ring_dev);
> + if (ret) {
> + dev_err(dev, "failed to unregister pka device\n");
> + return ret;
> + }
> + mlxbf_pka_ring_device_cnt--;
> + }
> +
> + if (!mlxbf_pka_ring_device_cnt) {
> + mutex_lock(&pka.idr_lock);
> + /* Only destroy IDR if there is no ring device registered. */
> + idr_destroy(&pka.ring_idr);
> + mutex_unlock(&pka.idr_lock);
> + }
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_drv_acpi_probe(struct platform_device *pdev, struct mlxbf_pka_info *info)
> +{
> + struct device *dev = &pdev->dev;
> + struct acpi_device *adev;
> + int ret;
> +
> + if (acpi_disabled)
> + return -ENOENT;
> +
> + adev = ACPI_COMPANION(dev);
> + if (!adev) {
> + dev_dbg(dev, "ACPI companion device not found for %s\n", pdev->name);
> + return -ENODEV;
> + }
> +
> + info->acpihid = acpi_device_hid(adev);
> + if (WARN_ON(!info->acpihid))
> + return -EINVAL;
> +
> + if (!strcmp(info->acpihid, mlxbf_pka_ring_acpihid_bf1) ||
> + !strcmp(info->acpihid, mlxbf_pka_ring_acpihid_bf2) ||
> + !strcmp(info->acpihid, mlxbf_pka_ring_acpihid_bf3)) {
> + ret = mlxbf_pka_drv_probe_ring_device(info);
> + if (ret) {
> + dev_dbg(dev, "failed to register ring device\n");
> + return ret;
> + }
> + dev_dbg(dev, "ring device probed\n");
> +
> + } else if (!strcmp(info->acpihid, mlxbf_pka_acpihid_bf1) ||
> + !strcmp(info->acpihid, mlxbf_pka_acpihid_bf2) ||
> + !strcmp(info->acpihid, mlxbf_pka_acpihid_bf3)) {
Split these two types to use own probe functions so you don't need to
duplicate matching here like this.
> + ret = mlxbf_pka_drv_probe_device(info);
> + if (ret) {
> + dev_dbg(dev, "failed to register device\n");
> + return ret;
> + }
> + dev_info(dev, "device probed\n");
> + }
> +
> + return 0;
> +}
> +
> +static int mlxbf_pka_drv_probe(struct platform_device *pdev)
> +{
> + struct mlxbf_pka_platdata *priv;
> + struct device *dev = &pdev->dev;
> + struct mlxbf_pka_info *info;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + spin_lock_init(&priv->lock);
> + priv->pdev = pdev;
> + /* Interrupt is disabled to begin with. */
> + priv->irq_flags = 0;
> +
> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> +
> + info->name = pdev->name;
> + info->module = THIS_MODULE;
> + info->dev = dev;
> + priv->info = info;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + ret = mlxbf_pka_drv_acpi_probe(pdev, info);
> + if (ret) {
> + dev_dbg(dev, "unknown device\n");
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static void mlxbf_pka_drv_remove(struct platform_device *pdev)
> +{
> + struct mlxbf_pka_platdata *priv = platform_get_drvdata(pdev);
> + struct mlxbf_pka_info *info = priv->info;
> +
> + if (info->flag == MLXBF_PKA_DRIVER_FLAG_RING_DEVICE) {
> + dev_info(&pdev->dev, "remove ring device\n");
> + if (mlxbf_pka_drv_remove_ring_device(pdev))
> + dev_dbg(&pdev->dev, "failed to remove ring device\n");
> + }
> +
> + if (info->flag == MLXBF_PKA_DRIVER_FLAG_DEVICE) {
> + dev_info(&pdev->dev, "remove PKA device\n");
> + if (mlxbf_pka_drv_remove_device(pdev))
> + dev_dbg(&pdev->dev, "failed to remove PKA device\n");
> + }
> +}
> +
> +MODULE_DEVICE_TABLE(acpi, mlxbf_pka_drv_acpi_ids);
> +
> +static struct platform_driver mlxbf_pka_drv = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .acpi_match_table = ACPI_PTR(mlxbf_pka_drv_acpi_ids),
> + },
> + .probe = mlxbf_pka_drv_probe,
> + .remove = mlxbf_pka_drv_remove,
> +};
> +
> +module_platform_driver(mlxbf_pka_drv);
> +MODULE_DESCRIPTION(MLXBF_PKA_DRIVER_DESCRIPTION);
> +MODULE_AUTHOR("Ron Li <xiangrongl@xxxxxxxxxx>");
> +MODULE_AUTHOR("Khalil Blaiech <kblaiech@xxxxxxxxxx>");
> +MODULE_AUTHOR("Mahantesh Salimath <mahantesh@xxxxxxxxxx>");
> +MODULE_AUTHOR("Shih-Yi Chen <shihyic@xxxxxxxxxx>");
> +MODULE_LICENSE("Dual BSD/GPL");
>
This is extremely long and it would make reviewing it significantly easier
if it would be split into multiple, logical patches. Mixing two types of
devices in a single probe adds to the confusion, can like one of those be
introduced first and then the other?
--
i.
