Re: RTL8152_INACCESSIBLE was Re: [PATCH 6.1 000/194] 6.1.68-rc1 review

[Doug Anderson]

Hi,

On Wed, Dec 13, 2023 at 9:38 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Dec 13, 2023 at 07:16:52AM -0800, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Dec 13, 2023 at 12:50 AM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 08:52:25AM +0100, Pavel Machek wrote:
> > > > Hi!
> > > >
> > > > > > > This is the start of the stable review cycle for the 6.1.68 release.
> > > > > > > There are 194 patches in this series, all will be posted as a response
> > > > > > > to this one.  If anyone has any issues with these being applied, please
> > > > > > > let me know.
> > > > > >
> > > > > >
> > > > > > > Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > > > > >      r8152: Add RTL8152_INACCESSIBLE to r8153_aldps_en()
> > > > > > >
> > > > > > > Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > > > > >      r8152: Add RTL8152_INACCESSIBLE to r8153_pre_firmware_1()
> > > > > > >
> > > > > > > Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > > > > >      r8152: Add RTL8152_INACCESSIBLE to r8156b_wait_loading_flash()
> > > > > > >
> > > > > > > Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > > > > >      r8152: Add RTL8152_INACCESSIBLE checks to more loops
> > > > > > >
> > > > > > > Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > > > > >      r8152: Rename RTL8152_UNPLUG to RTL8152_INACCESSIBLE
> > > > > >
> > > > > > Central patch that actually fixes something is:
> > > > > >
> > > > > > commit d9962b0d42029bcb40fe3c38bce06d1870fa4df4
> > > > > > Author: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > > > > Date:   Fri Oct 20 14:06:59 2023 -0700
> > > > > >
> > > > > >      r8152: Block future register access if register access fails
> > > > > >
> > > > > > ...but we don't have that in 6.1. So we should not need the rest,
> > > > > > either.
> > > > > >
> > > > >
> > > > > Also, the missing patch is fixed subsequently by another patch, so it can not
> > > > > be added on its own.
> > > >
> > > > For the record I'm trying to advocate "drop all patches listed as they
> > > > don't fix the bug", not "add more", as this does not meet stable
> > > > criteria.
> > >
> > > But the original commit here does say it fixes a bug, see the text of
> > > the commits listed above.  So perhaps someone got this all wrong when
> > > they wrote the original commits that got merged into 6.7-rc?  Otherwise
> > > this seems like they are sane to keep for now, unless the original
> > > author says they should be dropped, or someone who can test this driver
> > > says something went wrong.
> >
> > Right. The patches that "add RTL8152_INACCESSIBLE" to more loops are
> > bugfixes, but they're not terribly important ones to backport. While
> > they technically make sense even on older kernels and could
> > conceivably make the older kernels unload the r8152 driver a little
> > faster when a device is unplugged, it's not a big deal. On the first
> > version of the recent patches I didn't even add a "Fixes" tag for them
> > but I was asked to during the review process.
> >
> > The "add RTL8152_INACCESSIBLE" patches become more important with
> > commit d9962b0d4202 ("r8152: Block future register access if register
> > access fails"). Once you have that it's possible to end up in the
> > "INACCESSIBLE" situation in response to normal (ish) error handling
> > and thus you want it to be faster.
> >
> > Based on our experience in ChromeOS, commit d9962b0d4202 ("r8152:
> > Block future register access if register access fails") is a pretty
> > important fix and I would say it should be backported to stable.
> > Certainly we've backported it to our kernels in ChromeOS. In our case
> > we made things easier on ourselves by backporting pretty much all
> > patches to the r8152 driver.
>
> Ok, as lots of fixes seem to be needed here, do you have a list of the
> git ids that we should backport to bring this up to a workable state
> like you have in your tree?

The ones that were relevant to commit d9962b0d4202 ("r8152: Block
future register access if register access fails") were these, which is
basically the two series that I landed recently.:

79321a793945 r8152: Add RTL8152_INACCESSIBLE to r8153_aldps_en()
8c53a7bd7065 r8152: Add RTL8152_INACCESSIBLE to r8153_pre_firmware_1()
8a67b47fced9 r8152: Add RTL8152_INACCESSIBLE to r8156b_wait_loading_flash()
32a574c7e268 r8152: Add RTL8152_INACCESSIBLE checks to more loops
e62adaeecdc6 r8152: Hold the rtnl_lock for all of reset

d9962b0d4202 r8152: Block future register access if register access fails
715f67f33af4 r8152: Rename RTL8152_UNPLUG to RTL8152_INACCESSIBLE
bc65cc42af73 r8152: Check for unplug in r8153b_ups_en() / r8153c_ups_en()
dc90ba37a8c3 r8152: Check for unplug in rtl_phy_patch_request()
b8d35024d405 r8152: Release firmware if we have an error in probe
bb8adff9123e r8152: Cancel hw_phy_work if we have an error in probe
5dd176895269 r8152: Run the unload routine if we have errors during probe
a5feba71ec9c r8152: Increase USB control msg timeout to 5000ms as per spec

The problem, of course, is that picking commit d9962b0d4202 ("r8152:
Block future register access if register access fails") causes
conflicts with other changes that have happened to this driver. On
ChromeOS we generally resolve things like this by just picking the
extra patches. We already had quite a few backports to the ChromeOS
kernel trees, so I can't say it would be the same for stable branches.
On ChromeOS 5.15 you can see the picks that were needed at:

https://crrev.com/c/4857700

If you click "SHOW ALL (17)" then you can see the patches that I
uploaded together to get things resolved on our ChromeOS 5.15 branch.
There are also links to our 6.1, 5.10, and 5.4 branches.

One note is that as part of taking these extra patches we ended up
getting commit ec51fbd1b8a2 ("r8152: add USB device driver for config
selection") which caused a regression. That's where the following
recent changes that landed in your tree came from:

aa4f2b3e418e r8152: Choose our USB config with choose_configuration()
rather than probe()
a87b8e3be926 usb: core: Allow subclassed USB drivers to override
usb_choose_configuration()
c2d95fcff0f0 usb: core: Don't force USB generic_subclass drivers to
define probe()

...and where the (pending) extra patch came from:

https://lore.kernel.org/r/20231211073237.v3.1.If27eb3bf7812f91ab83810f232292f032f4203e0@changeid
("usb: core: Fix crash w/ usb_choose_configuration() if no driver")


-Doug