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@xxxxxxxxxx> 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.
