RE: [PATCH net-next v7 08/10] r8152: add TGPHY register access for RTL8157 and RTL8159

[Chih Kai Hsu <hsu.chih.kai@realtek.com>]

On Wednesday, October 21, 2026 8:52 AM, Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> ---
> r8152: add TGPHY register access for RTL8157 and RTL8159
> 
> This adds a TGPHY command channel backend for PHY register access on
> RTL8157 and RTL8159. It is selected through new rtl_ops.phy_read and
> phy_write hooks. The helpers built on ocp_reg_read()/ocp_reg_write() now
> return int, and a subset of callers propagate the errors.
> 
> > This patch propagates the status only where an errno would otherwise
> > be taken for register content or reported as
> > success: the pause parameters, rtl8152_set_speed(),
> > SIOCGMIIREG/SIOCSMIIREG, the PHY patch handshake and the PHY firmware
> > version checks.
> >
> > Not handled here, and left to a follow-up series: the remaining
> > callers, notably the hw_phy_cfg and init sequences,
> > r8153_phy_status(), r8152_mmd_read(), EEE handling and the MII library
> > callbacks, still ignore the status.
> 
> [Severity: Medium]
> Is this description accurate? Some of the callers listed as still ignoring the status
> do act on it in this diff.
> 
> r8153_get_eee() returns an errno from each of its ocp_reg_read() calls:
> 
>         ret = ocp_reg_read(tp, OCP_EEE_ABLE, &val);
>         if (ret < 0)
>                 goto out;
> 
> r8153_phy_status() now leaves its poll on the first failed read:
> 
>         if (ocp_reg_read(tp, OCP_PHY_STATUS, &data) < 0)
>                 break;
> 
> rtl8152_get_link_ksettings() is not in either list. It now fails the whole request
> when the OCP_10GBT_CTRL or OCP_10GBT_STAT read fails.
> 
> rtl8153_in_nway(), r8156_mdio_force_mode(), r8156_eee_en() and
> rtl8152_fw_phy_fixup() also return early on a read failure now.
> 
> Could the commit message be updated to cover these changes?

Sure, I will update the commit message.

> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 558ccb4682c27..7c0de56266963 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -1664,16 +1675,33 @@ static void ocp_reg_write(struct r8152 *tp,
> > u16 addr, u16 data)
> 
> [ ... ]
> 
> > +static int r8152_mdio_read(struct r8152 *tp, u32 reg_addr) {
> > +     u16 data;
> > +     int ret;
> > +
> > +     ret = ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2, &data);
> > +
> > +     return ret < 0 ? ret : data;
> >  }
> 
> [Severity: Medium]
> On RTL8157/RTL8159, can this pass -ETIMEDOUT to the generic MII library as if
> it were a BMCR value? read_mii_word() is the mii_if_info.mdio_read callback,
> and it returns the result of r8152_mdio_read() unchanged.
> 
> mii_nway_restart() in drivers/net/mii.c does:
> 
>         bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);
> 
>         if (bmcr & BMCR_ANENABLE) {
>                 bmcr |= BMCR_ANRESTART;
>                 mii->mdio_write(mii->dev, mii->phy_id, MII_BMCR, bmcr);
>                 r = 0;
>         }
> 
> -ETIMEDOUT is 0xffffff92, which has BMCR_ANENABLE set. write_mii_word()
> only filters RTL8152_INACCESSIBLE, and a TGPHY timeout does not set that flag.
> So 0xff92 would be written to BMCR. That sets BMCR_RESET, BMCR_LOOPBACK,
> BMCR_PDOWN and BMCR_ISOLATE, and the call still returns 0.
> 
> The path is:
> 
> rtl8152_nway_reset()
>   mii_nway_restart()
>     read_mii_word()
>       r8152_mdio_read()
>         r8157_phy_read()
>           rtl_tgphy_access()
>             wait_tgphy_cmd_ready() returns -ETIMEDOUT
> 
> rtl_ethtool_set_eee() also calls mii_nway_restart(). In addition,
> mii_ethtool_get_link_ksettings() would decode link modes from the errno.
> 
> Before this patch, the only errno read_mii_word() could return was -ENODEV,
> and write_mii_word() filters that case.
> 
> The commit message mentions this case, and the later patches in the series do
> not seem to address it. rtl_ops_init() switches RTL_VER_16/17 to
> r8157_phy_read() in this patch. Could read_mii_word()/write_mii_word()
> handle the errno before or in this patch?

The driver uses mii_nway_restart() and mii_ethtool_get_link_ksettings(), returning 0
when tgphy timed out can prevent the two functions from using the -ETIMEDOUT as
a BMCR/BMSR value.

