Re: [PATCH net-next v3 1/2] r8152: add helper functions for PLA/USB OCP registers

[Simon Horman <horms@kernel.org>]

On Wed, Mar 25, 2026 at 03:35:24AM +0000, Chih Kai Hsu wrote:
> On 25/03/26 2:06 am, Simon Horman 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.
> > 
> > For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> > ---
> > r8152: add helper functions for PLA/USB OCP registers
> > 
> > This commit introduces new helper functions (ocp_dword_w0w1,
> > ocp_word_w0w1, ocp_byte_w0w1, and their _clr_bits/_set_bits variants) to
> > simplify bitwise operations on PLA/USB OCP registers throughout the
> > r8152 driver.
> > 
> > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > > 3b6d4252d34c..ba6dc4dbfd80 100644
> > > --- a/drivers/net/usb/r8152.c
> > > +++ b/drivers/net/usb/r8152.c
> > 
> > [ ... ]
> > 
> > > @@ -3875,35 +3873,30 @@ static void r8153c_ups_en(struct r8152 *tp,
> > > bool enable)
> > >
> > >  static void r8156_ups_en(struct r8152 *tp, bool enable)  {
> > > -     u32 ocp_data = ocp_read_byte(tp, MCU_TYPE_USB,
> > USB_POWER_CUT);
> > > -
> > >       if (enable) {
> > >               r8156_ups_flags(tp);
> > >
> > > -             ocp_data |= UPS_EN | USP_PREWAKE | PHASE2_EN;
> > > -             ocp_write_byte(tp, MCU_TYPE_USB, USB_POWER_CUT,
> > ocp_data);
> > > +             ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
> > > +                               UPS_EN | USP_PREWAKE |
> > PHASE2_EN);
> > >
> > > -             ocp_data = ocp_read_byte(tp, MCU_TYPE_USB,
> > USB_MISC_2);
> > > -             ocp_data |= UPS_FORCE_PWR_DOWN;
> > > -             ocp_write_byte(tp, MCU_TYPE_USB, USB_MISC_2,
> > ocp_data);
> > > +             ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_MISC_2,
> > > +                               UPS_FORCE_PWR_DOWN);
> > >
> > >               switch (tp->version) {
> > >               case RTL_VER_13:
> > >               case RTL_VER_15:
> > > -                     ocp_data = ocp_read_word(tp, MCU_TYPE_USB,
> > USB_UPHY_XTAL);
> > > -                     ocp_data &= ~OOBS_POLLING;
> > > -                     ocp_write_byte(tp, MCU_TYPE_USB,
> > USB_UPHY_XTAL, ocp_data);
> > > +                     ocp_word_clr_bits(tp, MCU_TYPE_USB,
> > USB_UPHY_XTAL,
> > > +                                       OOBS_POLLING);
> >                               ^^^^
> > 
> > Does this actually change the hardware behavior for RTL_VER_13 and
> > RTL_VER_15?
> > 
> > The old code read a 16-bit word with ocp_read_word() to get OOBS_POLLING
> > (BIT(8)), then cleared BIT(8) in the local variable, but wrote it back using
> > ocp_write_byte(). Since ocp_write_byte() masks the data with 0xff, the write
> > operation would only touch the low 8 bits and never actually clear BIT(8) in
> > the hardware register. This made the old code effectively a no-op for clearing
> > OOBS_POLLING.
> > 
> > The new code uses ocp_word_clr_bits() which calls ocp_word_w0w1(), and
> > that performs a proper 16-bit read-modify-write cycle with ocp_write_word().
> > This means BIT(8) will now actually be cleared in the hardware.
> > 
> The OOBS_POLLING (BIT(8)) must be cleared in the USB_UPHY_XTAL register.
> The old code using ocp_write_byte() is wrong.

I expect the best way forward here is to submit a fix for (only) that
problem to net-next.

> 
> > While this looks like it fixes a latent bug where OOBS_POLLING was never
> > being cleared, the commit message describes this as pure refactoring to
> > simplify the code with no mention of a behavioral change.
> > 
> I will mention this fix in the v4 commit message.
> 
> Thanks for your review.
> 
> Best Regards,
> Chih-Kai