Re: [PATCH 5/8] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Kernel crash due to unchecked vendor register access if the hardware lacks the ASPEED extended capability.
- [High] Incomplete clearing of previous DAA index across registers leaves stale bits set.
- [Low] Contradictory use of optional clock API leads to misleading probe failures.
--

commit 3d4e54b2ee6f05bbf18ce4fc75b881c1d3330487
Author: Billy Tsai <billy_tsai@aspeedtech.com>

i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller

This commit adds support for the AST2700 I3C controller by caching vendor
data through a MIPI extended capability parser. It implements ASPEED-specific
initialization hooks, hardware configurations like DAA indexing, and custom
IRQ dispatching.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> --- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
[ ... ]
> @@ -293,6 +294,24 @@ static void hci_cmd_v1_prep_i2c_xfer(struct i3c_hci *hci,
>  	}
>  }
>  
> +/*
> + * The DAA index registers name the DAT slot being assigned during ENTDAA;
> + * they live in the vendor capability block discovered at probe time.
> + */
> +static void aspeed_i3c_set_daa_index(struct i3c_hci *hci, unsigned int dat_idx)
> +{
> +	void __iomem *inhouse_regs = to_aspeed_vendor_data(hci)->inhouse_regs;
> +
> +	if (dat_idx < 32)
> +		writel(BIT(dat_idx), inhouse_regs + ASPEED_I3C_DAA_INDEX0);
> +	else if (dat_idx < 64)
> +		writel(BIT(dat_idx - 32), inhouse_regs + ASPEED_I3C_DAA_INDEX1);
> +	else if (dat_idx < 96)
> +		writel(BIT(dat_idx - 64), inhouse_regs + ASPEED_I3C_DAA_INDEX2);
> +	else
> +		writel(BIT(dat_idx - 96), inhouse_regs + ASPEED_I3C_DAA_INDEX3);

[Severity: High]
Does this leave stale bits set in the other register banks?

Writing to one register bank leaves the other three register banks untouched.
As devices are sequentially assigned to DAT slots during ENTDAA, if the index 
crosses a 32-bit boundary (e.g., from index 31 to 32), the old bit remains set
globally.

The hardware might interpret multiple set bits as instructions to assign
multiple DAT slots simultaneously, corrupting the HCI's internal routing.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
[ ... ]
> @@ -866,6 +944,11 @@ static int i3c_hci_reset_and_init(struct i3c_hci *hci)
>  	 * Bit 0:5 are defined in IP version < 0.8 but not handled by PIO code
>  	 */
>  	reg_write(INTR_STATUS_ENABLE, GENMASK(31, 10));
> +	if (is_aspeed(hci)) {
> +		aspeed_i3c_write(hci, ASPEED_I3C_INTR_SIGNAL_ENABLE, 0);

[Severity: High]
Can this cause a kernel panic due to a NULL pointer dereference or type
confusion?

The unconditional write relies solely on the DT compatible string through
is_aspeed() without verifying if the hardware capability was actually found and
parsed. If the DT indicates an ASPEED compatible device but the hardware does
not expose the ASPEED MIPI vendor extended capability (e.g. due to a mismatched
DT binding), i3c_hci_probe() allocates vd with a NULL inhouse_regs pointer.

Additionally, if the hardware is NXP, the NXP parser incorrectly overwrites
hci->vendor_data. Later, i3c_hci_reset_and_init() unconditionally executes
ASPEED-specific writes using to_aspeed_vendor_data(hci)->inhouse_regs,
causing a crash.

[ ... ]
> @@ -1189,6 +1289,40 @@ static int i3c_hci_probe(struct platform_device *pdev)
>  					     "cannot set DMA mask\n");
>  	}
>  
> +	if (is_aspeed(hci)) {
> +		struct aspeed_i3c_vendor_data *vd;
> +
> +		vd = devm_kzalloc(&pdev->dev, sizeof(*vd), GFP_KERNEL);
[ ... ]
> +		vd->dma_rst = devm_reset_control_get_optional_shared(&pdev->dev, "dma");
> +		if (IS_ERR(vd->dma_rst))
> +			return PTR_ERR(vd->dma_rst);
> +		if (vd->dma_rst) {
> +			ret = reset_control_deassert(vd->dma_rst);
> +			if (ret)
> +				return ret;
> +			ret = devm_add_action_or_reset(&pdev->dev,
> +						       i3c_hci_dma_rst_assert,
> +						       vd->dma_rst);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		vd->clk = devm_clk_get_optional(&pdev->dev, NULL);

[Severity: Low]
Is the core clock actually optional?

The clock is retrieved as optional here, which will return NULL if it is not
specified in the device tree (masking the -ENOENT failure). However, in
aspeed_i3c_phy_init(), it appears to be treated as a mandatory resource.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
> --- a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
[ ... ]
> +void aspeed_i3c_write(struct i3c_hci *hci, u32 reg, u32 val)
> +{
> +	writel(val, to_aspeed_vendor_data(hci)->inhouse_regs + reg);
> +}

[Severity: High]
Does this result in an invalid memory access or NULL pointer dereference?

If the ASPEED extended capability wasn't present in hardware (or if an NXP
device incorrectly overwrote hci->vendor_data), to_aspeed_vendor_data(hci)
might be NULL or point to invalid memory here, triggering a crash when called
unconditionally from i3c_hci_reset_and_init().

[ ... ]
> +int aspeed_i3c_phy_init(struct i3c_hci *hci)
> +{
> +	u16 hcnt, lcnt, total_cnt, min_tbit_cnt, cas_lcnt, cas_cnt, cbp_cnt;
> +	u32 sda_tx_hold = 0, cas_ns = 0, cbp_ns = 0, sr_p_low = 0;
> +	struct i3c_bus *bus = i3c_master_get_bus(&hci->master);
> +	u32 pp_high = 0, pp_low = 0, od_high = 0, od_low = 0;
> +	struct device_node *np = hci->master.dev.of_node;
> +	u32 sdr_ctrl0_reg, ctrl0, ctrl1, ctrl2;
> +	unsigned long core_rate, period_ns;
> +
> +	core_rate = clk_get_rate(to_aspeed_vendor_data(hci)->clk);
> +	if (!core_rate) {
> +		dev_err(&hci->master.dev, "invalid core clock rate\n");
> +		return -EINVAL;
> +	}

[Severity: Low]
Does this contradict the optional clock retrieval in i3c_hci_probe()?

If devm_clk_get_optional() returns NULL during probe, clk_get_rate() will
return 0 here.

The code hard-fails if the rate is 0, completely negating the optional
retrieval pattern. This hides the actual missing resource error, returning
a confusing failure message instead of properly propagating a missing DT
requirement.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-b4-i3c-hci-ast2700-v1-0-19909e7cbd7e@aspeedtech.com?part=5

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c