Re: [PATCH net-next v2 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 5 potential issue(s) to consider:
- [High] Unconfigured RGMII delays are not explicitly disabled, leaking hardware state.
- [High] Software reset is triggered before RGMII settings are written.
- [Medium] Invalid RGMII delay values in the Device Tree are silently ignored instead of failing probe.
- [Low] The Kconfig help text claims support for an 'internal packet generator' that is not implemented.
- [Low] The <linux/delay.h> header is not explicitly included despite using fsleep().
--

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

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

This commit adds a new PHY driver for the DAPU Telecom DAP8211R(I) Gigabit
Ethernet PHY. It implements extended register access and provides device
tree support for RGMII delay configuration and TX clock phase shifting.

> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 099f25dceabbc..4576f707ac944 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
[ ... ]
> @@ -237,6 +237,16 @@ config DAVICOM_PHY
>  	help
>  	  Currently supports dm9161e and dm9131
>  
> +config DAP8211R_PHY
> +	tristate "DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY"
> +	depends on OF
> +	help
> +	  Support for the DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY.
> +	  This PHY is designed for enterprise and industrial networking
> +	  applications, supporting 10/100/1000 Mbps operation.
> +	  RGMII with: configurable TX/RX clock delays, optional flag to enable
> +	  180-degree TX clock phase shift and internal packet generator.

[Severity: Low]
Is this help text accurate regarding the internal packet generator?

The description mentions an internal packet generator, but there doesn't
appear to be any functionality for it implemented in the driver itself.

> diff --git a/drivers/net/phy/dap8211r.c b/drivers/net/phy/dap8211r.c
> new file mode 100644
> index 0000000000000..e1e6a322ef0c4
> --- /dev/null
> +++ b/drivers/net/phy/dap8211r.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL
> +/*
> + * Driver for the DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY.
[ ... ]
> +#include <linux/bitfield.h>
> +#include <linux/errno.h>
> +#include <linux/ethtool.h>

[Severity: Low]
Does this file need to include <linux/delay.h>?

The code uses fsleep() in dap8211r_config_init(), but the delay header
is not explicitly included. This might cause build failures on some
architectures or configs.

[ ... ]
> +static int dap8211r_delay_ps_to_sel(u32 ps)
> +{
> +	for (int i = 0; i < DAP8211R_DELAY_COUNT; i++)
> +		if (ps == delay_config[i].ps)
> +			return delay_config[i].sel;
> +
> +	return -EINVAL;
> +}
[ ... ]
> +static int dap8211r_get_rgmii_delay(struct phy_device *phydev, const char *prop_name, bool is_id)
> +{
> +	struct device_node *np = phydev->mdio.dev.of_node;
> +	u32 ps = 0;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, prop_name, &ps);
> +	if (ret == -EINVAL)
> +		return (is_id) ? DAP8211R_DEFAULT_DELAY_SEL : ret;
> +	if (ret < 0)
> +		return ret;
> +
> +	return dap8211r_delay_ps_to_sel(ps);
> +}
[ ... ]
> +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, retries = 10;
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		ret = dap8211r_get_rgmii_delay(phydev, "rx-internal-delay-ps", false);
> +		if (ret >= 0) {
> +			set = FIELD_PREP(DAP8211R_RGMII_RX_DEL_MASK, ret);
> +			mask = DAP8211R_RGMII_RX_DEL_MASK;
> +		} else if ((ret < 0) && (ret != -EINVAL)) {
> +			return ret;
> +		}

[Severity: Medium]
Does this logic unintentionally skip invalid delay values in the device tree?

Because dap8211r_get_rgmii_delay() returns -EINVAL when a property is
entirely missing, and dap8211r_delay_ps_to_sel() also returns -EINVAL for
an invalid out-of-bounds value (like rx-internal-delay-ps = <200>), the
check here treats both conditions exactly the same.

This means if an invalid delay is provided in the device tree, it will be
silently ignored instead of failing probe.

[Severity: High]
Will this leave stale hardware default delays enabled if properties are missing?

In PHY_INTERFACE_MODE_RGMII, if the delay properties are not provided,
the mask bits remain empty. The driver doesn't explicitly clear the
DAP8211R_RGMII_RX_DEL_MASK or DAP8211R_RGMII_TX_DEL_MASK bits in the mask.
If the bootloader or hardware defaults left these delays active, they will
not be disabled, violating the requested PHY_INTERFACE_MODE_RGMII mode.

> +
> +		ret = dap8211r_get_rgmii_delay(phydev, "tx-internal-delay-ps", false);
> +		if (ret >= 0) {
> +			set |= FIELD_PREP(DAP8211R_RGMII_TX_DEL_MASK, ret);
> +			mask |= DAP8211R_RGMII_TX_DEL_MASK;
> +		} else if ((ret < 0) && (ret != -EINVAL)) {
> +			return ret;
> +		}
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		ret = dap8211r_get_rgmii_delay(phydev, "rx-internal-delay-ps", true);
> +		if (ret < 0)
> +			return ret;
> +
> +		set = FIELD_PREP(DAP8211R_RGMII_RX_DEL_MASK, ret);
> +		mask = DAP8211R_RGMII_RX_DEL_MASK;
> +		break;

[Severity: High]
Similarly, in PHY_INTERFACE_MODE_RGMII_RXID, the TX delay mask is never added
to the 'mask' variable. Could this leave the TX delay active if it was
previously configured by the bootloader?

[ ... ]
> +	if (of_property_read_bool(phydev_node, "dapu,tx-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);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for reset self-clear */
> +	do {
> +		fsleep(20);
> +		ret = dap8211r_read_ext(phydev, DAP8211R_PHY_CON);
> +		if (ret < 0)
> +			return ret;
> +	} while (!(ret & DAP8211R_PHY_SW_RST) && --retries);
> +
> +	if (!retries)
> +		return -ETIMEDOUT;
> +
> +	ret = dap8211r_modify_ext(phydev, DAP8211R_RGMII_CON, mask, set);
> +	if (ret)
> +		return ret;

[Severity: High]
Are the RGMII settings applied correctly here?

The comment earlier in dap8211r_config_init() mentions "Software reset to
apply settings", but the software reset is executed and waited upon before
writing to DAP8211R_RGMII_CON.

Because the software reset happens before the new delay configuration is written,
does this mean the newly programmed RGMII delays and clock inversions will not
be latched into the PHY's datapath?

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