Re: [PATCH net-next 2/2] net: phy: add DAPU Telecom DAP8210R(I) Gigabit Ethernet PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] PHY software reset is not actually triggered because the driver writes `0` to the reset bit instead of `1`.
- [High] RGMII delay configurations are not explicitly cleared when the interface mode disables them.
- [Medium] The debugfs package generator interface unconditionally overwrites the Auto-Negotiation state upon disable.
- [Medium] The debugfs write callback modifies the PHY's MII_BMCR register without acquiring `phydev->lock`.
- [Low] The driver uses a fixed sleep instead of polling the self-clearing reset bit.
- [Low] Incorrect use of `abs()` macro with unsigned types causes a build failure.
- [Low] The device tree property for clock inversion in the code does not match the commit message.
--

--- Patch [2]: [PATCH net-next 2/2] net: phy: add DAPU Telecom DAP8210R(I) Gigabit Ethernet PHY driver ---
commit 78567017e0db0de14fb31e8cedc75e8b75b18e2f
Author: Artem Shimko <a.shimko.dev@gmail.com>

net: phy: add DAPU Telecom DAP8210R(I) Gigabit Ethernet PHY driver

This commit adds support for the DAPU Telecom DAP8211R(I) Gigabit
Ethernet PHY. It implements extended register access for RGMII delay
configuration and provides a debugfs interface for diagnostics.

> The rx-internal-delay-ps and
> tx-internal-delay-ps properties allow precise tuning of clock delays in
> 150 ps steps from 0 to 2250 ps, with automatic selection of the nearest
> supported value if an exact match is not specified. Additionally,
> the optional tx-inverted-clk flag enables 180-degree TX clock phase shift
> for boards where signal integrity or MAC requirements necessitate clock

[Severity: Low]
The commit message documents the property as tx-inverted-clk, but the
code implementation looks for tx-use-inverted-clk.

Which one is the intended property name?

> diff --git a/drivers/net/phy/dap8211r.c b/drivers/net/phy/dap8211r.c
> new file mode 100644
> index 0000000000000..bd695bc4953a2
> --- /dev/null
> +++ b/drivers/net/phy/dap8211r.c

[ ... ]

> +static u16 dap8211r_delay_ps_to_sel(struct phy_device *phydev, u32 ps)
> +{
> +	int i, best_idx = 0;
> +	u32 best_diff = UINT_MAX, diff = 0;
> +
> +	if (!ps)
> +		return 0;
> +
> +	for (i = 0; i < DAP8211R_DELAY_COUNT; i++) {
> +		diff = abs(ps - delay_config[i].ps);

[Severity: Low]
Does this cause a compilation error?

The abs() macro in the kernel strictly checks for signed types, but the
result of the subtraction here is unsigned.

[ ... ]

> +static int dap8211r_config_init(struct phy_device *phydev)
> +{
> +	struct device_node *phydev_node = phydev->mdio.dev.of_node;
> +	u16 mask = 0, set = 0;
> +	int ret;
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		break;

[Severity: High]
If the bootloader configured RGMII delays, are they left intact here?

By not setting the mask to clear the delays for pure RGMII mode, this
leaves the PHY with whatever delays were previously configured, violating
the RGMII interface contract.

[ ... ]

> +	if (of_property_read_bool(phydev_node, "tx-use-inverted-clk"))
> +		set |= DAP8211R_RGMII_CLK_INVERT;
> +
> +	mask |= DAP8211R_RGMII_CLK_INVERT;
> +
> +	ret = dap8211r_modify_ext(phydev, DAP8211R_PHY_CON, DAP8211R_PHY_SW_RST, 0);

[Severity: High]
Does this write actually trigger the software reset?

This clears the reset bit by passing 0 for the set argument, instead of
asserting it by writing DAP8211R_PHY_SW_RST.

> +	if (ret)
> +		return ret;
> +
> +	/* Wait for reset self-clear */
> +	fsleep(200);

[Severity: Low]
Would it be safer to poll the self-clearing reset bit instead of using a
fixed sleep?

A fixed sleep might fail if the hardware reset takes longer than expected,
and contradicts the comment about waiting for a self-clear.

[ ... ]

> +static ssize_t pkg_gen_write(struct file *file,
> +			     const char __user *user_buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct seq_file *sf = file->private_data;
> +	struct dap8211r_priv *priv = sf->private;
> +	struct phy_device *phydev = to_phy_device(priv->dev);
> +	bool en;
> +	u16 mask, set, set_bmcr;
> +	int ret;
> +
> +	ret = kstrtobool_from_user(user_buf, count, &en);
> +	if (ret)
> +		return ret;
> +
> +	if (en) {
> +		set = DAP8211R_PKG_GEN_EN | DAP8211R_PKG_CHK_EN | DAP8211R_PKG_PL_5AA5;
> +
> +		if (priv->crc_cor)
> +			set |= DAP8211R_PKG_COR_CRC;
> +
> +		if (priv->loopback)
> +			set_bmcr = BMCR_LOOPBACK;
> +		else
> +			set_bmcr = BMCR_ANENABLE;
> +	} else {
> +		set = DAP8211R_PKG_GEN_MODE | DAP8211R_PKG_GATE_EN;
> +		set_bmcr = BMCR_ANENABLE;
> +	}
> +
> +	ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK | BMCR_ANENABLE, set_bmcr);

[Severity: Medium]
Is it safe to modify MII_BMCR here without acquiring phydev->lock?

Concurrent execution of the phylib state machine might race with this
debugfs write, leading to corrupted link state.

[Severity: Medium]
Does the else branch intentionally destroy the previous BMCR state?

By unconditionally writing BMCR_ANENABLE to 1 when disabling the packet
generator, any manual speed or duplex settings previously configured by the
user via ethtool will be overwritten.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713131223.279555-1-a.shimko.dev@gmail.com?part=2