[PATCH v3 3/3] phy: airoha: Add support for Airoha AN7583 USB PHY
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Thu Aug 27 06:26:50 PDT 2026
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Off-by-one error in array bounds check allows out-of-bounds array write.
- [High] Incorrect device node is used to fetch the per-port serdes PHY.
- [High] Missing per-port register offset in `an7583_usb_phy_u2_set_mode` causes cross-instance state corruption.
- [Medium] Uninitialized variable `ret` is used as an error return code in `an7583_usb_phy_parse_ports`.
- [Medium] `fm_out` may be evaluated uninitialized if the regmap polling macro fails.
- [Medium] The PHY initialization error codes for USB3 and PCIe are silently ignored.
- [Medium] Nested acquisition of same-class PHY mutex triggers lockdep warning.
- [Medium] Excessive busy-waiting using mdelay() in PHY initialization loops.
- [Low] Missing `fallthrough;` annotation in switch statements triggers implicit fallthrough warnings.
--
commit e08eb2476fb864bd66e0895961c95bf0fbc89352
Author: Christian Marangi <ansuelsmth at gmail.com>
phy: airoha: Add support for Airoha AN7583 USB PHY
Adds support for USB PHY on Airoha AN7583 SoC, supporting two U2 ports and
one U3 port via a single USB PHY instance, plus a second instance for
PCIe Gen2.
> diff --git a/drivers/phy/airoha/phy-an7583-usb.c b/drivers/phy/airoha/phy-an7583-usb.c
> new file mode 100644
> index 0000000000000..33915c9f4a0de
> --- /dev/null
> +++ b/drivers/phy/airoha/phy-an7583-usb.c
> @@ -280,18 +280,18 @@ static int an7583_usb_phy_u2_slew_rate_calibration(struct phy *phy)
> /* Timeout can happen and we will apply workaround at the end */
> regmap_read_poll_timeout(priv->regs[AIROHA_USB_PHY_PHY],
> AIROHA_USB_PHY_FMMONR0 + offset,
> fm_out, fm_out, AIROHA_USB_PHY_FREQDET_SLEEP,
> AIROHA_USB_PHY_FREQDET_TIMEOUT);
>
> /* Disable Frequency meter */
> regmap_clear_bits(priv->regs[AIROHA_USB_PHY_PHY],
> AIROHA_USB_PHY_FMCR0 + offset,
> AIROHA_USB_PHY_FREQDET_EN);
>
> /* Disable Free run clock */
> regmap_clear_bits(priv->regs[AIROHA_USB_PHY_PHY],
> AIROHA_USB_PHY_FMMONR1 + offset,
> AIROHA_USB_PHY_FRCK_EN);
>
> /* Disable HS TX SR calibration */
> regmap_clear_bits(priv->regs[AIROHA_USB_PHY_PHY],
> AIROHA_USB_PHY_USBPHYACR5 + offset,
> AIROHA_USB_PHY_USB20_HSTX_SRCAL_EN);
>
> usleep_range(1000, 1500);
>
> /* Frequency was not detected, use default SR calibration value */
> if (!fm_out) {
[Severity: Medium]
If the regmap read returns an error here in
an7583_usb_phy_u2_slew_rate_calibration(), regmap_read_poll_timeout()
will terminate early without initializing fm_out.
Will this cause the subsequent check if (!fm_out) to evaluate garbage data?
[ ... ]
> @@ -340,11 +340,11 @@ static bool an7583_usb_phy_u3_kband_is_calibrated(struct an7583_usb_phy_priv *pr
> /* Read KBand */
> regmap_clear_bits(priv->regs[AIROHA_USB_PHY_ANA],
> AIROHA_USB_ANA_TDC_FT_CK_EN,
> AIROHA_USB_ANA_PLL_DEBUG_SEL);
>
> mdelay(5);
[Severity: Medium]
Can this delay in an7583_usb_phy_u3_kband_is_calibrated() cause excessive
CPU stalling?
It uses mdelay(5) twice per loop, and is called up to 50 times in
an7583_usb_phy_u3_init(). Because mdelay spins the CPU instead of sleeping,
this could freeze the CPU for up to 500ms during hardware initialization
failures.
Should this be using usleep_range() or msleep() instead?
[ ... ]
> @@ -1020,16 +1020,16 @@ static int an7583_usb_phy_init(struct phy *phy)
> struct an7583_usb_phy_instance *instance = phy_get_drvdata(phy);
>
> switch (instance->type) {
> case PHY_TYPE_USB2:
> an7583_usb_phy_u2_init(phy);
> break;
> case PHY_TYPE_USB3:
> if (phy_get_mode(phy) == PHY_MODE_ETHERNET)
> return 0;
>
> an7583_usb_phy_u3_init(phy);
> break;
> case PHY_TYPE_PCIE:
> if (phy_get_mode(phy) == PHY_MODE_ETHERNET)
> return 0;
>
> an7583_usb_phy_pcie_init(phy);
[Severity: Medium]
Both an7583_usb_phy_u3_init() and an7583_usb_phy_pcie_init() return an
error code (-EINVAL) when calibration fails, but those values are ignored
here.
Does this falsely notify the PHY framework of a successful initialization?
> break;
> default:
> return -EINVAL;
> }
[ ... ]
> @@ -1176,8 +1176,8 @@ static int an7583_usb_phy_u2_set_mode(struct phy *phy, enum phy_mode mode)
> case PHY_MODE_USB_HOST:
> val |= AIROHA_USB_PHY_FORCE_IDDIG;
> break;
> case PHY_MODE_USB_OTG:
> break;
> default:
> return 0;
> }
>
> regmap_update_bits(priv->regs[AIROHA_USB_PHY_PHY], AIROHA_USB_PHY_U2PHYDTM1,
[Severity: High]
Does this code in an7583_usb_phy_u2_set_mode() write to the base register
space of port 0 instead of the correct port?
Unlike other functions (e.g., power_on or init), this directly accesses
AIROHA_USB_PHY_U2PHYDTM1 without adding instance->offset. If mode
configuration is applied to a secondary port, it seems like this will corrupt
port 0's state.
> AIROHA_USB_PHY_FORCE_IDDIG |
> AIROHA_USB_PHY_IDDIG, val);
[ ... ]
> @@ -1193,12 +1193,12 @@ static int an7583_usb_phy_u3_set_mode(struct phy *phy, enum phy_mode mode,
> int submode)
> {
> struct an7583_usb_phy_instance *instance = phy_get_drvdata(phy);
> int ret;
>
> switch (mode) {
> case PHY_MODE_ETHERNET:
> if (submode != PHY_INTERFACE_MODE_SGMII &&
> submode != PHY_INTERFACE_MODE_1000BASEX &&
> submode != PHY_INTERFACE_MODE_2500BASEX)
> return -EOPNOTSUPP;
> case PHY_MODE_USB_DEVICE_SS:
[Severity: Low]
Is this an intentional fallthrough?
If so, this switch block needs a fallthrough annotation to prevent compiler
warnings.
> break;
> default:
> return -EOPNOTSUPP;
> }
>
> ret = phy_set_mode_ext(instance->serdes_phy, mode, submode);
[Severity: Medium]
Will this nested call to phy_set_mode_ext() in an7583_usb_phy_u3_set_mode()
trigger a lockdep warning?
The core PHY framework invokes the driver's set_mode callback with the
primary phy->mutex held. Calling phy_set_mode_ext() on the secondary PHY
attempts to acquire the secondary PHY's mutex, which belongs to the same
lock class.
[ ... ]
> @@ -1215,12 +1215,12 @@ static int an7583_usb_phy_pcie_set_mode(struct phy *phy, enum phy_mode mode,
> int submode)
> {
> struct an7583_usb_phy_instance *instance = phy_get_drvdata(phy);
> int ret;
>
> switch (mode) {
> case PHY_MODE_ETHERNET:
> if (submode != PHY_INTERFACE_MODE_SGMII &&
> submode != PHY_INTERFACE_MODE_1000BASEX &&
> submode != PHY_INTERFACE_MODE_2500BASEX)
> return -EOPNOTSUPP;
> case PHY_MODE_PCIE:
[Severity: Low]
Is this an intentional fallthrough?
If so, this switch block needs a fallthrough annotation to prevent compiler
warnings.
> break;
> default:
> return -EOPNOTSUPP;
> }
>
> ret = phy_set_mode_ext(instance->serdes_phy, mode, submode);
[Severity: Medium]
Will this nested call to phy_set_mode_ext() in an7583_usb_phy_pcie_set_mode()
trigger a lockdep warning?
The core PHY framework invokes the driver's set_mode callback with the
primary phy->mutex held. Calling phy_set_mode_ext() on the secondary PHY
attempts to acquire the secondary PHY's mutex, which belongs to the same
lock class.
[ ... ]
> @@ -1297,29 +1297,29 @@ static int an7583_usb_phy_parse_ports(struct device *dev,
> struct an7583_usb_phy_priv *priv,
> int index, struct device_node *subnode)
> {
> const struct an7583_usb_phy_match_data *data;
> struct an7583_usb_phy_instance *instance;
> struct phy *phy;
> int ret;
>
> data = of_device_get_match_data(dev);
>
> if (index > data->max_instance)
[Severity: High]
Should this bounds check use >= instead of > in an7583_usb_phy_parse_ports()?
The max_instance is the size of the array (3 for USB), so allowing index to
equal max_instance will cause an out-of-bounds write when priv->phys[index]
is assigned later.
> return -EINVAL;
>
> if (!of_device_is_available(subnode))
> return 0;
>
> instance = devm_kzalloc(dev, sizeof(*instance), GFP_KERNEL);
> if (!instance)
> return -ENOMEM;
>
> instance->serdes_phy = devm_phy_optional_get(dev, NULL);
[Severity: High]
Does devm_phy_optional_get() fetch the per-port serdes PHY from the parent
device node rather than the specific port subnode?
Because it uses dev and not subnode, it appears all subnodes will resolve to
the exact same PHY instance, which would misclassify every port as
PHY_TYPE_USB3 and break USB2 port initialization.
Should this use devm_of_phy_optional_get(dev, subnode, NULL)?
> if (IS_ERR(instance->serdes_phy))
> return dev_err_probe(dev, PTR_ERR(instance->serdes_phy), "error on serdes phy for USB 3.0\n");
>
> if (!instance->serdes_phy &&
> !of_property_present(subnode, "airoha,usb2-monitor-clk-sel"))
> return dev_err_probe(dev, ret, "monitor clock selection or serdes PHY is mandatory for USB PHY\n");
[Severity: Medium]
If the subnode lacks both the serdes_phy and the airoha,usb2-monitor-clk-sel
properties, the error path returns the uninitialized ret variable.
Can this return a garbage error code (or 0) leading to silent initialization
failures?
>
> if (instance->serdes_phy) {
> instance->type = PHY_TYPE_USB3;
> } else {
> ret = of_property_read_u32(subnode, "airoha,usb2-monitor-clk-sel",
> &instance->monclk_sel);
> if (ret)
> return dev_err_probe(dev, ret, "failed to parse Monitor clock selection\n");
>
> ret = of_property_read_u32(subnode, "reg",
> &instance->offset);
> if (ret)
> return dev_err_probe(dev, ret, "missing reg for U2 PHY\n");
>
> instance->type = PHY_TYPE_USB2;
> }
>
> phy = devm_phy_create(dev, subnode, &airoha_phy);
> if (IS_ERR(phy))
> return dev_err_probe(dev, PTR_ERR(phy), "failed to create phy\n");
>
> priv->phys[index] = phy;
[ ... ]
> @@ -1341,12 +1341,12 @@ static int airoha_pcie_phy_parse_ports(struct device *dev,
> struct an7583_usb_phy_priv *priv,
> int index, struct device_node *subnode)
> {
> const struct an7583_usb_phy_match_data *data;
> struct an7583_usb_phy_instance *instance;
> struct phy *phy;
>
> data = of_device_get_match_data(dev);
>
> if (index > data->max_instance)
[Severity: High]
Should this bounds check use >= instead of > in airoha_pcie_phy_parse_ports()?
The max_instance is the size of the array (1 for PCIe), so allowing index to
equal max_instance will cause an out-of-bounds write when priv->phys[index]
is assigned later.
> return -EINVAL;
>
> if (!of_device_is_available(subnode))
> return 0;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260827131551.26725-1-ansuelsmth@gmail.com?part=3
More information about the linux-phy
mailing list