Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20231020083125.1.I3e5f7abcbf6f08d392e31a5826b7f234df662276@changeid>
 <60a358c9-b44b-4d25-9a20-aa9e00c65ab6@rowland.harvard.edu>
 <CAD=FV=VQ22p7iDdzK-CXOBEqpu0b3J=LKRZuPcajQWFUmr=1cA@mail.gmail.com> <2b6352b1-e192-47d6-bdce-b63216ab674b@rowland.harvard.edu>
In-Reply-To: <2b6352b1-e192-47d6-bdce-b63216ab674b@rowland.harvard.edu>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 20 Oct 2023 09:27:48 -0700
Message-ID: <CAD=FV=Vv+xiJu0MwEgF9VpdzcQVmmCE+m2Msa09QXB_gh4U=Hg@mail.gmail.com>
Subject: Re: [PATCH] usb: core: Prevent infinite loops when usb_reset_device() unbinds/binds
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, Grant Grundler <grundler@chromium.org>,
        Hayes Wang <hayeswang@realtek.com>,
        Bastien Nocera <hadess@hadess.net>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Flavio Suligoi <f.suligoi@asem.it>,
        Hans de Goede <hdegoede@redhat.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Ivan Orlov <ivan.orlov0322@gmail.com>,
        Marc Kleine-Budde <mkl@pengutronix.de>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Ray Chi <raychi@google.com>,
        =?UTF-8?Q?Ricardo_Ca=C3=B1uelo?= <ricardo.canuelo@collabora.com>,
        Rob Herring <robh@kernel.org>, Roy Luo <royluo@google.com>,
        Stanley Chang <stanley_chang@realtek.com>,
        Vincent Mailhol <mailhol.vincent@wanadoo.fr>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Fri, Oct 20, 2023 at 9:23=E2=80=AFAM Alan Stern <stern@rowland.harvard.e=
du> wrote:
>
> On Fri, Oct 20, 2023 at 08:59:42AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Oct 20, 2023 at 8:46=E2=80=AFAM Alan Stern <stern@rowland.harva=
rd.edu> wrote:
> > >
> > > On Fri, Oct 20, 2023 at 08:31:38AM -0700, Douglas Anderson wrote:
> > > > When we call usb_reset_device() and a driver doesn't implement
> > > > pre_reset() and post_reset() methods then the USB core will attempt=
 to
> > > > unbind and rebind the driver in order to make reset work. This is a
> > > > great general solution, but it has the potential to loop forever.
> > > > Specifically, if the USB device is in a state that the USB device
> > > > driver issues another usb_reset_device() after each rebind then we'=
ll
> > > > just continually unbind and rebind with no end.
> > > >
> > > > It's difficult to address this condition in a USB device driver
> > > > because it's hard for the driver to keep state across each
> > > > unbind/bind.
> > >
> > > How about just adding appropriate pre_reset() and post_reset() method=
s?
> > > This is precisely what they are meant for.  Then the the unbind/rebin=
d
> > > loop wouldn't ever get started.
> >
> > Right, and we have pre_reset() and post_reset() in the r1852 driver.
> > The issue is that we are seeing occasional control transfer errors
> > while the r8152 driver is still running its probe() routine and we
> > want to reset in response to those. It is relatively difficult to have
> > the pre_reset() and post_reset() methods work properly if failures
> > happen and probe() hasn't finished yet.
>
> Why is that?
>
> >  The current proposal I have
> > for the r8152 driver is to have the pre_reset() routine return -EIO if
> > we saw errors during probe, which tells the USB core to unbind/rebind
> > us. This gets us to a known good state.
>
> Don't you also get to a known good state if pre_reset() and post_reset()
> both return 0?  Then there's no unbinding, so the driver can just jump
> back to the start of its probe() routine.  Or fail the probe, if there
> have been too many errors.
>
> >  If we need to do a reset later
> > on (after probe finished successfully) then pre_reset() and
> > post_reset() can avoid the unbind/bind.
> >
> > The worry was that this could cause an infinite loop. Hence this patch.=
 ;-)
>
> With no unbinding/rebinding, any loops that occur will be entirely under
> the driver's control.  Then it should easily be able to avoid looping
> forever.

Hmmm, OK. Let's see. I guess the most annoying thing would be dealing
with anything "devm". I guess in the case of the r8152 driver, though,
there is no use of devm. Thus I should be able to do this. Let me give
it a shot and see how it looks.

-Doug