[PATCH net 0/2] r8152: device reset

[PATCH net 0/2] r8152: device reset

[Hayes Wang]

Although the driver works normally, we find the device may get all 0xff data when
tranmitting packets on certain platforms. It would break the device and no packet
could be transmitted. The reset is necessary to recover the hw for this situation.

Hayes Wang (2):
  r8152: add pre_reset and post_reset
  r8152: reset device when tx timeout

 drivers/net/usb/r8152.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 86 insertions(+), 4 deletions(-)

-- 
2.4.2

[PATCH net 1/2] r8152: add pre_reset and post_reset

[Hayes Wang]

Add rtl8152_pre_reset() and rtl8152_post_reset() which are used when
calling usb_reset_device(). The two functions could reduce the time
of reset when calling usb_reset_device() after probe().

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 144dc64..a6caa60 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3342,6 +3342,72 @@ static void r8153_init(struct r8152 *tp)
 	r8153_u2p3en(tp, true);
 }
 
+static int rtl8152_pre_reset(struct usb_interface *intf)
+{
+	struct r8152 *tp = usb_get_intfdata(intf);
+	struct net_device *netdev;
+	int ret;
+
+	if (intf->condition != USB_INTERFACE_BOUND || !tp)
+		return 0;
+
+	netdev = tp->netdev;
+	if (!netif_running(netdev))
+		return 0;
+
+	ret = usb_autopm_get_interface(intf);
+	if (ret < 0)
+		return ret;
+
+	napi_disable(&tp->napi);
+	clear_bit(WORK_ENABLE, &tp->flags);
+	usb_kill_urb(tp->intr_urb);
+	cancel_delayed_work_sync(&tp->schedule);
+	if (netif_carrier_ok(netdev)) {
+		netif_stop_queue(netdev);
+		mutex_lock(&tp->control);
+		tp->rtl_ops.disable(tp);
+		mutex_unlock(&tp->control);
+	}
+
+	usb_autopm_put_interface(intf);
+
+	return 0;
+}
+
+static int rtl8152_post_reset(struct usb_interface *intf)
+{
+	struct r8152 *tp = usb_get_intfdata(intf);
+	struct net_device *netdev;
+	int ret;
+
+	if (intf->condition != USB_INTERFACE_BOUND || !tp)
+		return 0;
+
+	netdev = tp->netdev;
+	if (!netif_running(netdev))
+		return 0;
+
+	ret = usb_autopm_get_interface(intf);
+	if (ret < 0)
+		return ret;
+
+	set_bit(WORK_ENABLE, &tp->flags);
+	if (netif_carrier_ok(netdev)) {
+		mutex_lock(&tp->control);
+		tp->rtl_ops.enable(tp);
+		rtl8152_set_rx_mode(netdev);
+		mutex_unlock(&tp->control);
+		netif_wake_queue(netdev);
+	}
+
+	napi_enable(&tp->napi);
+
+	usb_autopm_put_interface(intf);
+
+	return ret;
+}
+
 static int rtl8152_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct r8152 *tp = usb_get_intfdata(intf);
@@ -4164,6 +4230,8 @@ static struct usb_driver rtl8152_driver = {
 	.suspend =	rtl8152_suspend,
 	.resume =	rtl8152_resume,
 	.reset_resume =	rtl8152_resume,
+	.pre_reset =	rtl8152_pre_reset,
+	.post_reset =	rtl8152_post_reset,
 	.supports_autosuspend = 1,
 	.disable_hub_initiated_lpm = 1,
 };
-- 
2.4.2

Re: [PATCH net 1/2] r8152: add pre_reset and post_reset

[Oliver Neukum]

