Hi,
On Mon, Oct 16, 2023 at 2:16 AM Hayes Wang <hayeswang@xxxxxxxxxxx> wrote:
>
> Douglas Anderson <dianders@xxxxxxxxxxxx>
> > Sent: Friday, October 13, 2023 3:25 AM
> [...]
> > static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
> > @@ -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 the end of
> > + * probe without any register access fails, which sets
> > + * `PROBED_WITH_NO_ERRORS` to true. If we didn't have that then return
> > + * an error here which tells the USB framework to fully unbind/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.
I can update the comment in __rtl_get_hw_ver() to make it more obvious
that this is by design?
>
> > + */
> > + mutex_lock(&tp->control);
>
> I don't think you need the mutex for testing the bit.
Sure, I'll remove it.
> > + if (!test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) {
> > + mutex_unlock(&tp->control);
> > + return -EIO;
> > + }
> > + mutex_unlock(&tp->control);
> > +
> > netdev = tp->netdev;
> > if (!netif_running(netdev))
> > return 0;
> > @@ -8277,7 +8378,9 @@ static int rtl8152_pre_reset(struct usb_interface
> > *intf)
> > napi_disable(&tp->napi);
> > if (netif_carrier_ok(netdev)) {
> > mutex_lock(&tp->control);
> > + set_bit(IN_PRE_RESET, &tp->flags);
> > tp->rtl_ops.disable(tp);
> > + clear_bit(IN_PRE_RESET, &tp->flags);
> > mutex_unlock(&tp->control);
> > }
> >
> > @@ -8293,6 +8396,10 @@ static int rtl8152_post_reset(struct usb_interface
> > *intf)
> > if (!tp)
> > return 0;
> >
> > + mutex_lock(&tp->control);
>
> I don't think clear_bit() needs the protection of mutex.
> I think you could call rtl_set_accessible() directly.
Agreed, I'll take this out.
Unless something else comes up, I'll send a new version tomorrow with
the above small changes.
-Doug
