[net-next,12/12] r8152: modify the tx timeout funcfion

Reset and reinitialize the device when the tx timeout occurs.

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

On Tue, Mar 4, 2014 at 4:01 AM, Hayes Wang <hayeswang@realtek.com> wrote:
> Reset and reinitialize the device when the tx timeout occurs.

Hayes,
I believe this patch was dropped after the series was split.
Can you please repost this patch by itself?

(and fix the "function" typo in the patch header)

>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>  drivers/net/usb/r8152.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index e04fcbd..23e03a6 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1791,16 +1791,6 @@ 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);
> -}
> -
>  static void rtl8152_set_rx_mode(struct net_device *netdev)
>  {
>         struct r8152 *tp = netdev_priv(netdev);
> @@ -3177,6 +3167,37 @@ out:
>         return res;
>  }
>
> +static void rtl8152_tx_timeout(struct net_device *netdev)
> +{
> +       struct r8152 *tp = netdev_priv(netdev);
> +
> +       netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> +
> +       if (usb_autopm_get_interface(tp->intf) < 0)
> +               return;
> +
> +       netif_stop_queue(netdev);
> +       clear_bit(WORK_ENABLE, &tp->flags);
> +       usb_kill_urb(tp->intr_urb);
> +       cancel_delayed_work_sync(&tp->schedule);
> +       tp->rtl_ops.down(tp);
> +
> +       usb_reset_device(tp->udev);
> +
> +       tp->rtl_ops.init(tp);
> +       tp->rtl_ops.up(tp);
> +       rtl8152_set_speed(tp, AUTONEG_ENABLE,
> +                         tp->mii.supports_gmii ? SPEED_1000 : SPEED_100,
> +                         DUPLEX_FULL);
> +       tp->speed = 0;

Nit: Could rtl_ops.up() set speed since it appears to be changing the
state of the link?

rtl8152_open() uses a remarkably similar code sequence. Is there an
opportunity to refactor and  make sure this sequence is consistent?
(different patch, not this one)

cheers,
grant

> +       netif_carrier_off(netdev);
> +       netif_start_queue(netdev);
> +       set_bit(WORK_ENABLE, &tp->flags);
> +       usb_submit_urb(tp->intr_urb, GFP_KERNEL);
> +
> +       usb_autopm_put_interface(tp->intf);
> +}
> +
>  static const struct net_device_ops rtl8152_netdev_ops = {
>         .ndo_open               = rtl8152_open,
>         .ndo_stop               = rtl8152_close,
> --
> 1.8.4.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
--
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

Grant Grundler [mailto:grundler@google.com] 
> Sent: Wednesday, March 26, 2014 4:12 AM
[...]
> Hayes,
> I believe this patch was dropped after the series was split.
> Can you please repost this patch by itself?

There is no problem for current behavior, and I don't get the
issue of tx timeout, yet. I think the other patches are prior
to this one, so I plan to deal with this one after the others.
Besides, maybe I would have different idea for this one then.
Although reinitialization is more safe, I don't sure if it is
necessary.

> (and fix the "function" typo in the patch header)

oops.

[...]
> Nit: Could rtl_ops.up() set speed since it appears to be changing the
> state of the link?

I don't plan to do it. I don't think it is a part of rtl_ops.up().
Although it alwayes follows rtl_ops.up() now, I think they should
be separeted because they are for different purposes. The set_speed
is used to make sure the speed is correct, because you don't know
what speed it is before the driver is loaded. The other OS may
change the speed, so the device should have opportunity to change
the speed to the default when the driver is loaded. Normally, the
set_speed is not necessary.

> rtl8152_open() uses a remarkably similar code sequence. Is there an
> opportunity to refactor and  make sure this sequence is consistent?
> (different patch, not this one)

It is a good question. I would think about it.
 
Best Regards,
Hayes

--
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