Re: [PATCH 1/3] net/axgbe: Add external PHY read/write functions

Stephen Hemminger
2026-02-17T00:27:52Z

On Mon, 16 Feb 2026 18:22:03 +0530
Ashok Kumar Natarajan <ashokkumar.natarajan@amd.com> wrote:

> Introduce helper functions to perform external PHY register read and
> write operations. These helpers currently support only IEEE Clause 22
> PHY access, providing a simple and consistent API for accessing
> standard 16‑bit MII registers on external PHY devices.
>
> This patch does not implement Clause 45 transactions; support for C45
> access can be added in future updates as needed.
>
> No functional changes are introduced for existing drivers. The new
> helpers will be used by subsequent patches that require external PHY
> management.
>
> Signed-off-by: Ashok Kumar Natarajan <ashokkumar.natarajan@amd.com>
> ---

The result of AI review feedback reports some issues that need resolution.
All AI feedback can be wrong, so use this is inspiration.

Correctness issues (must fix):

1. axgbe_get_phy_id() uses the return value of read() directly in
bit-shift arithmetic without checking for errors. If either MDIO read
fails (returns negative), the resulting PHY ID is garbage and could
accidentally match M88E1512_E_PHY_ID, triggering the wrong init
sequence. Please add error checks after each read.

2. axgbe_phy_an_config() silently drops the return value of
axgbe_m88e1512_config_aneg(). That function can fail at multiple points
but the caller always returns 0. Should be: return
axgbe_m88e1512_config_aneg(pdata);

3. axgbe_m88e1512_init() switches PHY pages multiple times but never
restores page 0 on error paths. If a write fails while on page 0xFF or
0x12, all subsequent MDIO accesses from other code paths will hit the
wrong registers. Add an error cleanup label that restores page 0.

4. The read/write helpers (patch 1) don't check the return value of the
underlying read_ext_mii_regs_c22/write_ext_mii_regs_c22 calls. Errors
from the hardware layer pass through silently.

Other items:

5. Function name typo: axgbe_m88e5112_set_page should be axgbe_m88e1512_set_page to match the actual PHY model.

6. All three subject lines use uppercase "Add" after the colon — should be lowercase per DPDK convention.

7. Missing space: if(ret) should be if (ret) in the init caller in patch 2.

8. The init function blocks for 2 seconds total (two rte_delay_ms(1000) calls). Is polling for reset completion feasible instead?

9. Missing release notes for external PHY support and 100M speed — these are user-visible features.

10. Various magic numbers (0x8001, 0x1177, 0xC00D) written to registers without defines or comments referencing the datasheet.

Please fix at least items 1-4 and resubmit.