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

[Jean-Philippe Brucker <jpb@kernel.org>]

On Fri, Jul 17, 2026 at 10:56:37AM +0000, sashiko-bot@kernel.org wrote:
> > +#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.

linux/types.h gets included, but 4 levels deep starting from
bitfields.h. I'll move the include from arm-cla.h here.


> > +/* 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.

The generic offsets are for both EL1 and EL2. These are differentiated
by their _EL2 suffix. Targetting the EL3 frame or beyond causes
cla_op_setctx() to fail loudly.


> > diff --git a/drivers/misc/arm-cla/arm-cla.h b/drivers/misc/arm-cla/arm-cla.h
> > +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.

We include device.h in a later patch. It includes dev_prink.h