On Tue, 2015-07-28 at 15:36 +0800, Hayes Wang wrote:
> Add rtl8152_pre_reset() and rtl8152_post_reset() which are used when
> calling usb_reset_device(). The two functions could reduce the time
> of reset when calling usb_reset_device() after probe().
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>  drivers/net/usb/r8152.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 144dc64..a6caa60 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -3342,6 +3342,72 @@ static void r8153_init(struct r8152 *tp)
>  	r8153_u2p3en(tp, true);
>  }
>  
> +static int rtl8152_pre_reset(struct usb_interface *intf)
> +{
> +	struct r8152 *tp = usb_get_intfdata(intf);
> +	struct net_device *netdev;
> +	int ret;
> +
> +	if (intf->condition != USB_INTERFACE_BOUND || !tp)

If the interface weren't bound, you wouldn't be called.

> +		return 0;
> +
> +	netdev = tp->netdev;
> +	if (!netif_running(netdev))
> +		return 0;
> +
> +	ret = usb_autopm_get_interface(intf);
> +	if (ret < 0)
> +		return ret;

What sense does this make?

> +
> +	napi_disable(&tp->napi);
> +	clear_bit(WORK_ENABLE, &tp->flags);
> +	usb_kill_urb(tp->intr_urb);
> +	cancel_delayed_work_sync(&tp->schedule);
> +	if (netif_carrier_ok(netdev)) {
> +		netif_stop_queue(netdev);
> +		mutex_lock(&tp->control);
> +		tp->rtl_ops.disable(tp);
> +		mutex_unlock(&tp->control);
> +	}
> +
> +	usb_autopm_put_interface(intf);
> +
> +	return 0;
> +}
> +
> +static int rtl8152_post_reset(struct usb_interface *intf)
> +{
> +	struct r8152 *tp = usb_get_intfdata(intf);
> +	struct net_device *netdev;
> +	int ret;
> +
> +	if (intf->condition != USB_INTERFACE_BOUND || !tp)

Again unnecessary

> +		return 0;
> +
> +	netdev = tp->netdev;
> +	if (!netif_running(netdev))
> +		return 0;
> +
> +	ret = usb_autopm_get_interface(intf);

The device will be awake.

> +	if (ret < 0)
> +		return ret;
> +
> +	set_bit(WORK_ENABLE, &tp->flags);
> +	if (netif_carrier_ok(netdev)) {
> +		mutex_lock(&tp->control);
> +		tp->rtl_ops.enable(tp);
> +		rtl8152_set_rx_mode(netdev);
> +		mutex_unlock(&tp->control);
> +		netif_wake_queue(netdev);
> +	}
> +
> +	napi_enable(&tp->napi);
> +
> +	usb_autopm_put_interface(intf);
> +
> +	return ret;
> +}
> +

	HTH
		Oliver

RE: [PATCH net 1/2] r8152: add pre_reset and post_reset

[Hayes Wang]

Oliver Neukum [mailto:oneukum@suse.com]
> Sent: Tuesday, July 28, 2015 4:53 PM
[...]
> > +		return 0;
> > +
> > +	netdev = tp->netdev;
> > +	if (!netif_running(netdev))
> > +		return 0;
> > +
> > +	ret = usb_autopm_get_interface(intf);
> > +	if (ret < 0)
> > +		return ret;
> 
> What sense does this make?
> 
[...]
> > +		return 0;
> > +
> > +	netdev = tp->netdev;
> > +	if (!netif_running(netdev))
> > +		return 0;
> > +
> > +	ret = usb_autopm_get_interface(intf);
> 
> The device will be awake.

I don't sure if the device would be in runtimesuspend, so I wake it up by myself.
I think you mean I don't have to do this. I would remove them and resend the
patch. Thanks.

Best Regards,
Hayes

Re: [PATCH net 1/2] r8152: add pre_reset and post_reset

[Oliver Neukum]

On Tue, 2015-07-28 at 09:52 +0000, Hayes Wang wrote:
> Oliver Neukum [mailto:oneukum@suse.com]
> > Sent: Tuesday, July 28, 2015 4:53 PM
> [...]
> > > +		return 0;
> > > +
> > > +	netdev = tp->netdev;
> > > +	if (!netif_running(netdev))
> > > +		return 0;
> > > +
> > > +	ret = usb_autopm_get_interface(intf);
> > > +	if (ret < 0)
> > > +		return ret;
> > 
> > What sense does this make?
> > 
> [...]
> > > +		return 0;
> > > +
> > > +	netdev = tp->netdev;
> > > +	if (!netif_running(netdev))
> > > +		return 0;
> > > +
> > > +	ret = usb_autopm_get_interface(intf);
> > 
> > The device will be awake.
> 
> I don't sure if the device would be in runtimesuspend, so I wake it up by myself.
> I think you mean I don't have to do this. I would remove them and resend the
> patch. Thanks.

