RE: [PATCH net-next v3 6/8] r8152: add TGPHY access support

[Chih Kai Hsu]

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 0
> 
> - [Medium] r8157_phy_read() (drivers/net/usb/r8152.c:2086) and
>   r8157_phy_write() (:2095) discard rtl_tgphy_access()'s return value.
> - [Medium] wait_tgphy_cmd_ready() (drivers/net/usb/r8152.c:2045) makes
>   its loop-exit decision on the value returned by ocp_read_word(), which…
> 
> --
> 
> r8152: add TGPHY access support
> 
> This adds a TGPHY register access path for RTL8157/RTL8159 using
> USB_TGPHY_CMD/ADDR/DATA, so PHY registers can be reached without
> reprogramming the PLA OCP base. It also introduces phy_read/phy_write
> function pointers in struct rtl_ops so each chip gets the right access method,
> with ocp_reg_read()/ocp_reg_write() becoming thin dispatchers.
> 
> A couple of questions about error propagation in the new path below.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 06fad895fce09..1fcb1cc5b4a18 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -959,6 +966,8 @@ struct r8152 {
> >               void (*hw_phy_cfg)(struct r8152 *tp);
> >               void (*autosuspend_en)(struct r8152 *tp, bool enable);
> >               void (*change_mtu)(struct r8152 *tp);
> > +             u16 (*phy_read)(struct r8152 *tp, u16 addr);
> > +             void (*phy_write)(struct r8152 *tp, u16 addr, u16 data);
> >       } rtl_ops;
> 
> [ ... ]
> 
> > @@ -1687,6 +1696,16 @@ static void ocp_reg_write(struct r8152 *tp, u16
> addr, u16 data)
> >       ocp_write_word(tp, MCU_TYPE_PLA, ocp_index, data);  }
> >
> > +static u16 ocp_reg_read(struct r8152 *tp, u16 addr) {
> > +     return tp->rtl_ops.phy_read(tp, addr); }
> > +
> > +static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data) {
> > +     tp->rtl_ops.phy_write(tp, addr, data); }
> > +
> 
> [ ... ]
> 
> > @@ -2023,6 +2042,61 @@ static int r8152_mdio_test_and_clr_bit(struct
> r8152 *tp, u16 addr, u16 clear)
> >       return data & clear;
> >  }
> >
> > +static int wait_tgphy_cmd_ready(struct r8152 *tp) {
> > +     u16 ocp_data;
> > +
> > +     return poll_timeout_us(ocp_data = ocp_read_word(tp,
> MCU_TYPE_USB,
> > +
> USB_TGPHY_CMD),
> > +                            !(ocp_data & TGPHY_CMD_BUSY), 2000,
> 20000,
> > +                            false);
> > +}
> 
> [Severity: Medium]
> 
> wait_tgphy_cmd_ready() makes its loop-exit decision purely on the value
> returned by ocp_read_word(), which is indeterminate if the underlying
> transfer never happens:
> 
>   ocp_read_word() {
>         u32 data;
>         __le32 tmp;
>         ...
>         generic_ocp_read(tp, index, sizeof(tmp), &tmp, type | byen);
>         data = __le32_to_cpu(tmp);
>         ...
>   }
> 
> generic_ocp_read() starts with:
> 
>         if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
>                 return -ENODEV;
> 
> so tmp is left untouched and the u16 handed to the poll loop is stale stack
> content.  If bit 0 happens to be set, does this helper then sleep for its whole
> 20 ms budget instead of returning right away?
> 
> The USB-error case looks deterministic rather than random, since
> get_registers() does:
> 
>         if (ret < 0)
>                 memset(data, 0xff, size);
> 
> USB_TGPHY_CMD then reads back as 0xffff, TGPHY_CMD_BUSY always appears
> asserted, and every TGPHY access consumes the full timeout.
> 
> Since every PHY register access on RTL8157/RTL8159 now funnels through
> rtl_tgphy_access() (one wait per write, two per read), would a device marked
> inaccessible mid-sequence make r8157_hw_phy_cfg()/hw_phy_work, the
> up/down paths and the mdio ioctl sleep 20-40 ms per register while holding
> tp->control?  Would an is-accessible check in the helper be preferable
> tp->to
> polling a value that may never have been read?

I will change the poll condition to break when the device is inaccessible.

> 
> > +
> > +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);
> > +
> > +     if (!write) {
> > +             ret = wait_tgphy_cmd_ready(tp);
> > +             if (ret < 0)
> > +                     goto out;
> > +
> > +             *data = ocp_read_word(tp, MCU_TYPE_USB,
> USB_TGPHY_DATA);
> > +     }
> > +
> > +out:
> > +     return ret;
> > +}
> > +
> > +static u16 r8157_phy_read(struct r8152 *tp, u16 addr) {
> > +     u16 data = 0;
> > +
> > +     rtl_tgphy_access(tp, addr, &data, false);
> > +
> > +     return data;
> > +}
> > +
> > +static void r8157_phy_write(struct r8152 *tp, u16 addr, u16 data) {
> > +     rtl_tgphy_access(tp, addr, &data, true); }
> 
> [Severity: Medium]
> 
> Both wrappers discard rtl_tgphy_access()'s status.  On a
> wait_tgphy_cmd_ready() timeout, r8157_phy_read() returns the pre-initialised
> 0, which is indistinguishable from real register content.
> 
> Can that fabricated 0 get written back to the PHY?  ocp_reg_read() now
> dispatches to r8157_phy_read() for RTL8157/RTL8159, and the
> read-modify-write helpers use it directly:
> 
>   ocp_reg_w0w1() {
>         u16 data;
> 
>         data = ocp_reg_read(tp, addr);
>         data = (data & ~clear) | set;
>         ocp_reg_write(tp, addr, data);
>   }
> 
> so a timed-out read makes the driver write (0 & ~clear) | set, dropping every
> other bit of the MII/PHY register (advertisement, EEE, PLL/ALDPS).
> r8152_mdio_set_bit(), r8152_mdio_test_and_clr_bit() and sram_write_w0w1()
> have the same shape.
> 
> For the write direction, a first-wait timeout jumps to out before any of the
> ADDR/DATA/CMD writes are issued, so the PHY write is dropped entirely.
> With the void signature there is no way for the caller to notice, and no
> dev_err()/dev_warn() is emitted, so should this at least be logged?
> 
> Note that the existing global mitigation does not cover a plain BUSY
> timeout: rtl_set_inaccessible() is only reached when a USB control transfer fails
> in r8152_control_msg(), so here the control transfers succeed and
> generic_ocp_write() still pushes the clobbered value to the chip.
> 
> The sibling ADV interface in the same file returns int for exactly this
> reason:
> 
>   static int ocp_adv_read(struct r8152 *tp, u16 cmd, u16 addr, u32 *data)
>   ...
>   static int rtl_bmu_w0w1(struct r8152 *tp, u16 addr, u32 clear, u32 set)
>   {
>         ret = rtl_bmu_read(tp, addr, &bmu);
>         if (ret < 0)
>                 goto out;
>         ...
>   }
> 
> and wait_tgphy_cmd_ready() duplicates wait_cmd_ready() almost verbatim.
> Would it make sense for the new phy_read/phy_write ops in struct rtl_ops to
> carry a status back, or to reuse the existing wait_cmd_ready() helper, so the
> TGPHY timeout can stop the read-modify-write sequence instead of being
> swallowed?

Yes. If the read operation times out, the following write operation shouldn't be executed.
I will change it to reflect the error in the next version.

> 
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908075634.23740-1
> -nic_swsd%40realtek.com

Best,
Chih-Kai