Re: [PATCH v10 2/2] i3c: master: Add driver for AMD AXI I3C master controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Use-After-Free due to using devm_kzalloc for a structure embedding a registered device.
- [High] Custom FIFO accessors incorrectly use host-endian memory access with BE I/O functions, causing data corruption on Little-Endian architectures.
- [Medium] Clock math rounding errors violate I3C/I2C timing requirements.
--

--- Patch [2]: [PATCH v10 2/2] i3c: master: Add driver for AMD AXI I3C master controller ---
commit 00474ccc3cfbb305b1555163333b3484dcd7f140
Author: Manikanta Guntupalli <manikanta.guntupalli@amd.com>

i3c: master: Add driver for AMD AXI I3C master controller

This commit introduces a new driver for the AMD AXI I3C master controller.
It supports Standard Data Rate (SDR) mode, Dynamic Address Assignment, private
transfers, and CCC transfers in both broadcast and direct modes, as well as
falling back to I2C mode.

> diff --git a/drivers/i3c/master/amd-i3c-master.c b/drivers/i3c/master/amd-i3c-master.c
> new file mode 100644
> index 0000000000000..ef5ad5abb788c
> --- /dev/null
> +++ b/drivers/i3c/master/amd-i3c-master.c
> @@ -0,0 +1,1124 @@
[ ... ]
> +static inline void xi3c_readsl_be(const void __iomem *addr, void *buffer,
> +				  unsigned int count)
> +{
> +	u32 *buf = buffer;
> +
> +	while (count--)
> +		put_unaligned(ioread32be(addr), buf++);
> +}

[Severity: High]
Can these custom stream accessors corrupt data on Little-Endian
architectures?

In xi3c_readsl_be(), ioread32be() returns a native 32-bit integer. When
put_unaligned() writes this back to memory on a Little-Endian system, it
writes it in Little-Endian order, effectively reversing the byte stream for
every 4-byte chunk.

[ ... ]
> +static inline void xi3c_readl_fifo(const void __iomem *addr, void *buf,
> +				   int nbytes)
> +{
> +	xi3c_readsl_be(addr, buf, nbytes / 4);
> +	if (nbytes & 3) {
> +		u32 tmp;
> +
> +		xi3c_readsl_be(addr, &tmp, 1);
> +		memcpy((u8 *)buf + (nbytes & ~3), &tmp, nbytes & 3);
> +	}
> +}

[Severity: High]
Will this remainder copy logic discard the payload on Little-Endian systems?

Because the remainder is read into a stack variable (tmp), the lowest memory
address of this 32-bit integer receives the zero-padding on Little-Endian
architectures. The memcpy() will then copy this zero-padding into the buffer
instead of the actual payload bytes.

[ ... ]
> +static int xi3c_clk_cfg(struct xi3c_master *master, unsigned long sclhz, u8 mode)
> +{
> +	unsigned long core_rate, core_periodns;
> +	u32 tcasmin, tsustart, tsustop, thdstart;
> +	u32 thigh, tlow, thold;
> +	u32 odthigh, odtlow;
> +
> +	core_rate = clk_get_rate(master->pclk);
> +	if (!core_rate)
> +		return -EINVAL;
> +
> +	if (!sclhz)
> +		return -EINVAL;
> +
> +	core_periodns = DIV_ROUND_UP(NSEC_PER_SEC, core_rate);
> +
> +	thigh = DIV_ROUND_UP(core_rate, sclhz) >> 1;
> +	tlow = thigh;

[Severity: Medium]
Does this clock calculation risk violating minimum I3C/I2C timing
requirements?

The DIV_ROUND_UP for core_periodns overestimates the period. When calculating
tcasmin later by dividing XI3C_TCASMIN_NS by core_periodns, this
overestimated period could result in fewer nanoseconds than required by the
specification.

Additionally, the right shift by 1 on the DIV_ROUND_UP of core_rate and sclhz
discards the remainder. If the quotient is odd, thigh and tlow will lose a
cycle, resulting in an overall bus frequency that exceeds the requested sclhz
limit.

[ ... ]
> +static int xi3c_master_probe(struct platform_device *pdev)
> +{
> +	struct xi3c_master *master;
> +	int ret;
> +
> +	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);

[Severity: Critical]
Is it safe to use devm_kzalloc for the master structure here?

The master structure embeds struct i3c_master_controller, which contains a
struct device registered with the driver core. During driver unbind, the
devres core will free this memory immediately after remove() returns.

However, userspace or another subsystem might still hold a reference to the
embedded device (for example, through an open sysfs file). If that happens,
it could trigger a use-after-free when the final reference is eventually
dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709064233.1451482-1-shubhamsanjay.patil@amd.com?part=2