Usbcore will resume the device.

	HTH
		Oliver

     A. 
        /* Prevent autosuspend during the reset */
        usb_autoresume_device(udev);

        if (config) {
                for (i = 0; i < config->desc.bNumInterfaces; ++i) {
                        struct usb_interface *cintf = config->interface[i];
                        struct usb_driver *drv;
                        int unbind = 0;

                        if (cintf->dev.driver) {
                                drv = to_usb_driver(cintf->dev.driver);
                                if (drv->pre_reset && drv->post_reset)
                                        unbind = (drv->pre_reset)(cintf);

[PATCH net 2/2] r8152: reset device when tx timeout

[Hayes Wang]

The device reset is necessary if the hw becomes abnormal and stops
transmitting packets.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index a6caa60..9bf6e0c 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -27,7 +27,7 @@
 #include <linux/usb/cdc.h>
 
 /* Version Information */
-#define DRIVER_VERSION "v1.08.0 (2015/01/13)"
+#define DRIVER_VERSION "v1.08.1 (2015/07/28)"
 #define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
 #define DRIVER_DESC "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
 #define MODULENAME "r8152"
@@ -591,6 +591,7 @@ struct r8152 {
 	struct sk_buff_head tx_queue, rx_queue;
 	spinlock_t rx_lock, tx_lock;
 	struct delayed_work schedule;
+	struct delayed_work work_reset;
 	struct mii_if_info mii;
 	struct mutex control;	/* use for hw setting */
 
@@ -1902,11 +1903,11 @@ static void rtl_drop_queued_tx(struct r8152 *tp)
 static void rtl8152_tx_timeout(struct net_device *netdev)
 {
 	struct r8152 *tp = netdev_priv(netdev);
-	int i;
 
 	netif_warn(tp, tx_err, netdev, "Tx timeout\n");
-	for (i = 0; i < RTL8152_MAX_TX; i++)
-		usb_unlink_urb(tp->tx_info[i].urb);
+
+	schedule_delayed_work(&tp->work_reset, 0);
+	cancel_delayed_work(&tp->schedule);
 }
 
 static void rtl8152_set_rx_mode(struct net_device *netdev)
@@ -3408,6 +3409,18 @@ static int rtl8152_post_reset(struct usb_interface *intf)
 	return ret;
 }
 
+static void rtl_hw_reset(struct work_struct *work)
+{
+	struct r8152 *tp = container_of(work, struct r8152, work_reset.work);
+
+	netif_info(tp, drv, tp->netdev, "usb reset device\n");
+
+	if (test_bit(RTL8152_UNPLUG, &tp->flags))
+		return;
+
+	usb_reset_device(tp->udev);
+}
+
 static int rtl8152_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct r8152 *tp = usb_get_intfdata(intf);
@@ -4102,6 +4115,7 @@ static int rtl8152_probe(struct usb_interface *intf,
 
 	mutex_init(&tp->control);
 	INIT_DELAYED_WORK(&tp->schedule, rtl_work_func_t);
+	INIT_DELAYED_WORK(&tp->work_reset, rtl_hw_reset);
 
 	netdev->netdev_ops = &rtl8152_netdev_ops;
 	netdev->watchdog_timeo = RTL8152_TX_TIMEOUT;
-- 
2.4.2

Re: [PATCH net 2/2] r8152: reset device when tx timeout

[Oliver Neukum]

On Tue, 2015-07-28 at 15:36 +0800, Hayes Wang wrote:
> The device reset is necessary if the hw becomes abnormal and stops
> transmitting packets.

You are not the first one to face this problem. Hence there
is a helper:

 * usb_queue_reset_device - Reset a USB device from an atomic context
 * @iface: USB interface belonging to the device to reset
 *
 * This function can be used to reset a USB device from an atomic
 * context, where usb_reset_device() won't work (as it blocks).

Please use it if you can. Your version for example is buggy.
It will oops if you unplug the device while a reset is scheduled.

	Regards
		Oliver

RE: [PATCH net 2/2] r8152: reset device when tx timeout

[Hayes Wang]

Oliver Neukum [mailto:oneukum@suse.com]
> Sent: Tuesday, July 28, 2015 4:57 PM
[...]
>  * usb_queue_reset_device - Reset a USB device from an atomic context
>  * @iface: USB interface belonging to the device to reset
>  *
>  * This function can be used to reset a USB device from an atomic
>  * context, where usb_reset_device() won't work (as it blocks).
> 
> Please use it if you can. Your version for example is buggy.
> It will oops if you unplug the device while a reset is scheduled.

Thanks for your suggestion. I would replace it. 

Best Regards,
Hayes

[PATCH net v2 0/2] r8152: device reset

[Hayes Wang]

v2:
For patch #1, remove usb_autopm_get_interface(), usb_autopm_put_interface(), and
the checking of intf->condition.

For patch #2, replace the original method with usb_queue_reset_device() to reset
the device. 

v1:
Although the driver works normally, we find the device may get all 0xff data when
transmitting packets on certain platforms. It would break the device and no packet
could be transmitted. The reset is necessary to recover the hw for this situation.

Hayes Wang (2):
  r8152: add pre_reset and post_reset
  r8152: reset device when tx timeout

 drivers/net/usb/r8152.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 86 insertions(+), 4 deletions(-)

-- 
2.4.2

[PATCH net v2 1/2] r8152: add pre_reset and post_reset

[Hayes Wang]

Add rtl8152_pre_reset() and rtl8152_post_reset() which are used when
calling usb_reset_device(). The two functions could reduce the time
of reset when calling usb_reset_device() after probe().

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 144dc64..e1b6d6d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3342,6 +3342,58 @@ static void r8153_init(struct r8152 *tp)
 	r8153_u2p3en(tp, true);
 }
 
+static int rtl8152_pre_reset(struct usb_interface *intf)
+{
+	struct r8152 *tp = usb_get_intfdata(intf);
+	struct net_device *netdev;
+
+	if (!tp)
+		return 0;
+
+	netdev = tp->netdev;
+	if (!netif_running(netdev))
+		return 0;
+
+	napi_disable(&tp->napi);
+	clear_bit(WORK_ENABLE, &tp->flags);
+	usb_kill_urb(tp->intr_urb);
+	cancel_delayed_work_sync(&tp->schedule);
+	if (netif_carrier_ok(netdev)) {
+		netif_stop_queue(netdev);
+		mutex_lock(&tp->control);
+		tp->rtl_ops.disable(tp);
+		mutex_unlock(&tp->control);
+	}
+
+	return 0;
+}
+
+static int rtl8152_post_reset(struct usb_interface *intf)
+{
+	struct r8152 *tp = usb_get_intfdata(intf);
+	struct net_device *netdev;
+
+	if (!tp)
+		return 0;
+
+	netdev = tp->netdev;
+	if (!netif_running(netdev))
+		return 0;
+
+	set_bit(WORK_ENABLE, &tp->flags);
+	if (netif_carrier_ok(netdev)) {
+		mutex_lock(&tp->control);
+		tp->rtl_ops.enable(tp);
+		rtl8152_set_rx_mode(netdev);
+		mutex_unlock(&tp->control);
+		netif_wake_queue(netdev);
+	}
+
+	napi_enable(&tp->napi);
+
+	return 0;
+}
+
 static int rtl8152_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct r8152 *tp = usb_get_intfdata(intf);
@@ -4164,6 +4216,8 @@ static struct usb_driver rtl8152_driver = {
 	.suspend =	rtl8152_suspend,
 	.resume =	rtl8152_resume,
 	.reset_resume =	rtl8152_resume,
+	.pre_reset =	rtl8152_pre_reset,
+	.post_reset =	rtl8152_post_reset,
 	.supports_autosuspend = 1,
 	.disable_hub_initiated_lpm = 1,
 };
-- 
2.4.2

[PATCH net v2 2/2] r8152: reset device when tx timeout

[Hayes Wang]

The device reset is necessary if the hw becomes abnormal and stops
transmitting packets.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index e1b6d6d..6af299f 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -27,7 +27,7 @@
 #include <linux/usb/cdc.h>
 
 /* Version Information */
-#define DRIVER_VERSION "v1.08.0 (2015/01/13)"
+#define DRIVER_VERSION "v1.08.1 (2015/07/28)"
 #define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
 #define DRIVER_DESC "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
 #define MODULENAME "r8152"
@@ -1902,11 +1902,11 @@ static void rtl_drop_queued_tx(struct r8152 *tp)
 static void rtl8152_tx_timeout(struct net_device *netdev)
 {
 	struct r8152 *tp = netdev_priv(netdev);
-	int i;
 
 	netif_warn(tp, tx_err, netdev, "Tx timeout\n");
-	for (i = 0; i < RTL8152_MAX_TX; i++)
-		usb_unlink_urb(tp->tx_info[i].urb);
+
+	usb_queue_reset_device(tp->intf);
+	cancel_delayed_work(&tp->schedule);
 }
 
 static void rtl8152_set_rx_mode(struct net_device *netdev)
-- 
2.4.2

Re: [PATCH net v2 2/2] r8152: reset device when tx timeout

[Oliver Neukum]

On Tue, 2015-07-28 at 20:08 +0800, Hayes Wang wrote:
>  static void rtl8152_tx_timeout(struct net_device *netdev)
>  {
>         struct r8152 *tp = netdev_priv(netdev);
> -       int i;
>  
>         netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> -       for (i = 0; i < RTL8152_MAX_TX; i++)
> -               usb_unlink_urb(tp->tx_info[i].urb);
> +
> +       usb_queue_reset_device(tp->intf);
> +       cancel_delayed_work(&tp->schedule);

Sorry to bother you again, but this looks wrong.
You want to cancel first. There is no point in
running any work before the reset is done. It will
undo any progress anyway.

	Regards
		Oliver

RE: [PATCH net v2 2/2] r8152: reset device when tx timeout

[Hayes Wang]

Oliver Neukum [mailto:oneukum@suse.com]
> Sent: Tuesday, July 28, 2015 8:14 PM
[...]
> >  static void rtl8152_tx_timeout(struct net_device *netdev)  {
> >         struct r8152 *tp = netdev_priv(netdev);
> > -       int i;
> >
> >         netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> > -       for (i = 0; i < RTL8152_MAX_TX; i++)
> > -               usb_unlink_urb(tp->tx_info[i].urb);
> > +
> > +       usb_queue_reset_device(tp->intf);
> > +       cancel_delayed_work(&tp->schedule);
> 
> Sorry to bother you again, but this looks wrong.
> You want to cancel first. There is no point in running any work before the reset is
> done. It will undo any progress anyway.

Excuse me. Do you mean I don't need cancel the other work because it wouldn't be
run before the reset is finished?

Best Regards,
Hayes

Re: [PATCH net v2 2/2] r8152: reset device when tx timeout

[Oliver Neukum]

On Tue, 2015-07-28 at 12:31 +0000, Hayes Wang wrote:
> Oliver Neukum [mailto:oneukum@suse.com]
> > Sent: Tuesday, July 28, 2015 8:14 PM
> [...]
> > >  static void rtl8152_tx_timeout(struct net_device *netdev)  {
> > >         struct r8152 *tp = netdev_priv(netdev);
> > > -       int i;
> > >
> > >         netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> > > -       for (i = 0; i < RTL8152_MAX_TX; i++)
> > > -               usb_unlink_urb(tp->tx_info[i].urb);
> > > +
> > > +       usb_queue_reset_device(tp->intf);
> > > +       cancel_delayed_work(&tp->schedule);
> > 
> > Sorry to bother you again, but this looks wrong.
> > You want to cancel first. There is no point in running any work before the reset is
> > done. It will undo any progress anyway.
> 
> Excuse me. Do you mean I don't need cancel the other work because it wouldn't be
> run before the reset is finished?

No, whatever the other work will do, the reset will undo.

	Regards
		Oliver

RE: [PATCH net v2 2/2] r8152: reset device when tx timeout

[Hayes Wang]

 Oliver Neukum [mailto:oneukum@suse.com]
> Sent: Tuesday, July 28, 2015 8:59 PM
[...]
> > > >  static void rtl8152_tx_timeout(struct net_device *netdev)  {
> > > >         struct r8152 *tp = netdev_priv(netdev);
> > > > -       int i;
> > > >
> > > >         netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> > > > -       for (i = 0; i < RTL8152_MAX_TX; i++)
> > > > -               usb_unlink_urb(tp->tx_info[i].urb);
> > > > +
> > > > +       usb_queue_reset_device(tp->intf);
> > > > +       cancel_delayed_work(&tp->schedule);
> > >
> > > Sorry to bother you again, but this looks wrong.
> > > You want to cancel first. There is no point in running any work
> > > before the reset is done. It will undo any progress anyway.
> >
> > Excuse me. Do you mean I don't need cancel the other work because it
> > wouldn't be run before the reset is finished?
> 
> No, whatever the other work will do, the reset will undo.

Excuse me. I don't understand why I couldn't use usb_queue_reset_device() directly.
Why the reset will undo? 

Best Regards,
Hayes

Re: [PATCH net v2 2/2] r8152: reset device when tx timeout

[Oliver Neukum]

On Wed, 2015-07-29 at 02:06 +0000, Hayes Wang wrote:
>  Oliver Neukum [mailto:oneukum@suse.com]
> > Sent: Tuesday, July 28, 2015 8:59 PM
> [...]
> > > > >  static void rtl8152_tx_timeout(struct net_device *netdev)  {
> > > > >         struct r8152 *tp = netdev_priv(netdev);
> > > > > -       int i;
> > > > >
> > > > >         netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> > > > > -       for (i = 0; i < RTL8152_MAX_TX; i++)
> > > > > -               usb_unlink_urb(tp->tx_info[i].urb);
> > > > > +
> > > > > +       usb_queue_reset_device(tp->intf);
> > > > > +       cancel_delayed_work(&tp->schedule);
> > > >
> > > > Sorry to bother you again, but this looks wrong.
> > > > You want to cancel first. There is no point in running any work
> > > > before the reset is done. It will undo any progress anyway.
> > >
> > > Excuse me. Do you mean I don't need cancel the other work because it
> > > wouldn't be run before the reset is finished?
> > 
> > No, whatever the other work will do, the reset will undo.
> 
> Excuse me. I don't understand why I couldn't use usb_queue_reset_device() directly.
> Why the reset will undo? 

Now, I think I got the reason for the confusion.

You are using cancel_delayed_work(&tp->schedule); after you queue
a reset. Therefore the order in which the work and the reset will
be executed is undefined. Usually the scheduled work will be canceled,
but not always.

That is not good.

	Regards
		Oliver

RE: [PATCH net v2 2/2] r8152: reset device when tx timeout

[Hayes Wang]

Oliver Neukum [mailto:oneukum@suse.com]
> Sent: Wednesday, July 29, 2015 3:22 PM
[...]
> Now, I think I got the reason for the confusion.
> 
> You are using cancel_delayed_work(&tp->schedule); after you queue a reset.
> Therefore the order in which the work and the reset will be executed is undefined.
> Usually the scheduled work will be canceled, but not always.

Actually, the order is not important for me. There are some protections to avoid
the work and the reset run at the same time. Besides, the reset could be run before
or after the work. I cancel the work because I want to let the reset start as early as
possible. If the work runs before the reset, the reset wouldn't start until the work is
finished.

> That is not good.

I think I had better to remove cancel_delay_work(), because it makes confusion.
And, it doesn't seem to be necessary. Thanks. 

Best Regards,
Hayes

[PATCH net v3 0/2] r8152: device reset

[Hayes Wang]

v3:
For patch #2, remove cancel_delayed_work().

v2:
For patch #1, remove usb_autopm_get_interface(), usb_autopm_put_interface(), and
the checking of intf->condition.

For patch #2, replace the original method with usb_queue_reset_device() to reset
the device. 

v1:
Although the driver works normally, we find the device may get all 0xff data when
transmitting packets on certain platforms. It would break the device and no packet
could be transmitted. The reset is necessary to recover the hw for this situation.

Hayes Wang (2):
  r8152: add pre_reset and post_reset
  r8152: reset device when tx timeout

 drivers/net/usb/r8152.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 86 insertions(+), 4 deletions(-)

-- 
2.4.2

[PATCH net v3 1/2] r8152: add pre_reset and post_reset

[Hayes Wang]

Add rtl8152_pre_reset() and rtl8152_post_reset() which are used when
calling usb_reset_device(). The two functions could reduce the time
of reset when calling usb_reset_device() after probe().

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 144dc64..e1b6d6d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3342,6 +3342,58 @@ static void r8153_init(struct r8152 *tp)
 	r8153_u2p3en(tp, true);
 }
 
+static int rtl8152_pre_reset(struct usb_interface *intf)
+{
+	struct r8152 *tp = usb_get_intfdata(intf);
+	struct net_device *netdev;
+
+	if (!tp)
+		return 0;
+
+	netdev = tp->netdev;
+	if (!netif_running(netdev))
+		return 0;
+
+	napi_disable(&tp->napi);
+	clear_bit(WORK_ENABLE, &tp->flags);
+	usb_kill_urb(tp->intr_urb);
+	cancel_delayed_work_sync(&tp->schedule);
+	if (netif_carrier_ok(netdev)) {
+		netif_stop_queue(netdev);
+		mutex_lock(&tp->control);
+		tp->rtl_ops.disable(tp);
+		mutex_unlock(&tp->control);
+	}
+
+	return 0;
+}
+
+static int rtl8152_post_reset(struct usb_interface *intf)
+{
+	struct r8152 *tp = usb_get_intfdata(intf);
+	struct net_device *netdev;
+
+	if (!tp)
+		return 0;
+
+	netdev = tp->netdev;
+	if (!netif_running(netdev))
+		return 0;
+
+	set_bit(WORK_ENABLE, &tp->flags);
+	if (netif_carrier_ok(netdev)) {
+		mutex_lock(&tp->control);
+		tp->rtl_ops.enable(tp);
+		rtl8152_set_rx_mode(netdev);
+		mutex_unlock(&tp->control);
+		netif_wake_queue(netdev);
+	}
+
+	napi_enable(&tp->napi);
+
+	return 0;
+}
+
 static int rtl8152_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct r8152 *tp = usb_get_intfdata(intf);
@@ -4164,6 +4216,8 @@ static struct usb_driver rtl8152_driver = {
 	.suspend =	rtl8152_suspend,
 	.resume =	rtl8152_resume,
 	.reset_resume =	rtl8152_resume,
+	.pre_reset =	rtl8152_pre_reset,
+	.post_reset =	rtl8152_post_reset,
 	.supports_autosuspend = 1,
 	.disable_hub_initiated_lpm = 1,
 };
-- 
2.4.2

[PATCH net v3 2/2] r8152: reset device when tx timeout

[Hayes Wang]

The device reset is necessary if the hw becomes abnormal and stops
transmitting packets.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index e1b6d6d..ad8cbc6 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -27,7 +27,7 @@
 #include <linux/usb/cdc.h>
 
 /* Version Information */
-#define DRIVER_VERSION "v1.08.0 (2015/01/13)"
+#define DRIVER_VERSION "v1.08.1 (2015/07/28)"
 #define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
 #define DRIVER_DESC "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
 #define MODULENAME "r8152"
@@ -1902,11 +1902,10 @@ static void rtl_drop_queued_tx(struct r8152 *tp)
 static void rtl8152_tx_timeout(struct net_device *netdev)
 {
 	struct r8152 *tp = netdev_priv(netdev);
-	int i;
 
 	netif_warn(tp, tx_err, netdev, "Tx timeout\n");
-	for (i = 0; i < RTL8152_MAX_TX; i++)
-		usb_unlink_urb(tp->tx_info[i].urb);
+
+	usb_queue_reset_device(tp->intf);
 }
 
 static void rtl8152_set_rx_mode(struct net_device *netdev)
-- 
2.4.2

Re: [PATCH net v3 0/2] r8152: device reset

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
Date: Wed, 29 Jul 2015 20:39:07 +0800

> v3:
> For patch #2, remove cancel_delayed_work().
> 
> v2:
> For patch #1, remove usb_autopm_get_interface(), usb_autopm_put_interface(), and
> the checking of intf->condition.
> 
> For patch #2, replace the original method with usb_queue_reset_device() to reset
> the device. 
> 
> v1:
> Although the driver works normally, we find the device may get all 0xff data when
> transmitting packets on certain platforms. It would break the device and no packet
> could be transmitted. The reset is necessary to recover the hw for this situation.

Series applied, thanks.