[AMD Official Use Only - AMD Internal Distribution Only] Thanks for the review.
axgbe_phy_link_status() follows the existing driver convention where it returns 0 when the link is down and 1 when the link is up. The logic in the patch is aligned with this behavior. Added a brief comment in the code(v3) to make this return semantics clearer. Taken care of the other comments in v3 patches Could you share which AI tool was used for this review? Thanks, Ashok -----Original Message----- From: Stephen Hemminger <[email protected]> Sent: Thursday, February 26, 2026 4:39 AM To: Natarajan, Ashok Kumar <[email protected]> Cc: [email protected]; Sebastian, Selwin <[email protected]> Subject: Re: [PATCH v2 1/3] net/axgbe: add external PHY read/write functions [You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. On Wed, 25 Feb 2026 18:14:54 +0530 Ashok Kumar Natarajan <[email protected]> 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. > > Signed-off-by: Ashok Kumar Natarajan <[email protected]> > --- I didn't see anything here, but AI spotted some things. Please revise and resubmit. Thanks for the series. A few findings, mostly in patch 2: Patch 2 (M88E1512 support): 1. axgbe_phy_link_status() always returns 0 (link up) even when the M88E1512 reports link down or the read fails. The goto out path leads to return 0, which means "link up" in this function's contract. The link-down and error paths need to return a non-zero value. 2. Integer shift UB in axgbe_get_phy_id(): (u32)(phy_id_1 << 16) evaluates the shift at int width before widening. If bit 15 of phy_id_1 is set, shifting into the sign bit is undefined behavior. Move the cast before the shift: (u32)phy_id_1 << 16. 3. axgbe_get_ext_phy_link_status() doesn't set *linkup = false on the BMCR_ANRESTART early-return path. Works today because the caller pre-initializes the variable, but the function should be self-contained. 4. The new out label unconditionally clears rx_adapt_done on the M88E1512 link-down path — please verify this is intentional and doesn't interfere with the internal SerDes receiver adaptation state machine. Patch 3 (100Mbps): 5. The SPEED_1000 → SPEED_100 change in axgbe_sgmii_100_mode() is a bug fix in existing code — the 100M mode function was incorrectly configuring the MAC for 1G. This should have its own Fixes: tag and Cc: [email protected] so it gets backported to stable branches. Item 1 is the most critical — it inverts link status reporting for the M88E1512.
