Re: [PATCH 2/2] phy: econet: Add EcoNet USB PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Race condition on shared Frequency Meter registers during concurrent PHY initialization.
- [High] Hardware state machine race due to polling data register instead of valid bit.
- [High] Devres lifecycle bug caused by attaching memory and regmap resources to the child PHY device.
- [Medium] Iteration over disabled Device Tree nodes accesses unpowered hardware.
- [Medium] Missing bounds check on dynamically calculated slew rate configuration exposes hardware to silent bit truncation.
- [Low] Pointer signedness mismatch in `regmap_read_poll_timeout` causes compiler warnings and violates type safety.
--

commit ab614fd3da75925164048ae81b01b59ec760d67e
Author: Caleb James DeLisle <cjd@cjdns.fr>

phy: econet: Add EcoNet USB PHY

This commit adds support for the USB PHY found on EcoNet EN751221, EN751627,
and EN7528 based SoCs. It implements the setup and tuning rituals for both
USB 2.0 and USB 3.0 PHY blocks.

Link: https://github.com/openwrt/openwrt/pull/23264#issuecomment-4415906568

> diff --git a/drivers/phy/phy-econet-usb.c b/drivers/phy/phy-econet-usb.c
> new file mode 100644
> index 0000000000000..e8fa2e80eb130
> --- /dev/null
> +++ b/drivers/phy/phy-econet-usb.c

[ ... ]

> +static void u2_slew_rate_calibration(struct econet_usb_phy_priv *priv,
> +				     struct econet_phy_instance *instance)
> +{
> +	struct regmap *com = instance->regmap;
> +	struct regmap *regmap = priv->regmap;
> +	int fm_out = 0;

Does this pointer type cause compiler warnings?

regmap_read_poll_timeout() passes the address of fm_out to regmap_read(),
which expects an unsigned int pointer. Using a signed int can lead to
incompatible pointer type warnings.

> +	u32 srctrl;
> +
> +	/* Enable HS TX SR calibration */
> +	regmap_set_bits(com, U2PHY_COM + USBPHYACR0, RG_HSTX_SRCAL_EN);
> +
> +	usleep_range(1000, 1500);
> +
> +	/* Enable Free run clock */
> +	regmap_set_bits(regmap, FMREG + FMMONR1, RG_FRCK_EN);
> +
> +	/* Select Monitor Clock */
> +	regmap_update_bits(regmap, FMREG + FMCR0, RG_MONCLK_SEL,
> +			   FIELD_PREP(RG_MONCLK_SEL, instance->port_id));

Is it possible for multiple PHY ports to race when accessing these shared
FMREG registers?

The Linux PHY core serializes phy_init() only per-PHY instance, meaning
multiple ports could potentially initialize at the same time. If two
threads run this calibration concurrently, could one overwrite the active
port selection or prematurely clear the frequency detection enable bit
while the other is still polling?

Might this require a driver-level mutex to serialize access to the FMREG
hardware block?

> +
> +	/* Set cyclecnt */
> +	regmap_update_bits(regmap, FMREG + FMCR0, RG_CYCLECNT,
> +			   FIELD_PREP_CONST(RG_CYCLECNT,
> +					    ECONET_USB_PHY_U2_FM_DET_CYCLE_CNT));
> +
> +	/* Enable Frequency meter */
> +	regmap_set_bits(regmap, FMREG + FMCR0, RG_FREQDET_EN);
> +
> +	/* Timeout can happen and we will apply workaround at the end */
> +	regmap_read_poll_timeout(regmap, FMREG + FMMONR0, fm_out,
> +				 fm_out, ECONET_USB_PHY_FREQDET_SLEEP,
> +				 ECONET_USB_PHY_FREQDET_TIMEOUT);

Could this loop prematurely succeed by reading a stale value?

FMMONR0 holds the result of the calibration. If this function is called
a second time for another port, polling for fm_out != 0 might instantly
succeed using the previous run's result.

Would it be more robust to poll the FMMONR1 register for the RG_FM_VALID
flag before reading the result from FMMONR0?

[ ... ]

> +	} else {
> +		/* (1024 / FM_OUT) * REF_CK * U2_SR_COEF (round to nearest) */
> +		srctrl = ECONET_USB_PHY_REF_CK * ECONET_USB_PHY_U2_SR_COEF;
> +		srctrl = (srctrl * ECONET_USB_PHY_U2_FM_DET_CYCLE_CNT) / fm_out;
> +		srctrl = DIV_ROUND_CLOSEST(srctrl,
> +					   ECONET_USB_PHY_U2_SR_COEF_DIVISOR);
> +		dev_info(&instance->phy->dev,
> +			 "frequency meter result 0x%x, set slew rate 0x%x\n",
> +			 fm_out, srctrl);
> +	}
> +
> +	/* Set slew rate */
> +	regmap_update_bits(com, U2PHY_COM + USBPHYACR0, RG_HSTX_SRCTRL,
> +			   FIELD_PREP(RG_HSTX_SRCTRL, srctrl));

