Re: [PATCH v3 3/3] phy: airoha: Add support for Airoha AN7583 USB PHY

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 [off-list ref]

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.

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

[ ... ]

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

[ ... ]

[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;
 	}

[ ... ]

[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);

[ ... ]

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

[ ... ]

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

[ ... ]

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

[ ... ]

[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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy