Re: [PATCH net-next v23 3/3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: phy: motorcomm: missing phy_package_init_once in yt8824_config_init disrupts active ports
- [High] net: phy: motorcomm: missing U0 un-reset write leaves U0 TX channels permanently disabled
- [Medium] net: phy: motorcomm: incorrect mask in yt8824_config_utp_init_paged causes CH2 register corruption
--

commit 198ccc56ee2557369512bc1763b9592679651225
Author: Kyle Switch <kyle.switch@motor-comm.com>

net: phy: Add driver for Motorcomm Quad 2.5GbE phy

Adds support for the Motorcomm YT8824 quad-port 2.5G PHY to the existing
motorcomm driver, using the phy_package helpers for the shared top
extended register space.

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 90a4f86f2758b..dfa0612d713cb 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
[ ... ]
> +static int yt8824_config_utp_init_paged(struct phy_device *phydev)
> +{
[ ... ]
> +	/* 100M template amplitude */
> +	ctrl = FIELD_PREP(YT8824_DAC_IMID_CH3_ORG, 0x45);
> +	ctrl |= FIELD_PREP(YT8824_DAC_IMID_CH2_ORG, 0x45);
> +	mask = YT8824_DAC_IMID_CH3_ORG | YT8824_DAC_IMID_CH3_ORG;

[Severity: Medium]
Should this mask clear YT8824_DAC_IMID_CH2_ORG instead of duplicating
YT8824_DAC_IMID_CH3_ORG? The CH2 bits don't seem to be cleared before being
modified, which might lead to incorrect transmission amplitude on CH2.

> +	ret = ytphy_modify_ext_with_lock(phydev,
> +					 YT8824_DAC_IMID_CH2_CH3_CFG_ORG_REG,
> +					 mask, ctrl);
> +	if (ret < 0)
> +		goto err_restore;
[ ... ]
> +	ctrl = FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH0, 1);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH1, 1);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH2, 1);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH3, 1);
> +	mask = YT8824_U0_CSR_RESETB_TX_CH0 | YT8824_U0_CSR_RESETB_TX_CH1 |
> +	       YT8824_U0_CSR_RESETB_TX_CH2 | YT8824_U0_CSR_RESETB_TX_CH3;
> +
> +	ctrl = FIELD_PREP(YT8824_U1_CSR_RESETB_TX_CH0, 1);
> +	ctrl |= FIELD_PREP(YT8824_U1_CSR_RESETB_TX_CH1, 1);
> +	ctrl |= FIELD_PREP(YT8824_U1_CSR_RESETB_TX_CH2, 1);
> +	ctrl |= FIELD_PREP(YT8824_U1_CSR_RESETB_TX_CH3, 1);
> +	mask = YT8824_U1_CSR_RESETB_TX_CH0 | YT8824_U1_CSR_RESETB_TX_CH1 |
> +	       YT8824_U1_CSR_RESETB_TX_CH2 | YT8824_U1_CSR_RESETB_TX_CH3;
> +	ret = ytphy_modify_ext_with_lock(phydev, YT8824_U1_CRS_RESET_REG,
> +					 mask, ctrl);

[Severity: High]
Is the ytphy_modify_ext_with_lock() call for YT8824_U0_CRS_RESET_REG missing
here? Since ctrl and mask for U0 are immediately overwritten by U1's values
before a write occurs, the un-reset sequence is completely skipped for U0.
This appears to leave half of the quad PHY's transmit channels permanently
disabled.

> +	if (ret < 0)
> +		goto err_restore;
[ ... ]
> +static int yt8824_config_init(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	if (priv->package_mode == PHY_INTERFACE_MODE_INTERNAL) {
> +		ret = yt8824_config_utp_init_paged(phydev);
> +		if (ret < 0)
> +			goto err;
> +	} else {
> +		ret = yt8824_extern_config_sds_init_paged(phydev);

[Severity: High]
Does this initialization need to be guarded by phy_package_init_once()?

The Linux PHY library calls config_init individually for each PHY port on
the device when it is brought up. Unconditionally executing the shared SERDES
initialization for every port could overwrite shared configurations and assert
global hardware resets on other active ports within the same PHY package.

> +		if (ret < 0)
> +			goto err;
> +		ret = yt8824_config_utp_init_paged(phydev);
> +		if (ret < 0)
> +			goto err;
> +	}
> +	mutex_unlock(&priv->shared_lock);
> +	ret = yt8824_soft_reset(phydev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930194513.663575-1-kyle.switch@motor-comm.com?part=3