Is there a risk of silent bit truncation here?

The target field RG_HSTX_SRCTRL is only 3 bits wide. If the frequency meter
provides a low output value (for instance, due to a stale read), the division
above could yield a value much larger than 7.

Since FIELD_PREP masks runtime values without error, could this write
an unintended configuration to the hardware? Should the calculated srctrl
value be clamped before applying it?

[ ... ]

> +static int econet_usb_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;

[ ... ]

> +	phy_n = 0;
> +	for_each_child_of_node_scoped(np, child_np) {

Does this loop interact with disabled hardware blocks?

Using for_each_child_of_node_scoped() will iterate over all child nodes,
including those with status = "disabled" in the device tree.

This results in the driver mapping memory, initializing regmaps, and
calling econet_usb_phy_exit() to write to registers on unpowered or
unclocked hardware, which can sometimes cause system hangs.

Would it be safer to use for_each_available_child_of_node_scoped()?

> +		struct econet_phy_instance *instance;
> +		void __iomem *phy_base;
> +		struct device *subdev;
> +		struct resource res;
> +		int retval;

[ ... ]

> +		instance->phy = devm_phy_create(dev, child_np, &econet_phy);
> +		if (IS_ERR(instance->phy))
> +			return dev_err_probe(dev, PTR_ERR(instance->phy),
> +					     "failed to create phy\n");
> +
> +		subdev = &instance->phy->dev;
> +		retval = of_address_to_resource(child_np, 0, &res);
> +		if (retval)
> +			return dev_err_probe(subdev, retval,
> +					     "getting reg for phy %d\n",
> +					     phy_n);
> +
> +		phy_base = devm_ioremap_resource(subdev, &res);

Can using the child PHY device for resource allocations cause memory leaks
and probe failures?

The subdev pointer belongs to the child PHY device created by
devm_phy_create(). Devres resources are tied to the device's reference
count reaching zero.

If userspace holds an active sysfs file descriptor for the PHY device when
the parent platform driver unbinds, the subdev kobject stays alive and
the memory region is not freed.

If the platform driver is re-probed, it might fail with -EBUSY when
requesting the same memory region. Should devm_ioremap_resource and
devm_regmap_init_mmio be associated with the parent platform device instead?

> +		if (IS_ERR(phy_base))
> +			return dev_err_probe(subdev, PTR_ERR(phy_base),
> +					     "remapping reg for phy %d\n",
> +					     phy_n);
> +
> +		instance->regmap =
> +			devm_regmap_init_mmio(subdev, phy_base,
> +					      &econet_usb_phy_regmap_config);
> +		if (IS_ERR(instance->regmap))
> +			return dev_err_probe(subdev, PTR_ERR(instance->regmap),
> +					     "making regmap for phy %d\n",
> +					     phy_n);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518141343.401555-1-cjd@cjdns.fr?part=2