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

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Nov 11, 2026 at 04:22:34AM +0000, sashiko-bot@kernel.org wrote:
> 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().

Please check this one

Frank

>
> 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