Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
From:   Hayes Wang <hayeswang@realtek.com>
To:     Douglas Anderson <dianders@chromium.org>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S . Miller" <davem@davemloft.net>
CC:     Grant Grundler <grundler@chromium.org>,
        Edward Hill <ecgh@chromium.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Simon Horman <horms@kernel.org>,
        Laura Nao <laura.nao@collabora.com>,
        Alan Stern <stern@rowland.harvard.edu>,
        =?iso-8859-1?Q?Bj=F8rn_Mork?= <bjorn@mork.no>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: RE: [PATCH v4 5/5] r8152: Block future register access if register access fails
Thread-Topic: [PATCH v4 5/5] r8152: Block future register access if register
 access fails
Thread-Index: AQHaAtJRIbIHTuR1b0yzMA85nEY4kLBSW1lw
Date:   Fri, 20 Oct 2023 11:31:16 +0000
Message-ID: <eaf05cf1486c418790a1b54cbcda3a98@realtek.com>
References: <20231019212130.3146151-1-dianders@chromium.org>
 <20231019142019.v4.5.Ib2affdbfdc2527aaeef9b46d4f23f7c04147faeb@changeid>
In-Reply-To: <20231019142019.v4.5.Ib2affdbfdc2527aaeef9b46d4f23f7c04147faeb@changeid>
Accept-Language: zh-TW, en-US
Content-Language: zh-TW
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Precedence: bulk

Douglas Anderson <dianders@chromium.org>
> Sent: Friday, October 20, 2023 5:20 AM
[...]
>  static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
> @@ -8265,6 +8353,17 @@ 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.
> +        */
> +       if (!test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) {
> +               mutex_unlock(&tp->control);

I think you forget to remove mutex_unlock here.

> +               return -EIO;
> +       }
> +
>         netdev = tp->netdev;
>         if (!netif_running(netdev))
>                 return 0;
> @@ -8277,7 +8376,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 +8394,8 @@ static int rtl8152_post_reset(struct usb_interface *intf)
>         if (!tp)
>                 return 0;
> 
> +       rtl_set_accessible(tp);
> +

Excuse me. I have a new idea. You could check if it is possible.
If you remove test_bit(PROBED_WITH_NO_ERRORS, &tp->flags) in pre_reset(),
the driver wouldn't be unbound and rebound. Instead, you test PROBED_WITH_NO_ERRORS
here to re-initialize the device. Then, you could limit the times of USB reset, and
the infinite loop wouldn't occur. The code would be like the following,

	if (!test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) {
		/* re-init */
		mutex_lock(&tp->control);
		tp->rtl_ops.init(tp);
		mutex_unlock(&tp->control);
		rtl_hw_phy_work_func_t(&tp->hw_phy_work.work);

		/* re-open(). Maybe move after checking netif_running(netdev) */
		mutex_lock(&tp->control);
		tp->rtl_ops.up(tp);
		mutex_unlock(&tp->control);

		/* check if there is any control error */
		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags) {
			if (tp->reg_access_reset_count < REGISTER_ACCESS_MAX_RESETS) {
				/* queue reset again ? */
			} else {
				...
			}
			/* return 0 ? */
		} else {
			set_bit(PROBED_WITH_NO_ERRORS, &tp->flags)
		}
	}


Best Regards,
Hayes

>         /* reset the MAC address in case of policy change */
>         if (determine_ethernet_addr(tp, &sa) >= 0) {
>                 rtnl_lock();
> @@ -9494,17 +9597,35 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev)
>         __le32 *tmp;
>         u8 version;
>         int ret;
> +       int i;
> 
>         tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
>         if (!tmp)
>                 return 0;
> 
> -       ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> -                             RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> -                             PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp),
> -                             USB_CTRL_GET_TIMEOUT);
> -       if (ret > 0)
> -               ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
> +       /* Retry up to 3 times in case there is a transitory error. We do this
> +        * since retrying a read of the version is always safe and this
> +        * function doesn't take advantage of r8152_control_msg() which would
> +        * queue up a reset upon error.
> +        *
> +        * NOTE: The fact that this read never queues up a reset prevents us
> +        * from getting into a unbind/bind loop if usb_control_msg() fails
> +        * 100% of the time. This is the first control message we do at
> +        * probe time and 3 failures in a row here will cause probe to fail.
> +        */
> +       for (i = 0; i < 3; i++) {
> +               ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> +                                     RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> +                                     PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp),
> +                                     USB_CTRL_GET_TIMEOUT);
> +               if (ret > 0) {
> +                       ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
> +                       break;
> +               }
> +       }
> +
> +       if (i != 0 && ret > 0)
> +               dev_warn(&udev->dev, "Needed %d retries to read version\n", i);
> 
>         kfree(tmp);
> 
> @@ -9784,7 +9905,29 @@ static int rtl8152_probe(struct usb_interface *intf,
>         else
>                 device_set_wakeup_enable(&udev->dev, false);
> 
> -       netif_info(tp, probe, netdev, "%s\n", DRIVER_VERSION);
> +       mutex_lock(&tp->control);
> +       if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) {
> +               /* If the device is marked inaccessible before probe even
> +                * finished then one of two things happened. Either we got a
> +                * USB error during probe or the user already unplugged the
> +                * device.
> +                *
> +                * If we got a USB error during probe then we skipped doing a
> +                * reset in r8152_control_msg() and deferred it to here. This
> +                * is because the queued reset will give up after 1 second
> +                * (see usb_lock_device_for_reset()) and we want to make sure
> +                * that we queue things up right before probe finishes.
> +                *
> +                * If the user already unplugged the device then the USB
> +                * framework will call unbind right away for us. The extra
> +                * reset we queue up here will be harmless.
> +                */
> +               usb_queue_reset_device(tp->intf);
> +       } else {
> +               set_bit(PROBED_WITH_NO_ERRORS, &tp->flags);
> +               netif_info(tp, probe, netdev, "%s\n", DRIVER_VERSION);
> +       }
> +       mutex_unlock(&tp->control);
> 
>         return 0;
> 
> --
> 2.42.0.758.gaed0368e0e-goog