Re: [PATCH v5 8/8] r8152: Block future register access if register access fails

From: Simon Horman <horms@kernel.org>
To: Doug Anderson <dianders@chromium.org>
Cc: "Jakub Kicinski" <kuba@kernel.org>,
	"Hayes Wang" <hayeswang@realtek.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Edward Hill" <ecgh@chromium.org>,
	"Laura Nao" <laura.nao@collabora.com>,
	"Alan Stern" <stern@rowland.harvard.edu>,
	linux-usb@vger.kernel.org,
	"Grant Grundler" <grundler@chromium.org>,
	"Bjørn Mork" <bjorn@mork.no>,
	"Eric Dumazet" <edumazet@google.com>,
	"Paolo Abeni" <pabeni@redhat.com>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH v5 8/8] r8152: Block future register access if register access fails
Date: Fri, 3 Nov 2023 16:52:44 +0000	[thread overview]
Message-ID: <20231103165244.GB714036@kernel.org> (raw)
In-Reply-To: <CAD=FV=XVJVkyA09Ca_YGa5xRS4jGra4cw-6ArgwCekMzn7uWcA@mail.gmail.com>

On Wed, Oct 25, 2023 at 01:24:55PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Oct 25, 2023 at 9:28 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Fri, Oct 20, 2023 at 02:06:59PM -0700, Douglas Anderson wrote:
> >
> > ...
> >
> > > @@ -9603,25 +9713,14 @@ static bool rtl8152_supports_lenovo_macpassthru(struct usb_device *udev)
> > >       return 0;
> > >  }
> > >
> > > -static int rtl8152_probe(struct usb_interface *intf,
> > > -                      const struct usb_device_id *id)
> > > +static int rtl8152_probe_once(struct usb_interface *intf,
> > > +                           const struct usb_device_id *id, u8 version)
> > >  {
> > >       struct usb_device *udev = interface_to_usbdev(intf);
> > >       struct r8152 *tp;
> > >       struct net_device *netdev;
> > > -     u8 version;
> > >       int ret;
> > >
> > > -     if (intf->cur_altsetting->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC)
> > > -             return -ENODEV;
> > > -
> > > -     if (!rtl_check_vendor_ok(intf))
> > > -             return -ENODEV;
> > > -
> > > -     version = rtl8152_get_version(intf);
> > > -     if (version == RTL_VER_UNKNOWN)
> > > -             return -ENODEV;
> > > -
> > >       usb_reset_device(udev);
> > >       netdev = alloc_etherdev(sizeof(struct r8152));
> > >       if (!netdev) {
> > > @@ -9784,10 +9883,20 @@ static int rtl8152_probe(struct usb_interface *intf,
> > >       else
> > >               device_set_wakeup_enable(&udev->dev, false);
> > >
> > > +     /* If we saw a control transfer error while probing then we may
> > > +      * want to try probe() again. Consider this an error.
> > > +      */
> > > +     if (test_bit(PROBE_SHOULD_RETRY, &tp->flags))
> > > +             goto out2;
> >
> > Sorry for being a bit slow here, but if this is an error condition,
> > sould ret be set to an error value?
> >
> > As flagged by Smatch.
> 
> Thanks for the note. I think we're OK, though. If you look at the
> "out:" label, which is right after "out1" it tests for the same bit.
> That will set "ret = -EAGAIN" for us.

Thanks, and sorry for being even slower than the previous time.
I see your point regarding "out:" and agree that the code is correct.

> I'll admit it probably violates the principle of least astonishment,
> but there's a method to my madness. Specifically:
> 
> a) We need a test here to make sure we don't return "success" if the
> bit is set. The driver doesn't error check for success when it
> modifies HW registers so it might _thnk_ it was successful but still
> have this bit set. ...so we need this check right before we return
> "success".
> 
> b) We also need to test for this bit if we're in the error handling
> code. Even though the driver doesn't check for success in lots of
> places, there still could be some places that notice an error. It may
> return any kind of error here, so we need to override it to -EAGAIN.
> 
> ...so I just set "ret = -EAGAIN" in one place.
> 
> Does that make sense? If you want to submit a patch adjusting the
> comment to make this more obvious, I'm happy to review it.

Thanks it does make sense.
And I don't think any further action is required.