| Message ID | 1394712342-15778-161-Taiwan-albertk@realtek.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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)
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(-)