Re: [RFC PATCH v1 1/8] misc/arm-cla: Add driver skeleton and documentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The EL2 specific memory translation context (MTC) registers are defined as absolute indices rather than relative offsets, breaking consistency with the common MTC offsets.
- [Low] Headers rely on implicit inclusions for data types and device structs.
--

--- Patch [1]: [RFC PATCH v1 1/8] misc/arm-cla: Add driver skeleton and documentation ---
commit 8a70e14ab5fcacc24591a3baaa6d296f8a8f4c6b
Author: Jean-Philippe Brucker <jpb@kernel.org>

misc/arm-cla: Add driver skeleton and documentation

This adds the initial Kconfig, build-system plumbing, and driver header
definitions for the Arm Core Local Accelerator driver. It also adds
documentation describing the CLA programming model and access rules.

> diff --git a/drivers/misc/arm-cla/arm-cla-regs.h b/drivers/misc/arm-cla/arm-cla-regs.h
> new file mode 100644
> index 0000000000000..fcd187ecb7771
> --- /dev/null
> +++ b/drivers/misc/arm-cla/arm-cla-regs.h
> @@ -0,0 +1,177 @@
[ ... ]
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +
> +/* Registers */
> +#define CLA_REG_DATA(i)			(0x00 + (8 * (i)))
[ ... ]
> +#define CLA_PLxCTRL_AVAIL		BIT(0)
> +#define CLA_PLxCTRL_DBGPERM		GENMASK(3, 1)
> +#define CLA_PLxCTRL_PREP(accid, v)	((u64)(v) << (8 * (accid)))

[Severity: Low]
Should linux/types.h be included in drivers/misc/arm-cla/arm-cla-regs.h?

The CLA_PLxCTRL_PREP macro casts the value to u64, but the header doesn't
include linux/types.h. This could lead to build failures if the header is
included directly in new source files without prior type inclusions.

[ ... ]
> +/* Common register offsets */
> +#define CLA_MTC_PSTATE			0
> +#define CLA_MTC_TTBR0			1
> +#define CLA_MTC_TTBR1			2
> +#define CLA_MTC_TCR			3
> +#define CLA_MTC_SCTLR			4
> +#define CLA_MTC_MAIR			5
> +#define CLA_MTC_TCR2			8
> +
> +/* EL2 specific register offsets */
> +#define CLA_MTC_HCR_EL2			80
> +#define CLA_MTC_VTTBR_EL2		81
> +#define CLA_MTC_VTCR_EL2		82

[Severity: Medium]
Are these EL2 specific register offsets intended to be relative offsets like the
common ones (0-8)?

They are currently defined as absolute indices (80, 81, 82). If the driver
applies these by adding the base index (e.g., CLA_MTC_REGIDX_PL2, which is 64)
to the offset, using CLA_MTC_HCR_EL2 (80) would yield an index of 144. This
would exceed the defined CLA_MTC_PL_SIZE (64) and target the wrong hardware
address space.

> diff --git a/drivers/misc/arm-cla/arm-cla.h b/drivers/misc/arm-cla/arm-cla.h
> new file mode 100644
> index 0000000000000..f265d7b602686
> --- /dev/null
> +++ b/drivers/misc/arm-cla/arm-cla.h
> @@ -0,0 +1,38 @@
[ ... ]
> +#include <linux/types.h>
> +
> +#include "arm-cla-regs.h"
> +
> +/* Number of accelerators per CLA */
> +#define CLA_NUM_ACC		8
> +#define CLA_NUM_DATA_REGS	8
> +#define CLA_SRSTATE_LEN		8
> +
> +/**
> + * struct cla_dev - CLA device
> + *
> + * Immutable state:
> + * @cpu:		The CPU this CLA is attached to.
> + * @dev:		The platform device.
> + */
> +struct cla_dev {
> +	unsigned int cpu;
> +	struct device *dev;
> +};
> +
> +#define cla_dbg(dev, fmt, ...) \
> +	dev_dbg((dev)->dev, "[%u] " fmt, (dev)->cpu, ##__VA_ARGS__)

[Severity: Low]
Does this header need to include linux/device.h and linux/dev_printk.h?

The cla_dbg macro uses dev_dbg() and dereferences struct device. Relying on
implicit inclusions can cause build issues if arm-cla.h is included in
a different order or in new files.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717104759.123203-1-ryan.roberts@arm.com?part=1