Re: [PATCH net-next v19 05/10] net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unintended register overwrites using `phy_write_mmd` clobbers configurations for Gigabit pairs B and D.
- [High] Premature exposure of the network device to userspace and broken probe deferral due to requesting NVMEM cells in `config_init` instead of `probe`.
- [Medium] TX resistance calibration enable bits are defined but not set during calibration, likely causing the hardware to ignore the calibration values.
- [Medium] The downshift tunable silently ignores the requested count, breaking the Ethtool expectation that requested tunable values are enforced.
- [Low] Missing Kconfig dependency on NVMEM causes link errors when the PHY driver is built-in and NVMEM is a module.
- [Low] Multiple register macros contain typos (`R500HM` instead of `R50OHM`, `DELKAY` instead of `DELAY`, `SAMNPLE` instead of `SAMPLE`).
--

--- Patch [5]: [PATCH net-next v19 05/10] net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY ---
Note: Due to system recitation restrictions, I am providing a free-form summary of the review findings instead of the standard inline-quoted diff format.

Commit: 838a0b60afc5cdb8527d6d477a38318567cda39e
Author: Christian Marangi <[email protected]>
Subject: net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY
Summary: Add support for Airoha AN8855 Internal Switch Gigabit PHY. This is a simple PHY driver to configure and calibrate the PHY for the AN8855 Switch with the use of NVMEM cells.

[Severity: Low]
In drivers/net/phy/Kconfig, AIR_AN8855_PHY is added without a dependency on NVMEM.

Will this cause a linker error if the PHY driver is built-in (CONFIG_AIR_AN8855_PHY=y) and NVMEM is built as a module (CONFIG_NVMEM=m)? 

It might be safer to add depends on NVMEM || !NVMEM to prevent this linkage mismatch.

[Severity: High]
In drivers/net/phy/air_an8855.c, inside an8855_config_init(), nvmem_cell_read_u32() is called via an8855_get_r50ohm_val() to fetch calibration data.

Since config_init executes during phy_init_hw(), which typically happens when the interface is brought up (ndo_open), will this break probe deferral? 

If the NVMEM provider is not yet ready, nvmem_cell_read_u32() will return -EPROBE_DEFER. Returning this from an open() syscall prevents the kernel from properly rescheduling the probe. 

Should these NVMEM cells be requested and cached earlier during an8855_probe()?

[Severity: Medium]
Also in an8855_config_init(), when applying the calibration data for TX pairs A, B, C, and D using phy_modify_mmd(), the enable bits (such as AN8855_PHY_R50OHM_RSEL_TX_A_EN) are not included in the mask or set arguments.

Are these enable bits required by the hardware to apply the calibration? 

If so, will the hardware ignore the new calibration values because the enable bits are not being asserted?

[Severity: High]
Further down in an8855_config_init(), phy_write_mmd() is used to configure AN8855_PHY_TX_PAIR_DLY_SEL_GBE and AN8855_PHY_RXADC_CTRL for pairs A and C.

Because phy_write_mmd() overwrites the entire 16-bit register, does this unintentionally zero out the configurations for Gigabit pairs B and D? 

Since Gigabit Ethernet requires all four pairs, clearing the delay and phase selection for pairs B and D might break link establishment or degrade the signal. 

Should this use phy_modify_mmd() instead?

[Severity: Medium]
In an8855_set_downshift(), the function accepts a downshift count, but it entirely ignores the numeric value unless it equals DOWNSHIFT_DEV_DISABLE.

Does this break the ethtool expectation that requested tunable values are strictly enforced? 

Since an8855_get_downshift() unconditionally returns DOWNSHIFT_DEV_DEFAULT_COUNT, subsequent reads won't match the user's setting. 

Should the driver return -EOPNOTSUPP for unsupported numeric values rather than silently ignoring the requested count and returning success?

[Severity: Low]
This isn't a bug, but there are a few typos in the register macro definitions at the top of drivers/net/phy/air_an8855.c.

AN8855_PHY_R500HM_RSEL_TX_AB uses zeroes instead of the letter O. AN8855_PHY_CR_DA_TX_PAIR_DELKAY_SEL_A_GBE spells DELAY as DELKAY, and AN8855_PHY_RG_AD_SAMNPLE_PHSEL_A spells SAMPLE as SAMNPLE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5