Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20231012192552.3900360-1-dianders@chromium.org>
 <20231012122458.v3.5.Ib2affdbfdc2527aaeef9b46d4f23f7c04147faeb@changeid>
 <29f9a2ff1979406489213909b940184f@realtek.com> <CAD=FV=U4rGozXHoK8+ejPgRtyoACy1971ftoatQivqzk2tk5ng@mail.gmail.com>
 <052401da00fa$dacccd90$906668b0$@realtek.com> <CAD=FV=XQswgKZh-JQ6PuKGRmrDMfDmZwM+MUpAcOk1=7Ppjyiw@mail.gmail.com>
In-Reply-To: <CAD=FV=XQswgKZh-JQ6PuKGRmrDMfDmZwM+MUpAcOk1=7Ppjyiw@mail.gmail.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 17 Oct 2023 11:37:32 -0700
Message-ID: <CAD=FV=Vp_KE_hjWy7bKJbvmqwCQ67jhzfFoV368vB5ZGge=Yzw@mail.gmail.com>
Subject: Re: [PATCH v3 5/5] r8152: Block future register access if register
 access fails
To:     Hayes Wang <hayeswang@realtek.com>
Cc:     Jakub Kicinski <kuba@kernel.org>,
        "David S . Miller" <davem@davemloft.net>,
        Alan Stern <stern@rowland.harvard.edu>,
        Simon Horman <horms@kernel.org>,
        Edward Hill <ecgh@chromium.org>,
        Laura Nao <laura.nao@collabora.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Grant Grundler <grundler@chromium.org>,
        =?UTF-8?Q?Bj=C3=B8rn_Mork?= <bjorn@mork.no>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Tue, Oct 17, 2023 at 7:17=E2=80=AFAM Doug Anderson <dianders@chromium.or=
