Re: [PATCH v2 6/9] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] i3c: mipi-i3c-hci: NULL deref and missing offset check in
  hci_extcap_vendor_ASPEED()
- [Medium] i3c: mipi-i3c-hci: ACK_L programmed with hcnt instead of lcnt
  in aspeed_i3c_phy_init()
--

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

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

This commit adds vendor capability parsing, resource management, interrupt
summary handling, PHY timing setup, and DAA index configuration for the
ASPEED AST2700 I3C controller.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/aspeed.c b/drivers/i3c/master/mipi-i3c-hci/aspeed.c
> new file mode 100644
> index 0000000000000..3aa910e786f8e
> --- /dev/null
> +++ b/drivers/i3c/master/mipi-i3c-hci/aspeed.c
> @@ -0,0 +1,351 @@

[ ... ]

> +int hci_extcap_vendor_ASPEED(struct i3c_hci *hci, void __iomem *base)
> +{
> +	struct aspeed_i3c_vendor_data *vd = to_aspeed_vendor_data(hci);
> +
> +	vd->inhouse_regs = hci->base_regs + readl(base + 0x04);
> +	vd->phy_regs = hci->base_regs + readl(base + 0x08);
> +
> +	return 0;
> +}
> +
> +bool aspeed_i3c_vendor_cap_found(struct i3c_hci *hci)
> +{
> +	struct aspeed_i3c_vendor_data *vd = to_aspeed_vendor_data(hci);
> +
> +	return vd->inhouse_regs && vd->phy_regs;
> +}

[Severity: Medium]
Can hci_extcap_vendor_ASPEED() dereference a NULL vd pointer or populate
non-NULL register pointers from zero offsets?

EXT_CAP_VENDOR(ASPEED, 0xc0, 0x08) is registered unconditionally in
vendor_ext_caps[], whereas hci->vendor_data is only allocated in
i3c_hci_probe() when HCI_QUIRK_VENDOR_RESOURCES is set.  If an ASPEED
controller is bound without HCI_QUIRK_VENDOR_RESOURCES (for example via the
generic "mipi-i3c-hci" compatible), i3c_hci_parse_ext_caps() calls
hci_extcap_vendor_ASPEED() with hci->vendor_data == NULL and crashes.
Similarly, the other four vendor quirks (HCI_QUIRK_VENDOR_BUS_SETUP,
HCI_QUIRK_BOARD_PHY_TIMINGS, HCI_QUIRK_VENDOR_INTR_SUMMARY, and
HCI_QUIRK_VENDOR_DAA_INDEX) dereference hci->vendor_data without checking
that HCI_QUIRK_VENDOR_RESOURCES allocated it or verified
aspeed_i3c_vendor_cap_found().

In addition, unlike the section parsers in i3c_hci_init() that check
"offset ? hci->base_regs + offset : NULL", hci_extcap_vendor_ASPEED() adds
readl(base + 0x04) and readl(base + 0x08) directly to hci->base_regs.  A
zero offset in either register yields hci->base_regs rather than NULL, so
aspeed_i3c_vendor_cap_found() returns true and later vendor or PHY writes
overwrite the core HCI registers at hci->base_regs.

[ ... ]

> +	hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_SCL_H_NS, period_ns);
> +	lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_SCL_L_NS, period_ns);
> +	aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FM_CTRL1,
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL1_SCL_H, hcnt) |
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_PHY_I2C_FM_CTRL1_SCL_L, lcnt));
> +	aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FM_CTRL2,
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL2_ACK_H, hcnt) |
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL2_ACK_L, hcnt));
> +
> +	hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_HD_DAT_NS, period_ns);
> +	lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_AHD_DAT_NS, period_ns);
> +	aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FM_CTRL3,
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL3_HD_DAT, hcnt) |
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL3_AHD_DAT, lcnt));
> +
> +	hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_CAS_NS, period_ns);
> +	lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_SU_STO_NS, period_ns);
> +	aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FMP_CTRL0,
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL0_CAS, hcnt) |
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL0_SU_STO, lcnt));
> +
> +	hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_SCL_H_NS, period_ns);
> +	lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_SCL_L_NS, period_ns);
> +	aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FMP_CTRL1,
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL1_SCL_H, hcnt) |
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL1_SCL_L, lcnt));
> +	aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FMP_CTRL2,
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL2_ACK_H, hcnt) |
> +			     FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL2_ACK_L, hcnt));

[Severity: Medium]
Should ASPEED_I3C_PHY_I2C_FM_CTRL2_ACK_L and
ASPEED_I3C_PHY_I2C_FMP_CTRL2_ACK_L be programmed with lcnt instead of hcnt,
matching ASPEED_I3C_PHY_I3C_OD_CTRL2_ACK_L further down?  Passing hcnt sets
the I2C ACK SCL low period to 1130 ns in Fast Mode (below the 1300 ns I2C
minimum tLOW) and 380 ns in Fast Mode Plus (below the 500 ns minimum).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-b4-i3c-hci-ast2700-v2-0-4d8fff5c5196@aspeedtech.com?part=6

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