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().
> + */
> + mutex_lock(&tp->control);
I don't think you need the mutex for testing the bit.
> + 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.
> + rtl_set_accessible(tp);
> + mutex_unlock(&tp->control);
> +
> /* reset the MAC address in case of policy change */
> if (determine_ethernet_addr(tp, &sa) >= 0) {
> rtnl_lock();
Best Regards,
Hayes