g> wrote:
>
> Hi,
>
> On Tue, Oct 17, 2023 at 6:07=E2=80=AFAM Hayes Wang <hayeswang@realtek.com=
> wrote:
> >
> > Doug Anderson <dianders@chromium.org>
> > > Sent: Tuesday, October 17, 2023 12:47 AM
> > [...
> > > > >  static int generic_ocp_read(struct r8152 *tp, u16 index, u16 siz=
e,
> > > > > @@ -8265,6 +8353,19 @@ static int rtl8152_pre_reset(struct
> > > usb_interface
> > > > > *intf)
> > > > >         if (!tp)
> > > > >                 return 0;
> > > > >
> > > > > +       /* We can only use the optimized reset if we made it to t=
he end of
> > > > > +        * probe without any register access fails, which sets
> > > > > +        * `PROBED_WITH_NO_ERRORS` to true. If we didn't have tha=
t then return
> > > > > +        * an error here which tells the USB framework to fully u=
nbind/rebind
> > > > > +        * our driver.
> > > >
> > > > Would you stay in a loop of unbind and rebind,
> > > > if the control transfers in the probe() are not always successful?
> > > > I just think about the worst case that at least one control always =
fails in probe().
> > >
> > > We won't! :-) One of the first things that rtl8152_probe() does is to
> > > call rtl8152_get_version(). That goes through to
> > > rtl8152_get_version(). That function _doesn't_ queue up a reset if
> > > there are communication problems, but it does do 3 retries of the
> > > read. So if all 3 reads fail then we will permanently fail probe,
> > > which I think is the correct thing to do.
> >
> > The probe() contains control transfers in
> >         1. rtl8152_get_version()
> >         2. tp->rtl_ops.init()
> >
> > If one of the 3 control transfers in 1) is successful AND
> > any control transfer in 2) fails,
> > you would queue a usb reset which would unbind/rebind the driver.
> > Then, the loop starts.
> > The loop would be broken, if and only if
> >         a) all control transfers in 1) fail, OR
> >         b) all control transfers in 2) succeed.
> >
> > That is, the loop would be broken when the fail rate of the control tra=
nsfer is high or low enough.
> > Otherwise, you would queue a usb reset again and again.
> > For example, if the fail rate of the control transfer is 10% ~ 60%,
> > I think you have high probability to keep the loop continually.
> > Would it never happen?
>
> Actually, even with a failure rate of 10% I don't think you'll end up
> with a fully continuous loop, right? All you need is to get 3 failures
> in a row in rtl8152_get_version() to get out of the loop. So with a
> 10% failure rate you'd unbind/bind 1000 times (on average) and then
> (finally) give up. With a 50% failure rate I think you'd only
> unbind/bind 8 times on average, right? Of course, I guess 1000 loops
> is pretty close to infinite.
>
> In any case, we haven't actually seen hardware that fails like this.
> We've seen failure rates that are much much lower and we can imagine
> failure rates that are 100% if we're got really broken hardware. Do
> you think cases where failure rates are middle-of-the-road are likely?
>
> I would also say that nothing we can do can perfectly handle faulty
> hardware. If we're imagining theoretical hardware, we could imagine
> theoretical hardware that de-enumerated itself and re-enumerated
> itself every half second because the firmware on the device crashed or
> some regulator kept dropping. This faulty hardware would also cause an
> infinite loop of de-enumeration and re-enumeration, right?
>
> Presumably if we get into either case, the user will realize that the
> hardware isn't working and will unplug it from the system. While the
> system is doing the loop of trying to enumerate the hardware, it will
> be taking up a bunch of extra CPU cycles but (I believe) it won't be
> fully locked up or anything. The machine will still function and be
> able to do non-Ethernet activities, right? I would say that the worst
> thing about this state would be that it would stress corner cases in
> the reset of the USB subsystem, possibly ticking bugs.
>
> So I guess I would summarize all the above as:
>
> If hardware is broken in just the right way then this patch could
> cause a nearly infinite unbinding/rebinding of the r8152 driver.
> However:
>
> 1. It doesn't seem terribly likely for hardware to be broken in just this=
 way.
>
> 2. We haven't seen hardware broken in just this way.
>
> 3. Hardware broken in a slightly different way could cause infinite
> unbinding/rebinding even without this patch.
>
> 4. Infinite unbinding/rebinding of a USB adapter isn't great, but not
> the absolute worst thing.
>
>
> That all being said, if we wanted to address this we could try two
> different ways:
>
> a) We could add a global in the r8152 driver and limit the number of
> times we reset. This gets a little ugly because if we have multiple
> r8152 adapters plugged in then the same global would be used for both,
> but maybe it's OK?
>
> b) We could improve the USB core to somehow prevent usb_reset_device()
> from running too much on a given device?
>
>
> ...though I would re-emphasize that I don't think this is something we
> need to address now. If later we actually see a problem we can always
> address it then.

One other idea occurred to me that we could do, if we cared to solve
this hypothetical failure case. We could change the code to always
read the version 4 times on every probe. If one of the transfers fails
then we could consider that OK. If 2 or more transfers fails then we
could consider that to be an error. You still might get a _few_
unbind/bind in this hypothetical failure mode, but I think it would
catch the problem more quickly.

My probability theory is rusty and I'm sure there's a better way, but
I think we can just add up all the cases. Assuming a 10% failures and
90% success of any transfer:

# Chance of 2 failures:
.10 * .10 * .90 * .90 +
.10 * .90 * .10 * .90 +
.10 * .90 * .90 * .10 +
.90 * .10 * .90 * .10 +
.90 * .90 * .10 * .10

# Chance of 3 failures:
.10 * .10 * .10 * .90 +
.10 * .10 * .90 * .10 +
.10 * .90 * .10 * .10 +
.90 * .10 * .10 * .10

# Chance of 4 failures:
.10 * .10 * .10 * .10

If I add that up I get about a 4.4% chance of 2 or more failures in 4
reads. That means if we got into an unbind/bind cycle we'd get out of
it (on average) in ~23 probes because we'd see enough failures. We
could likely reduce this further by reading the version 5 or 6 times.

I will note that my measurements showed that a normal probe is ~200
transfers and also includes a bunch of delays, so reading the version
a few times wouldn't be a huge deal.


In any case, I'm still of the opinion that we don't need to handle this.

-Doug