I will fix this in the next version.

> 
> > @@ -1906,100 +1950,217 @@ static void ocp_byte_set_bits(struct r8152
> > *tp, u16 type, u16 index, u8 set)
> 
> [ ... ]
> 
> > +static int wait_tgphy_cmd_ready(struct r8152 *tp) {
> > +     u16 ocp_data;
> > +     int ret;
> > +
> > +     ret = read_poll_timeout(ocp_read_word, ocp_data,
> > +                             test_bit(RTL8152_INACCESSIBLE,
> &tp->flags) ||
> > +                             !(ocp_data & TGPHY_CMD_BUSY),
> > +                             2000, 20000, false, tp,
> > +                             MCU_TYPE_USB, USB_TGPHY_CMD);
> > +
> > +     if (ret)
> > +             dev_err(&tp->intf->dev, "TGPHY cmd busy timeout\n");
> 
> [Severity: Medium]
> What happens if TGPHY_CMD_BUSY stays set while USB transfers still work?
> Each PHY access then waits up to 20ms here, or 40ms for a read. It also prints
> another error that is not ratelimited. There is no recovery, and
> RTL8152_INACCESSIBLE is never set.
> 
> Most callers ignore the status and keep going. r8157_hw_phy_cfg() makes well
> over a hundred PHY accesses, and the firmware loaders do one write per
> firmware word:
> 
> rtl8152_fw_phy_nc_apply() {
>     ...
>         for (i = 0; i < num; i++)
>                 ocp_reg_write(tp, OCP_SRAM_DATA,
> __le16_to_cpu(data[i]));
>     ...
> }
> 
> rtl8152_fw_phy_union_apply() has the same loop.
> 
> All of this runs with tp->control held in the probe, resume, open and reset paths.
> Could this stall for seconds or minutes and flood the log?
> Would ratelimiting the message, or stopping the sequence after the first
> timeout, avoid that?

I prefer to ratelimit the message.

> 
> > +
> > +     return test_bit(RTL8152_INACCESSIBLE, &tp->flags) ? -ENODEV :
> > +ret; }
> > +
> > +static int rtl_tgphy_access(struct r8152 *tp, u16 addr, u16 *data,
> > +bool write) {
> > +     u16 cmd = 0;
> > +     int ret;
> > +
> > +     ret = wait_tgphy_cmd_ready(tp);
> > +     if (ret < 0)
> > +             goto out;
> > +
> > +     if (write) {
> > +             cmd |= TGPHY_CMD_WR;
> > +             ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_DATA,
> *data);
> > +     }
> > +
> > +     ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_ADDR, addr);
> > +
> > +     cmd |= TGPHY_CMD_BUSY;
> > +     ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_CMD, cmd);
> 
> [Severity: Low]
> This is a minor point. The status of the USB_TGPHY_DATA, ADDR and CMD
> transfers is not checked here.
> 
> A misdirected command does not seem possible. r8152_control_msg() calls
> rtl_set_inaccessible() when a control transfer fails, so a CMD write that follows a
> failed ADDR write is never sent.
> 
> rtl_tgphy_access() can still return 0 once after a failed transfer, either for a write
> or when the final USB_TGPHY_DATA read fails. Would it be cleaner to return
> -ENODEV when RTL8152_INACCESSIBLE is set at the end of the transaction?

Agree. Returning -ENODEV when RTL8152_INACCESSIBLE is set is cleaner.

> 
> > +
> > +     if (!write) {
> > +             ret = wait_tgphy_cmd_ready(tp);
> > +             if (ret < 0)
> > +                     goto out;
> > +
> > +             *data = ocp_read_word(tp, MCU_TYPE_USB,
> USB_TGPHY_DATA);
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but ocp_read_word() ignores the
> return value of generic_ocp_read():
> 
> ocp_read_word() {
>     ...
>         generic_ocp_read(tp, index, sizeof(tmp), &tmp, type | byen);
> 
>         data = __le32_to_cpu(tmp);
>     ...
> }
> 
> RTL8152_INACCESSIBLE could be set between the second
> wait_tgphy_cmd_ready() and this read, for example by
> rtl8152_disconnect()->rtl_set_unplug(). In that case generic_ocp_read() returns
> -ENODEV early, tmp stays uninitialized, and rtl_tgphy_access() returns 0.
> SIOCGMIIREG could then copy that stack data into
> data->val_out.

The driver didn't propagate the error of ocp_read*; I think I should follow the
original behavior.

Propagating and handling the error of generic_ocp_read() is out of scope.

> 
> dev_ioctl() requires CAP_NET_ADMIN for SIOCGMIIREG, so unprivileged users
> cannot reach this. r8152_phy_read() already has the same flaw.
> 
> [ ... ]
> 
> > @@ -4177,11 +4338,13 @@ static void r8153b_green_en(struct r8152 *tp,
> > bool enable)
> >
> >  static u16 r8153_phy_status(struct r8152 *tp, u16 desired)  {
> > -     u16 data;
> > +     u16 data = 0;
> >       int i;
> >
> >       for (i = 0; i < 500; i++) {
> > -             data = ocp_reg_read(tp, OCP_PHY_STATUS);
> > +             if (ocp_reg_read(tp, OCP_PHY_STATUS, &data) < 0)
> > +                     break;
> > +
> 
> [Severity: Medium]
> On RTL8157/8159, one 20ms TGPHY busy timeout now ends this poll, which
> previously ran for up to 500 x 20ms. The function then returns 0 or a stale
> masked value. Is that safe for callers that switch on the result?
> 
> r8157_hw_phy_cfg() {
>     ...
>         data = r8153_phy_status(tp, 0);
>         switch (data) {
>         case PHY_STAT_EXT_INIT:
>                 rtl8152_apply_firmware(tp, true);
>                 ocp_reg_clr_bits(tp, 0xa466, BIT(0));
>                 ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
>                 break;
>         case PHY_STAT_LAN_ON:
>         case PHY_STAT_PWRDN:
>         default:
>                 rtl8152_apply_firmware(tp, false);
>                 break;
>         }
>     ...
> }
> 
> A return of 0 falls into the default case. That calls rtl8152_apply_firmware(tp,
> false) and skips the EXT_INIT release writes, even when the PHY really is in
> PHY_STAT_EXT_INIT. r8159_hw_phy_cfg() and
> r8157_init() follow the same pattern.
> 
> The later r8153_phy_status(tp, PHY_STAT_LAN_ON) call also stops after its first
> failed read. The PFM, ALDPS and related settings are then programmed before
> the PHY reaches LAN_ON, and WARN_ON_ONCE() fires.
> 
> Before this patch, the loop always ran to its full timeout on these chips.

r8153_phy_status() should skip the status check and keep polling when
ocp_read_word() failed instead of early break.

> 
> [ ... ]
> 
> > @@ -4598,25 +4762,29 @@ static int rtl_phy_patch_request(struct r8152
> *tp, bool request, bool wait)
> >       }
> >
> >       for (i = 0; wait && i < 5000; i++) {
> > -             u32 ocp_data;
> > -
> >               if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> >                       return -ENODEV;
> >
> >               usleep_range(1000, 2000);
> > -             ocp_data = ocp_reg_read(tp, OCP_PHY_PATCH_STAT);
> > -             if ((ocp_data & PATCH_READY) ^ check)
> > +             ret = ocp_reg_read(tp, OCP_PHY_PATCH_STAT, &ocp_data);
> > +             if (ret < 0)
> > +                     return ret;
> > +             else if ((ocp_data & PATCH_READY) ^ check)
> >                       break;
> >       }
> >
> > -     if (request && wait &&
> > -         !(ocp_reg_read(tp, OCP_PHY_PATCH_STAT) & PATCH_READY)) {
> > -             dev_err(&tp->intf->dev, "PHY patch request fail\n");
> > -             rtl_phy_patch_request(tp, false, false);
> > -             return -ETIME;
> > -     } else {
> > -             return 0;
> > +     if (request && wait) {
> > +             ret = ocp_reg_read(tp, OCP_PHY_PATCH_STAT, &ocp_data);
> > +             if (ret < 0) {
> > +                     return ret;
> > +             } else if (!(ocp_data & PATCH_READY)) {
> > +                     dev_err(&tp->intf->dev, "PHY patch request fail\n");
> > +                     rtl_phy_patch_request(tp, false, false);
> > +                     return -ETIME;
> > +             }
> >       }
> 
> [Severity: Medium]
> When request is true, PATCH_REQUEST is already set in OCP_PHY_PATCH_CMD
> by the time these two new return ret paths run. Do they leave it asserted? The
> -ETIME path releases it with rtl_phy_patch_request(tp, false, false), but the
> errno paths return without doing that.

Agree. Even if the ocp_read_word() failed, the driver should try to release it with
rtl_phy_patch_request() instead of returning without doing that.

> 
> Callers treat any non-zero return as "not acquired" and do not release:
> 
> r8157_hw_phy_cfg() {
>     ...
>         if (rtl_phy_patch_request(tp, true, true))
>                 return;
>     ...
> }
> 
> r8159_hw_phy_cfg() and rtl_ram_code_speed_up() do the same. A TGPHY
> timeout on the OCP_PHY_PATCH_STAT read would leave the PHY in patch
> request mode. r8157_hw_phy_cfg() would also skip the rest of its configuration.
> 
> [ ... ]
> 
> > @@ -6629,7 +6815,10 @@ static int rtl8152_set_speed(struct r8152 *tp, u8
> autoneg, u32 speed, u8 duplex,
> >               }
> >
> >               if (tp->support_2500full || tp->support_5000full ||
> tp->support_10000full) {
> > -                     orig = ocp_reg_read(tp, OCP_10GBT_CTRL);
> > +                     ret = ocp_reg_read(tp, OCP_10GBT_CTRL, &orig);
> > +                     if (ret < 0)
> > +                             goto out;
> > +
> >                       new1 = orig &
> ~(MDIO_AN_10GBT_CTRL_ADV2_5G | MDIO_AN_10GBT_CTRL_ADV5G
> >                                       |
> MDIO_AN_10GBT_CTRL_ADV10G);
> >
> 
> [Severity: Medium]
> The reads are checked now, but the writes in rtl8152_set_speed() still are not.
> The commit message names this function as one where an errno is no longer
> reported as success:
> 
>         if (orig != new1) {
>                 r8152_mdio_write(tp, MII_ADVERTISE, new1);
>                 tp->mii.advertising = new1;
>         }
>         ...
>         if (orig != new1)
>                 r8152_mdio_write(tp, MII_CTRL1000, new1);
>         ...
>         if (orig != new1)
>                 ocp_reg_write(tp, OCP_10GBT_CTRL, new1);
> 
> If one of these writes times out on RTL8157/8159, tp->mii.advertising is still
> updated. The function returns 0 as long as the BMCR write succeeds.
> rtl8152_set_link_ksettings() then saves tp->advertising, tp->speed and
> tp->duplex as if they had been applied.

Agree. If the write failed, tp->mii.advertising should not be updated.

> 
> The PHY patch handshake has the same gap. rtl_phy_patch_request() does not
> check the result of the PATCH_REQUEST write:
> 
>         if (request) {
>                 ocp_reg_set_bits(tp, OCP_PHY_PATCH_CMD,
> PATCH_REQUEST);
>                 check = 0;
>         } else {
>                 ocp_reg_clr_bits(tp, OCP_PHY_PATCH_CMD,
> PATCH_REQUEST);
>                 check = PATCH_READY;
>         }
> 
> This has three effects:
> 
> - With wait false, a failed request or release returns 0. The firmware
>   path uses this mode through rtl_pre_ram_code(..., !power_cut).
> - With request true, a failed write polls for more than 5 seconds and
>   then returns -ETIME instead of the real errno.
> - With request false and wait true, a failed release runs all 5000
>   iterations and still returns 0.
> 
> rtl_pre_ram_code() also turns any error into -ETIME, and
> rtl8152_apply_firmware() ignores its return value.
> 
> Should these write results be checked as well?

Yes. If the previous write failed, rtl_phy_patch_request() should return early
and pass the errno.

> 
> > @@ -6660,20 +6849,23 @@ static int rtl8152_set_speed(struct r8152 *tp,
> u8 autoneg, u32 speed, u8 duplex,
> >       if (test_and_clear_bit(PHY_RESET, &tp->flags))
> >               bmcr |= BMCR_RESET;
> >
> > -     r8152_mdio_write(tp, MII_BMCR, bmcr);
> > +     ret = r8152_mdio_write(tp, MII_BMCR, bmcr);
> > +     if (ret < 0)
> > +             goto out;
> 
> [Severity: Medium]
> Is the pending PHY reset lost when this write fails? PHY_RESET is cleared before
> the write. On RTL8157/8159, r8152_mdio_write() can now return -ETIMEDOUT
> before anything is sent to the hardware:
> 
> rtl_tgphy_access() {
>         ...
>         ret = wait_tgphy_cmd_ready(tp);
>         if (ret < 0)
>                 goto out;
>         ...
> }
> 
> This can happen if the preceding OCP_10GBT_CTRL write is still busy.
> 
> r8157_hw_phy_cfg() and r8159_hw_phy_cfg() end with set_bit(PHY_RESET,
> &tp->flags) so that the next rtl8152_set_speed() resets the PHY. If this write fails,
> BMCR_RESET never reaches the PHY and the flag is not set again.
> 
> Should PHY_RESET be restored on this error path?

Yes. If the write timed out, the driver should restore PHY_RESET. This gives the 
device another chance to reset PHY.

> 
> [ ... ]

Best Regards,
Chih-Kai