RE: [PATCH net-next 12/12] r8152: modify the tx timeout funcfion
Grant Grundler [mailto:grund...@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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 12/12] r8152: modify the tx timeout funcfion
Grant Grundler [mailto:grund...@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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v3 1/2] r8152:addRTL8152_EARLY_AGG_TIMEOUT_SUPER
From: Francois Romieu [mailto:rom...@fr.zoreil.com] > Sent: Saturday, March 15, 2014 7:43 AM [...] > > Besides, I don't wish to modify the setting by ethtool when re-loading > > the driver or rebooting every time. > > Why ? > > The recipe is different but there isn't much setup difference > between a module param and an ethtool command that is run through udev. > The latter is more versatile though. Thanks for your suggestion. I think I have to understand how to use them first. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v3 1/2]r8152:addRTL8152_EARLY_AGG_TIMEOUT_SUPER
From: David Miller [mailto:da...@davemloft.net] > Sent: Saturday, March 15, 2014 2:43 AM [...] > > Besides, I don't wish to modify the setting by ethtool when re-loading > > the driver or rebooting every time. > > You have code to reset the driver, you can do it when the user asks > for the setting to be changed via ethtool. I do not see this as > a problem. > > The ethtool change can occur while the driver is already up, you'll > just need to reset the chip and make the new configuration, this is > not a problem. Thanks for your answer. I would study how to set it by ethtool. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v3 1/2]r8152:addRTL8152_EARLY_AGG_TIMEOUT_SUPER
From: David Miller [mailto:da...@davemloft.net] Sent: Saturday, March 15, 2014 2:43 AM [...] Besides, I don't wish to modify the setting by ethtool when re-loading the driver or rebooting every time. You have code to reset the driver, you can do it when the user asks for the setting to be changed via ethtool. I do not see this as a problem. The ethtool change can occur while the driver is already up, you'll just need to reset the chip and make the new configuration, this is not a problem. Thanks for your answer. I would study how to set it by ethtool. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v3 1/2] r8152:addRTL8152_EARLY_AGG_TIMEOUT_SUPER
From: Francois Romieu [mailto:rom...@fr.zoreil.com] Sent: Saturday, March 15, 2014 7:43 AM [...] Besides, I don't wish to modify the setting by ethtool when re-loading the driver or rebooting every time. Why ? The recipe is different but there isn't much setup difference between a module param and an ethtool command that is run through udev. The latter is more versatile though. Thanks for your suggestion. I think I have to understand how to use them first. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v3 1/2] r8152:addRTL8152_EARLY_AGG_TIMEOUT_SUPER
From: David Miller [mailto:da...@davemloft.net] Sent: Friday, March 14, 2014 12:08 PM [...] > >> And I fundamentally disagree with this being a Kconfig parameter. > >> > >> Make it run-time calculated _or_ settable via ethtool. > > > > Excuse me. How should I make it run-time calculated without a > > Kconfig parameter? Should I use module_param? > > You run-time determine the setting based upon the negotiated link > speed and traffic patterns. It is difficult to design a algorithm which considers the hardware of the platform, network traffic, and even the USB behavior to dynamically modify the setting. I don't think I have the capability to do it. Besides, I don't wish to modify the setting by ethtool when re-loading the driver or rebooting every time. Excuse me. Why is it not accepted for being a Kconfig parameter. It let the manufactuers of some platforms, especially the embedded system, could tune their performance. Should I give up this patch? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v3 1/2] r8152:addRTL8152_EARLY_AGG_TIMEOUT_SUPER
From: David Miller [mailto:da...@davemloft.net] Sent: Friday, March 14, 2014 12:08 PM [...] And I fundamentally disagree with this being a Kconfig parameter. Make it run-time calculated _or_ settable via ethtool. Excuse me. How should I make it run-time calculated without a Kconfig parameter? Should I use module_param? You run-time determine the setting based upon the negotiated link speed and traffic patterns. It is difficult to design a algorithm which considers the hardware of the platform, network traffic, and even the USB behavior to dynamically modify the setting. I don't think I have the capability to do it. Besides, I don't wish to modify the setting by ethtool when re-loading the driver or rebooting every time. Excuse me. Why is it not accepted for being a Kconfig parameter. It let the manufactuers of some platforms, especially the embedded system, could tune their performance. Should I give up this patch? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v3 1/2] r8152: addRTL8152_EARLY_AGG_TIMEOUT_SUPER
From: David Miller [mailto:da...@davemloft.net] Sent: Friday, March 14, 2014 1:22 AM [...] > And I fundamentally disagree with this being a Kconfig parameter. > > Make it run-time calculated _or_ settable via ethtool. Excuse me. How should I make it run-time calculated without a Kconfig parameter? Should I use module_param? Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v3 1/2] r8152: addRTL8152_EARLY_AGG_TIMEOUT_SUPER
From: David Miller [mailto:da...@davemloft.net] Sent: Friday, March 14, 2014 1:22 AM [...] And I fundamentally disagree with this being a Kconfig parameter. Make it run-time calculated _or_ settable via ethtool. Excuse me. How should I make it run-time calculated without a Kconfig parameter? Should I use module_param? Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 0/7] r8152: tx/rx improvement
David Miller [mailto:da...@davemloft.net]
> Sent: Saturday, March 08, 2014 5:28 AM
> To: hayesw...@realtek.com
> Cc: net...@vger.kernel.org; nic_s...@realtek.com;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH net-next 0/7] r8152: tx/rx improvement
[...]
> Note that if you ever add ->ndo_poll_controller support to
> this driver,
> you will have to revert your spin_lock_irq{save,restore}() changes to
> your ->ndo_start_xmit.
>
> Because the transmit function can indeed be invoked from hard IRQ
> context once you support netpoll.
Thank you for your reminder. I would notice that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 0/7] r8152: tx/rx improvement
David Miller [mailto:da...@davemloft.net]
Sent: Saturday, March 08, 2014 5:28 AM
To: hayesw...@realtek.com
Cc: net...@vger.kernel.org; nic_s...@realtek.com;
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH net-next 0/7] r8152: tx/rx improvement
[...]
Note that if you ever add -ndo_poll_controller support to
this driver,
you will have to revert your spin_lock_irq{save,restore}() changes to
your -ndo_start_xmit.
Because the transmit function can indeed be invoked from hard IRQ
context once you support netpoll.
Thank you for your reminder. I would notice that.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 08/12] r8152: support TSO
David Laight [mailto:david.lai...@aculab.com] > Sent: Tuesday, March 04, 2014 8:12 PM > To: 'Hayes Wang'; net...@vger.kernel.org > Cc: nic_s...@realtek.com; linux-kernel@vger.kernel.org; > linux-...@vger.kernel.org > Subject: RE: [PATCH net-next 08/12] r8152: support TSO > > From: Hayes Wang > > Support scatter gather and TSO. > > > > Adjust the tx checksum function and set the max gso size to fix the > > size of the tx aggregation buffer. > > There is little point supporting TSO unless the usb host controller > supports arbitrary aligned scatter-gather. > All you do is require that a large skb be allocated (that may well > fail), and add it another data copy. I think I have done it. For also working for EHCI usb host controller, I allocate 16 KB continuous buffer and copy the fragmented packets to it and bulk out the buffer. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 08/12] r8152: support TSO
David Laight [mailto:david.lai...@aculab.com] Sent: Tuesday, March 04, 2014 8:12 PM To: 'Hayes Wang'; net...@vger.kernel.org Cc: nic_s...@realtek.com; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: RE: [PATCH net-next 08/12] r8152: support TSO From: Hayes Wang Support scatter gather and TSO. Adjust the tx checksum function and set the max gso size to fix the size of the tx aggregation buffer. There is little point supporting TSO unless the usb host controller supports arbitrary aligned scatter-gather. All you do is require that a large skb be allocated (that may well fail), and add it another data copy. I think I have done it. For also working for EHCI usb host controller, I allocate 16 KB continuous buffer and copy the fragmented packets to it and bulk out the buffer. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 12/14] r8152: replace netif_rxwithnetif_receive_skb
Francois Romieu [mailto:rom...@fr.zoreil.com] > Sent: Wednesday, February 19, 2014 3:47 PM > To: hayeswang > Cc: net...@vger.kernel.org; nic_s...@realtek.com; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH net-next 12/14] r8152: replace > netif_rxwithnetif_receive_skb > [...] > The change in rx_bottom is fine. My point is about read_bulk_callback. > > rx_bottom races with read_bulk_callback. rx_bottom is issued in > tasklet (softirq) context. read_bulk_callback is issued in irq > context, with irq disabled. read_bulk_callback does not need to > disable irq itself and could go with spin_lock in place of > spin_lock_irqsave (rx_bottom can't, of course). I think I misunderstand your meaning. I would modify them. Thanks. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 12/14] r8152: replace netif_rxwithnetif_receive_skb
Francois Romieu [mailto:rom...@fr.zoreil.com] Sent: Wednesday, February 19, 2014 3:47 PM To: hayeswang Cc: net...@vger.kernel.org; nic_s...@realtek.com; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH net-next 12/14] r8152: replace netif_rxwithnetif_receive_skb [...] The change in rx_bottom is fine. My point is about read_bulk_callback. rx_bottom races with read_bulk_callback. rx_bottom is issued in tasklet (softirq) context. read_bulk_callback is issued in irq context, with irq disabled. read_bulk_callback does not need to disable irq itself and could go with spin_lock in place of spin_lock_irqsave (rx_bottom can't, of course). I think I misunderstand your meaning. I would modify them. Thanks. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 12/14] r8152: replace netif_rx withnetif_receive_skb
Francois Romieu [mailto:rom...@fr.zoreil.com] > Sent: Wednesday, February 19, 2014 7:29 AM > To: Hayes Wang > Cc: net...@vger.kernel.org; nic_s...@realtek.com; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH net-next 12/14] r8152: replace netif_rx > withnetif_receive_skb > > Hayes Wang : > > Replace netif_rx with netif_receive_skb to avoid disabling irq frequently > > for increasing the efficiency. > > read_bulk_callback is issued in irq context. It could thus use plain > spin_lock / spin_unlock instead of the irq disabling version. The rx_bottom() is called in tasklet, so I just think I could use netif_receive_skb directly. The netif_rx seems to queue the packet, and local_irq_disable() would be called before dequeuing the skb. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 07/14] r8152: combine PHY reset with set_speed
Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: Wednesday, February 19, 2014 1:19 AM
> To: Hayes Wang
> Cc: netdev; nic_s...@realtek.com;
> linux-kernel@vger.kernel.org; linux-usb
> Subject: Re: [PATCH net-next 07/14] r8152: combine PHY reset
> with set_speed
[...]
> > +static void rtl_phy_reset(struct r8152 *tp)
> > +{
> > + u16 data;
> > + int i;
> > +
> > + clear_bit(PHY_RESET, >flags);
> > +
> > + data = r8152_mdio_read(tp, MII_BMCR);
> > +
> > + /* don't reset again before the previous one complete */
> > + if (data & BMCR_RESET)
> > + return;
> > +
> > + data |= BMCR_RESET;
> > + r8152_mdio_write(tp, MII_BMCR, data);
> > +
> > + for (i = 0; i < 50; i++) {
> > + msleep(20);
> > + if ((r8152_mdio_read(tp, MII_BMCR) &
> BMCR_RESET) == 0)
> > + break;
> > + }
> > +}
>
> If you implemented libphy in the driver you would not have to
> duplicate that and you could use "phy_init_hw()" or
> genphy_soft_reset() to perform the BMCR-based software reset.
Thanks for you suggestion. I would study about those.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 07/14] r8152: combine PHY reset with set_speed
Florian Fainelli [mailto:f.faine...@gmail.com]
Sent: Wednesday, February 19, 2014 1:19 AM
To: Hayes Wang
Cc: netdev; nic_s...@realtek.com;
linux-kernel@vger.kernel.org; linux-usb
Subject: Re: [PATCH net-next 07/14] r8152: combine PHY reset
with set_speed
[...]
+static void rtl_phy_reset(struct r8152 *tp)
+{
+ u16 data;
+ int i;
+
+ clear_bit(PHY_RESET, tp-flags);
+
+ data = r8152_mdio_read(tp, MII_BMCR);
+
+ /* don't reset again before the previous one complete */
+ if (data BMCR_RESET)
+ return;
+
+ data |= BMCR_RESET;
+ r8152_mdio_write(tp, MII_BMCR, data);
+
+ for (i = 0; i 50; i++) {
+ msleep(20);
+ if ((r8152_mdio_read(tp, MII_BMCR)
BMCR_RESET) == 0)
+ break;
+ }
+}
If you implemented libphy in the driver you would not have to
duplicate that and you could use phy_init_hw() or
genphy_soft_reset() to perform the BMCR-based software reset.
Thanks for you suggestion. I would study about those.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 12/14] r8152: replace netif_rx withnetif_receive_skb
Francois Romieu [mailto:rom...@fr.zoreil.com] Sent: Wednesday, February 19, 2014 7:29 AM To: Hayes Wang Cc: net...@vger.kernel.org; nic_s...@realtek.com; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH net-next 12/14] r8152: replace netif_rx withnetif_receive_skb Hayes Wang hayesw...@realtek.com : Replace netif_rx with netif_receive_skb to avoid disabling irq frequently for increasing the efficiency. read_bulk_callback is issued in irq context. It could thus use plain spin_lock / spin_unlock instead of the irq disabling version. The rx_bottom() is called in tasklet, so I just think I could use netif_receive_skb directly. The netif_rx seems to queue the packet, and local_irq_disable() would be called before dequeuing the skb. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v2 6/6] r8152: support RTL8153
Bjørn Mork [mailto:bj...@mork.no] > Sent: Monday, January 06, 2014 5:22 PM > To: Hayeswang > Cc: oli...@neukum.org; net...@vger.kernel.org; nic_swsd; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH net-next v2 6/6] r8152: support RTL8153 [...] > Exactly the same device, but now cfg #1 is active and a > different set of > drivers have bound to the interfaces. This is possible > because none of > the involved drivers disable the support for this device at > build-time. > Instead they use the available interface descriptors for matching and > probing supported functions. > > End users will of course normally not go around writing stuff to sysfs > attributes like this. Creating an udev rule to select a specific > counfiguration when the device is plugged is more useful for normal > usage. Thanks for your answer. I would study udev rule first. Does the udev alwayes exist for all Linux system, such as Android, embedded system, and so on? Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v2 6/6] r8152: support RTL8153
Bjørn Mork [mailto:bj...@mork.no] Sent: Monday, January 06, 2014 5:22 PM To: Hayeswang Cc: oli...@neukum.org; net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH net-next v2 6/6] r8152: support RTL8153 [...] Exactly the same device, but now cfg #1 is active and a different set of drivers have bound to the interfaces. This is possible because none of the involved drivers disable the support for this device at build-time. Instead they use the available interface descriptors for matching and probing supported functions. End users will of course normally not go around writing stuff to sysfs attributes like this. Creating an udev rule to select a specific counfiguration when the device is plugged is more useful for normal usage. Thanks for your answer. I would study udev rule first. Does the udev alwayes exist for all Linux system, such as Android, embedded system, and so on? Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v2 6/6] r8152: support RTL8153
Bjørn Mork [mailto:bj...@mork.no] [...] > Sorry, but then this makes even less sense. The active USB > configuration is user selectable and you should make any of > them work if > possible. Why can't the drivers figure out this at runtime? Excuse me. I have no idea about how to switch the configuration at the runtime ,and how to change the default configuration when a USB device is plugged. When a user select one of the configurations, he/she would wish to fix the configuration number after rebooting or after the dangle is unplugged and plugged again. For these reasons, this is the simple way which I could think. Maybe I choose the wrong method because I don't know how to satisfy the requests. May you provide me the relative information? Then, I could replace the current method. Thanks. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next] r8152: fix the wrong return value
David Miller [mailto:da...@davemloft.net] > Sent: Saturday, January 04, 2014 9:38 AM > To: Hayeswang > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-...@vger.kernel.org > Subject: Re: [PATCH net-next] r8152: fix the wrong return value > > From: Hayes Wang > Date: Fri, 3 Jan 2014 11:21:56 +0800 > > > The return value should be the boolean value, not the error code. > > > > Signed-off-by: Hayes Wang > > Spotted-by: Dan Carpenter > > Applied, but I agree with others that it's more canonical to have the > function return either an error code, or zero for success, rather than > a boolean. Thanks. I would find if there is suitable error code. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next] r8152: fix the wrong return value
David Miller [mailto:da...@davemloft.net] Sent: Saturday, January 04, 2014 9:38 AM To: Hayeswang Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH net-next] r8152: fix the wrong return value From: Hayes Wang hayesw...@realtek.com Date: Fri, 3 Jan 2014 11:21:56 +0800 The return value should be the boolean value, not the error code. Signed-off-by: Hayes Wang hayesw...@realtek.com Spotted-by: Dan Carpenter dan.carpen...@oracle.com Applied, but I agree with others that it's more canonical to have the function return either an error code, or zero for success, rather than a boolean. Thanks. I would find if there is suitable error code. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v2 6/6] r8152: support RTL8153
Bjørn Mork [mailto:bj...@mork.no] [...] Sorry, but then this makes even less sense. The active USB configuration is user selectable and you should make any of them work if possible. Why can't the drivers figure out this at runtime? Excuse me. I have no idea about how to switch the configuration at the runtime ,and how to change the default configuration when a USB device is plugged. When a user select one of the configurations, he/she would wish to fix the configuration number after rebooting or after the dangle is unplugged and plugged again. For these reasons, this is the simple way which I could think. Maybe I choose the wrong method because I don't know how to satisfy the requests. May you provide me the relative information? Then, I could replace the current method. Thanks. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v2 6/6] r8152: support RTL8153
Bjørn Mork [mailto:bj...@mork.no]
> Sent: Thursday, January 02, 2014 10:25 PM
> To: Hayeswang
> Cc: oli...@neukum.org; net...@vger.kernel.org; nic_swsd;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH net-next v2 6/6] r8152: support RTL8153
>
[...]
> > +#if defined(CONFIG_USB_RTL8152) ||
> defined(CONFIG_USB_RTL8152_MODULE)
> > +/* Samsung USB Ethernet Adapters */
> > +{
> > + USB_DEVICE_AND_INTERFACE_INFO(SAMSUNG_VENDOR_ID,
> 0xa101, USB_CLASS_COMM,
> > + USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
> > + .driver_info = 0,
> > +},
> > +#endif
>
>
> We don't play the if-other-driver-is-enabled for any other of the
> blacklisted devices, including other devices supported by the RTL8152
> driver. Why do we need it here?
The USB nic have two configurations. One is the config 2 which is the ECM
mode and uses the driver of cdc_ether. The other is the config 1 which use
the driver of r8152. When the dangle is plugged, I find the configuration
is 2 and the ECM driver would be loaded. By this way, you could select
which mode you want to run. Some people would select ECM mode for
performance, and the other would select r8152 for power saving.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v2 6/6] r8152: support RTL8153
Bjørn Mork [mailto:bj...@mork.no]
Sent: Thursday, January 02, 2014 10:25 PM
To: Hayeswang
Cc: oli...@neukum.org; net...@vger.kernel.org; nic_swsd;
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH net-next v2 6/6] r8152: support RTL8153
[...]
+#if defined(CONFIG_USB_RTL8152) ||
defined(CONFIG_USB_RTL8152_MODULE)
+/* Samsung USB Ethernet Adapters */
+{
+ USB_DEVICE_AND_INTERFACE_INFO(SAMSUNG_VENDOR_ID,
0xa101, USB_CLASS_COMM,
+ USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
+ .driver_info = 0,
+},
+#endif
We don't play the if-other-driver-is-enabled for any other of the
blacklisted devices, including other devices supported by the RTL8152
driver. Why do we need it here?
The USB nic have two configurations. One is the config 2 which is the ECM
mode and uses the driver of cdc_ether. The other is the config 1 which use
the driver of r8152. When the dangle is plugged, I find the configuration
is 2 and the ECM driver would be loaded. By this way, you could select
which mode you want to run. Some people would select ECM mode for
performance, and the other would select r8152 for power saving.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v5 0/4] r8152 bug fixes
Any response? > -Original Message- > From: Hayeswang [mailto:hayesw...@realtek.com] > Sent: Wednesday, November 20, 2013 5:31 PM > To: net...@vger.kernel.org > Cc: nic_swsd; linux-kernel@vger.kernel.org; > linux-...@vger.kernel.org; Hayeswang > Subject: [PATCH net v5 0/4] r8152 bug fixes > > For the patch #3, I add netif_tx_lock() before checking the > netif_queue_stopped(). Besides, I add checking the skb queue > length before waking the tx queue. > > Hayes Wang (4): > r8152: fix tx/rx memory overflow > r8152: modify the tx flow > r8152: support stopping/waking tx queue > r8152: fix incorrect type in assignment > > drivers/net/usb/r8152.c | 114 > +--- > 1 file changed, 50 insertions(+), 64 deletions(-) > > -- > 1.8.3.1 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v5 0/4] r8152 bug fixes
Any response? -Original Message- From: Hayeswang [mailto:hayesw...@realtek.com] Sent: Wednesday, November 20, 2013 5:31 PM To: net...@vger.kernel.org Cc: nic_swsd; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; Hayeswang Subject: [PATCH net v5 0/4] r8152 bug fixes For the patch #3, I add netif_tx_lock() before checking the netif_queue_stopped(). Besides, I add checking the skb queue length before waking the tx queue. Hayes Wang (4): r8152: fix tx/rx memory overflow r8152: modify the tx flow r8152: support stopping/waking tx queue r8152: fix incorrect type in assignment drivers/net/usb/r8152.c | 114 +--- 1 file changed, 50 insertions(+), 64 deletions(-) -- 1.8.3.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v4 3/4] r8152: support stopping/waking tx queue
David Miller [mailto:da...@davemloft.net] [...] > > If the situation occurs, it means there is no tx buffer at > that time. If the > > netif_wake_queue() is called, only one more packet would be > queued and the tx > > queue would be stopped again after calling > rtl8152_start_xmit(). That is, it > > is not necessary to wake the queue. Besides, after the tx > is completed, another > > tasklet would be scheduled if there is any packet which is > queued in the list. > > That is, the r8152_tx_agg_fill() would be called and the > netif_queue_stopped() > > would be check againg, so the tx queue would not be stopped forever. > > Then the queue can be woken when in fact r8152_start_xmit() > is not able to > actually queue packets. It is just as equally problematic. > > You have to synchronize this state, somehow. > > tg3 driver does this by taking netif tx queue lock during the wake > test sequence in TX reclaim. This works because ->ndo_start_xmit() is > run with this lock held. Thanks for your answer. I would modify it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v4 3/4] r8152: support stopping/waking tx queue
David Miller [mailto:da...@davemloft.net] [...] > This is racy. > > You have nothing which synchronizes r8152_tx_agg_fill() and > rtl8152_start_xmit(), > therefore: > > > + if (netif_queue_stopped(tp->netdev)) > > + netif_wake_queue(tp->netdev); > > + > > A netif_stop_queue() can occur right after the > netif_queue_stopped() check, > meaning you can end up with the queue being stopped forever > and the TX queue > stuck. If the situation occurs, it means there is no tx buffer at that time. If the netif_wake_queue() is called, only one more packet would be queued and the tx queue would be stopped again after calling rtl8152_start_xmit(). That is, it is not necessary to wake the queue. Besides, after the tx is completed, another tasklet would be scheduled if there is any packet which is queued in the list. That is, the r8152_tx_agg_fill() would be called and the netif_queue_stopped() would be check againg, so the tx queue would not be stopped forever. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v4 3/4] r8152: support stopping/waking tx queue
David Miller [mailto:da...@davemloft.net] [...] This is racy. You have nothing which synchronizes r8152_tx_agg_fill() and rtl8152_start_xmit(), therefore: + if (netif_queue_stopped(tp-netdev)) + netif_wake_queue(tp-netdev); + A netif_stop_queue() can occur right after the netif_queue_stopped() check, meaning you can end up with the queue being stopped forever and the TX queue stuck. If the situation occurs, it means there is no tx buffer at that time. If the netif_wake_queue() is called, only one more packet would be queued and the tx queue would be stopped again after calling rtl8152_start_xmit(). That is, it is not necessary to wake the queue. Besides, after the tx is completed, another tasklet would be scheduled if there is any packet which is queued in the list. That is, the r8152_tx_agg_fill() would be called and the netif_queue_stopped() would be check againg, so the tx queue would not be stopped forever. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v4 3/4] r8152: support stopping/waking tx queue
David Miller [mailto:da...@davemloft.net] [...] If the situation occurs, it means there is no tx buffer at that time. If the netif_wake_queue() is called, only one more packet would be queued and the tx queue would be stopped again after calling rtl8152_start_xmit(). That is, it is not necessary to wake the queue. Besides, after the tx is completed, another tasklet would be scheduled if there is any packet which is queued in the list. That is, the r8152_tx_agg_fill() would be called and the netif_queue_stopped() would be check againg, so the tx queue would not be stopped forever. Then the queue can be woken when in fact r8152_start_xmit() is not able to actually queue packets. It is just as equally problematic. You have to synchronize this state, somehow. tg3 driver does this by taking netif tx queue lock during the wake test sequence in TX reclaim. This works because -ndo_start_xmit() is run with this lock held. Thanks for your answer. I would modify it. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v3 1/4] r8152: fix tx/rx memory overflow
David Miller [mailto:da...@davemloft.net] > Sent: Saturday, November 16, 2013 6:40 AM > To: Hayeswang > Cc: net...@vger.kernel.org; nic_swsd; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH net v3 1/4] r8152: fix tx/rx memory overflow > > From: Hayes Wang > Date: Fri, 15 Nov 2013 15:57:56 +0800 > > > + unsigned pkt_len; > > Please fully specify the type as "unsigned int". Please > check for this > problem in the rest of your patches too. I would fix it. I check the other patches, and I don't find the same problem. Thanks. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v3 1/4] r8152: fix tx/rx memory overflow
David Miller [mailto:da...@davemloft.net] Sent: Saturday, November 16, 2013 6:40 AM To: Hayeswang Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH net v3 1/4] r8152: fix tx/rx memory overflow From: Hayes Wang hayesw...@realtek.com Date: Fri, 15 Nov 2013 15:57:56 +0800 + unsigned pkt_len; Please fully specify the type as unsigned int. Please check for this problem in the rest of your patches too. I would fix it. I check the other patches, and I don't find the same problem. Thanks. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v2 2/3] r8152: modify the tx flow
David Miller [mailto:da...@davemloft.net] > Sent: Tuesday, November 05, 2013 4:53 AM > To: Hayeswang > Cc: net...@vger.kernel.org; nic_swsd; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH net v2 2/3] r8152: modify the tx flow [...] > The more TX work you push into the workqueue handler, the longer the > latency for releasing the SKB and releasing all the queues that are > waiting for release of that packet. > > Do you know that sockets, queueing discplines, etc. all rely upon > there being a timely release of SKBs once they are successfully > transmitted? It must happen at the earliest moment possible that > can be reasonable obtained. Thanks for your answer. I would resend the patches after finishing the setting of the queue length. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v2 2/3] r8152: modify the tx flow
David Miller [mailto:da...@davemloft.net] Sent: Tuesday, November 05, 2013 4:53 AM To: Hayeswang Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH net v2 2/3] r8152: modify the tx flow [...] The more TX work you push into the workqueue handler, the longer the latency for releasing the SKB and releasing all the queues that are waiting for release of that packet. Do you know that sockets, queueing discplines, etc. all rely upon there being a timely release of SKBs once they are successfully transmitted? It must happen at the earliest moment possible that can be reasonable obtained. Thanks for your answer. I would resend the patches after finishing the setting of the queue length. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v2 2/3] r8152: modify the tx flow
From: David Miller [mailto:da...@davemloft.net] Sent: Thursday, October 31, 2013 5:05 AM > > From: Hayes Wang > Date: Wed, 30 Oct 2013 15:13:39 +0800 [...] > Basically, your driver will now queue up to 1,000 packets onto > this tx_queue list, because that is what tx_queue_len will be > for alloc_etherdev() allocated network devices. > > In my previous reply to you about this patch, I asked you to > quantify and study the effects of using a limit of 60. I said > that 60 might be too large. > > You've responded by removing the limit completely, which is exactly > the opposite of what I've asked you to do. Why did you do this? Excuse me. My question is that the original code doesn't stop the tx queue either, so I don't understand why it is necessary for this patch. I don't say I wouldn't find the suitable value for the tx queue length. I feel I need some time to think how to find the reasonable value. And I don't hope it influences the submission of the other patches, so I remove it first. Or, may I submit the other two patches first? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net v2 2/3] r8152: modify the tx flow
From: David Miller [mailto:da...@davemloft.net] Sent: Thursday, October 31, 2013 5:05 AM From: Hayes Wang hayesw...@realtek.com Date: Wed, 30 Oct 2013 15:13:39 +0800 [...] Basically, your driver will now queue up to 1,000 packets onto this tx_queue list, because that is what tx_queue_len will be for alloc_etherdev() allocated network devices. In my previous reply to you about this patch, I asked you to quantify and study the effects of using a limit of 60. I said that 60 might be too large. You've responded by removing the limit completely, which is exactly the opposite of what I've asked you to do. Why did you do this? Excuse me. My question is that the original code doesn't stop the tx queue either, so I don't understand why it is necessary for this patch. I don't say I wouldn't find the suitable value for the tx queue length. I feel I need some time to think how to find the reasonable value. And I don't hope it influences the submission of the other patches, so I remove it first. Or, may I submit the other two patches first? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net 2/3] r8152: modify the tx flow
David Miller [mailto:da...@davemloft.net] > Sent: Wednesday, October 30, 2013 5:50 AM > To: Hayeswang > Cc: net...@vger.kernel.org; nic_swsd; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH net 2/3] r8152: modify the tx flow > > From: Hayes Wang > Date: Tue, 29 Oct 2013 15:56:16 +0800 > > > Support stopping and waking tx queue. The maximum tx queue length > > is 60. > > What is so special about the number 60? It seems arbitrary, and if > it isn't arbitrary you haven't described why this value was choosen. The value is arbitrary. I think it is better to stop tx when queuing many packets, otherwise all the available memory may be used for tx skb. The queue length could be any value or unlimited if the memory is enough. Should I remove it? > I've asked you politely last time around to significantly improve > the quality of your commit messages, and you haven't done this at > all. I thought you indicated the last patch only and the others are clear enough. I would improve them. > I'm not applying any of these patches until your commit messages > properly describe your changes completely. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net 2/3] r8152: modify the tx flow
David Miller [mailto:da...@davemloft.net] Sent: Wednesday, October 30, 2013 5:50 AM To: Hayeswang Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH net 2/3] r8152: modify the tx flow From: Hayes Wang hayesw...@realtek.com Date: Tue, 29 Oct 2013 15:56:16 +0800 Support stopping and waking tx queue. The maximum tx queue length is 60. What is so special about the number 60? It seems arbitrary, and if it isn't arbitrary you haven't described why this value was choosen. The value is arbitrary. I think it is better to stop tx when queuing many packets, otherwise all the available memory may be used for tx skb. The queue length could be any value or unlimited if the memory is enough. Should I remove it? I've asked you politely last time around to significantly improve the quality of your commit messages, and you haven't done this at all. I thought you indicated the last patch only and the others are clear enough. I would improve them. I'm not applying any of these patches until your commit messages properly describe your changes completely. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v2 1/3] net/usb/r8152: support aggregation
Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Thursday, August 15, 2013 8:26 PM
> To: Hayeswang
> Cc: net...@vger.kernel.org; nic_swsd;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; David Miller
> Subject: Re: [PATCH net-next v2 1/3] net/usb/r8152: support
> aggregation
>
[...]
> > +static
> > +int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg,
> gfp_t mem_flags);
> > +
>
> It's a new, less than 10 lines function without driver
> internal dependencies.
>
> The forward declaration is not needed.
The r8152_submit_rx() need the declaration of read_bulk_callback(), and the
read_bulk_callback() need the declaration of r8152_submit_rx(), too. It likes
a dead lock, so I have no idea how to do it without another declaration.
[...]
> > - if (!netif_device_present(netdev))
> > + if (!netif_carrier_ok(netdev))
> > return;
>
> How is it related to the subject of the patch ?
When link down, the driver would cancel all bulks. This avoid the re-submitting
bulk.
> [...]
> > +static void rx_bottom(struct r8152 *tp)
> > +{
> > + struct net_device_stats *stats;
> > + struct net_device *netdev;
> > + struct rx_agg *agg;
> > + struct rx_desc *rx_desc;
> > + unsigned long lockflags;
>
> Idiom: 'flags'.
>
> > + struct list_head *cursor, *next;
> > + struct sk_buff *skb;
> > + struct urb *urb;
> > + unsigned pkt_len;
> > + int len_used;
> > + u8 *rx_data;
> > + int ret;
>
> The scope of these variables is needlessly wide.
>
> > +
> > + netdev = tp->netdev;
> > +
> > + stats = rtl8152_get_stats(netdev);
> > +
> > + spin_lock_irqsave(>rx_lock, lockflags);
> > + list_for_each_safe(cursor, next, >rx_done) {
> > + list_del_init(cursor);
> > + spin_unlock_irqrestore(>rx_lock, lockflags);
> > +
> > + agg = list_entry(cursor, struct rx_agg, list);
> > + urb = agg->urb;
> > + if (urb->actual_length < ETH_ZLEN) {
>
> goto submit;
>
> > + ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
> > + spin_lock_irqsave(>rx_lock, lockflags);
> > + if (ret && ret != -ENODEV) {
> > + list_add_tail(>list, next);
> > + tasklet_schedule(>tl);
> > + }
> > + continue;
> > + }
>
> (remove the line above)
>
> [...]
> > + rx_data = rx_agg_align(rx_data + pkt_len + 4);
> > + rx_desc = (struct rx_desc *)rx_data;
> > + pkt_len = le32_to_cpu(rx_desc->opts1) &
> RX_LEN_MASK;
> > + len_used = (int)(rx_data - (u8 *)agg->head);
> > + len_used += sizeof(struct rx_desc) + pkt_len;
> > + }
> > +
>
> submit:
>
> > + ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
> > + spin_lock_irqsave(>rx_lock, lockflags);
> > + if (ret && ret != -ENODEV) {
> > + list_add_tail(>list, next);
> > + tasklet_schedule(>tl);
> > + }
> > + }
> > + spin_unlock_irqrestore(>rx_lock, lockflags);
> > +}
>
> It should be possible to retrieve more items in the spinlocked section
> so as to have a chance to batch more work. I have not thought
> too deeply
> about it.
I only lock when I want to remove or inser the agg list, and unlock as soon as
possible. I don't think I keep locking for a long time.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next v2 1/3] net/usb/r8152: support aggregation
Francois Romieu [mailto:rom...@fr.zoreil.com]
Sent: Thursday, August 15, 2013 8:26 PM
To: Hayeswang
Cc: net...@vger.kernel.org; nic_swsd;
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; David Miller
Subject: Re: [PATCH net-next v2 1/3] net/usb/r8152: support
aggregation
[...]
+static
+int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg,
gfp_t mem_flags);
+
It's a new, less than 10 lines function without driver
internal dependencies.
The forward declaration is not needed.
The r8152_submit_rx() need the declaration of read_bulk_callback(), and the
read_bulk_callback() need the declaration of r8152_submit_rx(), too. It likes
a dead lock, so I have no idea how to do it without another declaration.
[...]
- if (!netif_device_present(netdev))
+ if (!netif_carrier_ok(netdev))
return;
How is it related to the subject of the patch ?
When link down, the driver would cancel all bulks. This avoid the re-submitting
bulk.
[...]
+static void rx_bottom(struct r8152 *tp)
+{
+ struct net_device_stats *stats;
+ struct net_device *netdev;
+ struct rx_agg *agg;
+ struct rx_desc *rx_desc;
+ unsigned long lockflags;
Idiom: 'flags'.
+ struct list_head *cursor, *next;
+ struct sk_buff *skb;
+ struct urb *urb;
+ unsigned pkt_len;
+ int len_used;
+ u8 *rx_data;
+ int ret;
The scope of these variables is needlessly wide.
+
+ netdev = tp-netdev;
+
+ stats = rtl8152_get_stats(netdev);
+
+ spin_lock_irqsave(tp-rx_lock, lockflags);
+ list_for_each_safe(cursor, next, tp-rx_done) {
+ list_del_init(cursor);
+ spin_unlock_irqrestore(tp-rx_lock, lockflags);
+
+ agg = list_entry(cursor, struct rx_agg, list);
+ urb = agg-urb;
+ if (urb-actual_length ETH_ZLEN) {
goto submit;
+ ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
+ spin_lock_irqsave(tp-rx_lock, lockflags);
+ if (ret ret != -ENODEV) {
+ list_add_tail(agg-list, next);
+ tasklet_schedule(tp-tl);
+ }
+ continue;
+ }
(remove the line above)
[...]
+ rx_data = rx_agg_align(rx_data + pkt_len + 4);
+ rx_desc = (struct rx_desc *)rx_data;
+ pkt_len = le32_to_cpu(rx_desc-opts1)
RX_LEN_MASK;
+ len_used = (int)(rx_data - (u8 *)agg-head);
+ len_used += sizeof(struct rx_desc) + pkt_len;
+ }
+
submit:
+ ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
+ spin_lock_irqsave(tp-rx_lock, lockflags);
+ if (ret ret != -ENODEV) {
+ list_add_tail(agg-list, next);
+ tasklet_schedule(tp-tl);
+ }
+ }
+ spin_unlock_irqrestore(tp-rx_lock, lockflags);
+}
It should be possible to retrieve more items in the spinlocked section
so as to have a chance to batch more work. I have not thought
too deeply
about it.
I only lock when I want to remove or inser the agg list, and unlock as soon as
possible. I don't think I keep locking for a long time.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/3] net/usb/r8152: support aggregation
David Miller [mailto:da...@davemloft.net] > Sent: Wednesday, August 14, 2013 7:41 AM > To: oneu...@suse.de > Cc: Hayeswang; net...@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation > [...] > > I don't understand what problem the function is supposed to > fix. As long > > as I don't understand it I cannot say for sure whether it > is correct. > > There seems no obvious reason for a memory barrier, but > there may be a > > hidden reason I don't see. > > Hayes, when Oliver asks you "Against what is the memory > barrier?" he is asking > you which memory operations you are trying to order. > > You do not explain this in your commit message, nor do you > explain it with a > suitable comment. This is not acceptable. > > It is absolutely critical, that any time you add a memory > barrier, you add a > comment above the new memory barrier explaining exactly what > the barrier is > trying to achieve. > > In fact, this is required by our coding standards. I just want to make sure the rx_desc and rx_data are set correctly before they are used. However, I study some examples and information from internet, and I think that the memory barries is not necessary here. Therefore, I would remove them later. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/3] net/usb/r8152: support aggregation
Oliver Neukum [mailto:oneu...@suse.de]
> Sent: Tuesday, August 13, 2013 4:49 PM
> To: Hayeswang
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-...@vger.kernel.org
> Subject: Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
>
[...]
> > + len_used = 0;
> > + rx_desc = agg->head;
> > + rx_data = agg->head;
> > + smp_wmb();
> > + pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
> > + len_used += sizeof(struct rx_desc) + pkt_len;
> > +
> > + while (urb->actual_length >= len_used) {
> > + if (pkt_len < ETH_ZLEN)
> > + break;
> > +
> > + pkt_len -= 4; /* CRC */
> > + rx_data += sizeof(struct rx_desc);
> > +
> > + skb = netdev_alloc_skb_ip_align(netdev,
> > pkt_len);
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + break;
> > + }
> > + memcpy(skb->data, rx_data, pkt_len);
> > + skb_put(skb, pkt_len);
> > + skb->protocol = eth_type_trans(skb, netdev);
> > + netif_rx(skb);
> > + stats->rx_packets++;
> > + stats->rx_bytes += pkt_len;
> > +
> > + rx_data = rx_agg_align(rx_data +
> pkt_len + 4);
> > + rx_desc = (struct rx_desc *)rx_data;
> > + smp_wmb();
>
> Against what is the memory barrier?
Excuse me. I don't understand your question. Do you mean the function should not
be used here?
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/3] net/usb/r8152: support aggregation
Oliver Neukum [mailto:oneu...@suse.de]
Sent: Tuesday, August 13, 2013 4:49 PM
To: Hayeswang
Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-...@vger.kernel.org
Subject: Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
[...]
+ len_used = 0;
+ rx_desc = agg-head;
+ rx_data = agg-head;
+ smp_wmb();
+ pkt_len = le32_to_cpu(rx_desc-opts1) RX_LEN_MASK;
+ len_used += sizeof(struct rx_desc) + pkt_len;
+
+ while (urb-actual_length = len_used) {
+ if (pkt_len ETH_ZLEN)
+ break;
+
+ pkt_len -= 4; /* CRC */
+ rx_data += sizeof(struct rx_desc);
+
+ skb = netdev_alloc_skb_ip_align(netdev,
pkt_len);
+ if (!skb) {
+ stats-rx_dropped++;
+ break;
+ }
+ memcpy(skb-data, rx_data, pkt_len);
+ skb_put(skb, pkt_len);
+ skb-protocol = eth_type_trans(skb, netdev);
+ netif_rx(skb);
+ stats-rx_packets++;
+ stats-rx_bytes += pkt_len;
+
+ rx_data = rx_agg_align(rx_data +
pkt_len + 4);
+ rx_desc = (struct rx_desc *)rx_data;
+ smp_wmb();
Against what is the memory barrier?
Excuse me. I don't understand your question. Do you mean the function should not
be used here?
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/3] net/usb/r8152: support aggregation
David Miller [mailto:da...@davemloft.net] Sent: Wednesday, August 14, 2013 7:41 AM To: oneu...@suse.de Cc: Hayeswang; net...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation [...] I don't understand what problem the function is supposed to fix. As long as I don't understand it I cannot say for sure whether it is correct. There seems no obvious reason for a memory barrier, but there may be a hidden reason I don't see. Hayes, when Oliver asks you Against what is the memory barrier? he is asking you which memory operations you are trying to order. You do not explain this in your commit message, nor do you explain it with a suitable comment. This is not acceptable. It is absolutely critical, that any time you add a memory barrier, you add a comment above the new memory barrier explaining exactly what the barrier is trying to achieve. In fact, this is required by our coding standards. I just want to make sure the rx_desc and rx_data are set correctly before they are used. However, I study some examples and information from internet, and I think that the memory barries is not necessary here. Therefore, I would remove them later. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] net/usb/r8152: make sure the USB buffer is DMA-able
Allocate the required memory before calling usb_control_msg. And
the additional memory copy is necessary.
Signed-off-by: Hayes Wang
---
drivers/net/usb/r8152.c | 60 -
1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ee13f9e..ef033ab 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -344,17 +344,41 @@ static const int multicast_filter_limit = 32;
static
int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
{
- return usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
+ int ret;
+ void *tmp;
+
+ tmp = kmalloc(size, GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ ret = usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
- value, index, data, size, 500);
+ value, index, tmp, size, 500);
+
+ memcpy(data, tmp, size);
+ kfree(tmp);
+
+ return ret;
}
static
int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
{
- return usb_control_msg(tp->udev, usb_sndctrlpipe(tp->udev, 0),
+ int ret;
+ void *tmp;
+
+ tmp = kmalloc(size, GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ memcpy(tmp, data, size);
+
+ ret = usb_control_msg(tp->udev, usb_sndctrlpipe(tp->udev, 0),
RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
- value, index, data, size, 500);
+ value, index, tmp, size, 500);
+
+ kfree(tmp);
+ return ret;
}
static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
@@ -685,21 +709,14 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16
data)
static inline void set_ethernet_addr(struct r8152 *tp)
{
struct net_device *dev = tp->netdev;
- u8 *node_id;
+ u8 node_id[8] = {0};
- node_id = kmalloc(sizeof(u8) * 8, GFP_KERNEL);
- if (!node_id) {
- netif_err(tp, probe, dev, "out of memory");
- return;
- }
-
- if (pla_ocp_read(tp, PLA_IDR, sizeof(u8) * 8, node_id) < 0)
+ if (pla_ocp_read(tp, PLA_IDR, sizeof(node_id), node_id) < 0)
netif_notice(tp, probe, dev, "inet addr fail\n");
else {
memcpy(dev->dev_addr, node_id, dev->addr_len);
memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len);
}
- kfree(node_id);
}
static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
@@ -882,15 +899,10 @@ static void rtl8152_set_rx_mode(struct net_device *netdev)
static void _rtl8152_set_rx_mode(struct net_device *netdev)
{
struct r8152 *tp = netdev_priv(netdev);
- u32 tmp, *mc_filter;/* Multicast hash filter */
+ u32 mc_filter[2]; /* Multicast hash filter */
+ __le32 tmp[2];
u32 ocp_data;
- mc_filter = kmalloc(sizeof(u32) * 2, GFP_KERNEL);
- if (!mc_filter) {
- netif_err(tp, link, netdev, "out of memory");
- return;
- }
-
clear_bit(RTL8152_SET_RX_MODE, >flags);
netif_stop_queue(netdev);
ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
@@ -918,14 +930,12 @@ static void _rtl8152_set_rx_mode(struct net_device
*netdev)
}
}
- tmp = mc_filter[0];
- mc_filter[0] = __cpu_to_le32(swab32(mc_filter[1]));
- mc_filter[1] = __cpu_to_le32(swab32(tmp));
+ tmp[0] = __cpu_to_le32(swab32(mc_filter[1]));
+ tmp[1] = __cpu_to_le32(swab32(mc_filter[0]));
- pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(u32) * 2, mc_filter);
+ pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(tmp), tmp);
ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
netif_wake_queue(netdev);
- kfree(mc_filter);
}
static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] net/usb/r8152: adjust relative ocp function
- fix the conversion between cpu and __le32
- replace some pla_ocp and usb_ocp functions with generic_ocp function
Signed-off-by: Hayes Wang
---
drivers/net/usb/r8152.c | 66 +
1 file changed, 23 insertions(+), 43 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ef033ab..11c51f2 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -514,37 +514,31 @@ int usb_ocp_write(struct r8152 *tp, u16 index, u16
byteen, u16 size, void *data)
static u32 ocp_read_dword(struct r8152 *tp, u16 type, u16 index)
{
- u32 data;
+ __le32 data;
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), );
- else
- usb_ocp_read(tp, index, sizeof(data), );
+ generic_ocp_read(tp, index, sizeof(data), , type);
return __le32_to_cpu(data);
}
static void ocp_write_dword(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(data), );
- else
- usb_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(data), );
+ __le32 tmp = __cpu_to_le32(data);
+
+ generic_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(tmp), , type);
}
static u16 ocp_read_word(struct r8152 *tp, u16 type, u16 index)
{
u32 data;
+ __le32 tmp;
u8 shift = index & 2;
index &= ~3;
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), );
- else
- usb_ocp_read(tp, index, sizeof(data), );
+ generic_ocp_read(tp, index, sizeof(tmp), , type);
- data = __le32_to_cpu(data);
+ data = __le32_to_cpu(tmp);
data >>= (shift * 8);
data &= 0x;
@@ -553,7 +547,8 @@ static u16 ocp_read_word(struct r8152 *tp, u16 type, u16
index)
static void ocp_write_word(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- u32 tmp, mask = 0x;
+ u32 mask = 0x;
+ __le32 tmp;
u16 byen = BYTE_EN_WORD;
u8 shift = index & 2;
@@ -566,34 +561,25 @@ static void ocp_write_word(struct r8152 *tp, u16 type,
u16 index, u32 data)
index &= ~3;
}
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(tmp), );
- else
- usb_ocp_read(tp, index, sizeof(tmp), );
+ generic_ocp_read(tp, index, sizeof(tmp), , type);
- tmp = __le32_to_cpu(tmp) & ~mask;
- tmp |= data;
- tmp = __cpu_to_le32(tmp);
+ data |= __le32_to_cpu(tmp) & ~mask;
+ tmp = __cpu_to_le32(data);
- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, byen, sizeof(tmp), );
- else
- usb_ocp_write(tp, index, byen, sizeof(tmp), );
+ generic_ocp_write(tp, index, byen, sizeof(tmp), , type);
}
static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16 index)
{
u32 data;
+ __le32 tmp;
u8 shift = index & 3;
index &= ~3;
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), );
- else
- usb_ocp_read(tp, index, sizeof(data), );
+ generic_ocp_read(tp, index, sizeof(tmp), , type);
- data = __le32_to_cpu(data);
+ data = __le32_to_cpu(tmp);
data >>= (shift * 8);
data &= 0xff;
@@ -602,7 +588,8 @@ static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16
index)
static void ocp_write_byte(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- u32 tmp, mask = 0xff;
+ u32 mask = 0xff;
+ __le32 tmp;
u16 byen = BYTE_EN_BYTE;
u8 shift = index & 3;
@@ -615,19 +602,12 @@ static void ocp_write_byte(struct r8152 *tp, u16 type,
u16 index, u32 data)
index &= ~3;
}
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(tmp), );
- else
- usb_ocp_read(tp, index, sizeof(tmp), );
+ generic_ocp_read(tp, index, sizeof(tmp), , type);
- tmp = __le32_to_cpu(tmp) & ~mask;
- tmp |= data;
- tmp = __cpu_to_le32(tmp);
+ data |= __le32_to_cpu(tmp) & ~mask;
+ tmp = __cpu_to_le32(data);
- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, byen, sizeof(tmp), );
- else
- usb_ocp_write(tp, index, byen, sizeof(tmp), );
+ generic_ocp_write(tp, index, byen, sizeof(tmp), , type);
}
static void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able
Some USB buffers use stack which may not be DMA-able.
Use the buffers from kmalloc to replace those one.
Signed-off-by: Hayes Wang
---
drivers/net/usb/r815x.c | 44 +++-
1 file changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/net/usb/r815x.c b/drivers/net/usb/r815x.c
index 8523922..e9b99ba 100644
--- a/drivers/net/usb/r815x.c
+++ b/drivers/net/usb/r815x.c
@@ -24,34 +24,43 @@
static int pla_read_word(struct usb_device *udev, u16 index)
{
- int data, ret;
+ int ret;
u8 shift = index & 2;
- __le32 ocp_data;
+ __le32 *tmp;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
index &= ~3;
ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
RTL815x_REQ_GET_REGS, RTL815x_REQT_READ,
- index, MCU_TYPE_PLA, _data, sizeof(ocp_data),
- 500);
+ index, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);
if (ret < 0)
- return ret;
+ goto out2;
- data = __le32_to_cpu(ocp_data);
- data >>= (shift * 8);
- data &= 0x;
+ ret = __le32_to_cpu(*tmp);
+ ret >>= (shift * 8);
+ ret &= 0x;
- return data;
+out2:
+ kfree(tmp);
+ return ret;
}
static int pla_write_word(struct usb_device *udev, u16 index, u32 data)
{
- __le32 ocp_data;
+ __le32 *tmp;
u32 mask = 0x;
u16 byen = BYTE_EN_WORD;
u8 shift = index & 2;
int ret;
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
data &= mask;
if (shift) {
@@ -63,19 +72,20 @@ static int pla_write_word(struct usb_device *udev, u16
index, u32 data)
ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
RTL815x_REQ_GET_REGS, RTL815x_REQT_READ,
- index, MCU_TYPE_PLA, _data, sizeof(ocp_data),
- 500);
+ index, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);
if (ret < 0)
- return ret;
+ goto out3;
- data |= __le32_to_cpu(ocp_data) & ~mask;
- ocp_data = __cpu_to_le32(data);
+ data |= __le32_to_cpu(*tmp) & ~mask;
+ *tmp = __cpu_to_le32(data);
ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
RTL815x_REQ_SET_REGS, RTL815x_REQT_WRITE,
- index, MCU_TYPE_PLA | byen, _data,
- sizeof(ocp_data), 500);
+ index, MCU_TYPE_PLA | byen, tmp, sizeof(*tmp),
+ 500);
+out3:
+ kfree(tmp);
return ret;
}
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able
Some USB buffers use stack which may not be DMA-able.
Use the buffers from kmalloc to replace those one.
Signed-off-by: Hayes Wang hayesw...@realtek.com
---
drivers/net/usb/r815x.c | 44 +++-
1 file changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/net/usb/r815x.c b/drivers/net/usb/r815x.c
index 8523922..e9b99ba 100644
--- a/drivers/net/usb/r815x.c
+++ b/drivers/net/usb/r815x.c
@@ -24,34 +24,43 @@
static int pla_read_word(struct usb_device *udev, u16 index)
{
- int data, ret;
+ int ret;
u8 shift = index 2;
- __le32 ocp_data;
+ __le32 *tmp;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
index = ~3;
ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
RTL815x_REQ_GET_REGS, RTL815x_REQT_READ,
- index, MCU_TYPE_PLA, ocp_data, sizeof(ocp_data),
- 500);
+ index, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);
if (ret 0)
- return ret;
+ goto out2;
- data = __le32_to_cpu(ocp_data);
- data = (shift * 8);
- data = 0x;
+ ret = __le32_to_cpu(*tmp);
+ ret = (shift * 8);
+ ret = 0x;
- return data;
+out2:
+ kfree(tmp);
+ return ret;
}
static int pla_write_word(struct usb_device *udev, u16 index, u32 data)
{
- __le32 ocp_data;
+ __le32 *tmp;
u32 mask = 0x;
u16 byen = BYTE_EN_WORD;
u8 shift = index 2;
int ret;
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
data = mask;
if (shift) {
@@ -63,19 +72,20 @@ static int pla_write_word(struct usb_device *udev, u16
index, u32 data)
ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
RTL815x_REQ_GET_REGS, RTL815x_REQT_READ,
- index, MCU_TYPE_PLA, ocp_data, sizeof(ocp_data),
- 500);
+ index, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);
if (ret 0)
- return ret;
+ goto out3;
- data |= __le32_to_cpu(ocp_data) ~mask;
- ocp_data = __cpu_to_le32(data);
+ data |= __le32_to_cpu(*tmp) ~mask;
+ *tmp = __cpu_to_le32(data);
ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
RTL815x_REQ_SET_REGS, RTL815x_REQT_WRITE,
- index, MCU_TYPE_PLA | byen, ocp_data,
- sizeof(ocp_data), 500);
+ index, MCU_TYPE_PLA | byen, tmp, sizeof(*tmp),
+ 500);
+out3:
+ kfree(tmp);
return ret;
}
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] net/usb/r8152: make sure the USB buffer is DMA-able
Allocate the required memory before calling usb_control_msg. And
the additional memory copy is necessary.
Signed-off-by: Hayes Wang hayesw...@realtek.com
---
drivers/net/usb/r8152.c | 60 -
1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ee13f9e..ef033ab 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -344,17 +344,41 @@ static const int multicast_filter_limit = 32;
static
int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
{
- return usb_control_msg(tp-udev, usb_rcvctrlpipe(tp-udev, 0),
+ int ret;
+ void *tmp;
+
+ tmp = kmalloc(size, GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ ret = usb_control_msg(tp-udev, usb_rcvctrlpipe(tp-udev, 0),
RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
- value, index, data, size, 500);
+ value, index, tmp, size, 500);
+
+ memcpy(data, tmp, size);
+ kfree(tmp);
+
+ return ret;
}
static
int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
{
- return usb_control_msg(tp-udev, usb_sndctrlpipe(tp-udev, 0),
+ int ret;
+ void *tmp;
+
+ tmp = kmalloc(size, GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ memcpy(tmp, data, size);
+
+ ret = usb_control_msg(tp-udev, usb_sndctrlpipe(tp-udev, 0),
RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
- value, index, data, size, 500);
+ value, index, tmp, size, 500);
+
+ kfree(tmp);
+ return ret;
}
static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
@@ -685,21 +709,14 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16
data)
static inline void set_ethernet_addr(struct r8152 *tp)
{
struct net_device *dev = tp-netdev;
- u8 *node_id;
+ u8 node_id[8] = {0};
- node_id = kmalloc(sizeof(u8) * 8, GFP_KERNEL);
- if (!node_id) {
- netif_err(tp, probe, dev, out of memory);
- return;
- }
-
- if (pla_ocp_read(tp, PLA_IDR, sizeof(u8) * 8, node_id) 0)
+ if (pla_ocp_read(tp, PLA_IDR, sizeof(node_id), node_id) 0)
netif_notice(tp, probe, dev, inet addr fail\n);
else {
memcpy(dev-dev_addr, node_id, dev-addr_len);
memcpy(dev-perm_addr, dev-dev_addr, dev-addr_len);
}
- kfree(node_id);
}
static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
@@ -882,15 +899,10 @@ static void rtl8152_set_rx_mode(struct net_device *netdev)
static void _rtl8152_set_rx_mode(struct net_device *netdev)
{
struct r8152 *tp = netdev_priv(netdev);
- u32 tmp, *mc_filter;/* Multicast hash filter */
+ u32 mc_filter[2]; /* Multicast hash filter */
+ __le32 tmp[2];
u32 ocp_data;
- mc_filter = kmalloc(sizeof(u32) * 2, GFP_KERNEL);
- if (!mc_filter) {
- netif_err(tp, link, netdev, out of memory);
- return;
- }
-
clear_bit(RTL8152_SET_RX_MODE, tp-flags);
netif_stop_queue(netdev);
ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
@@ -918,14 +930,12 @@ static void _rtl8152_set_rx_mode(struct net_device
*netdev)
}
}
- tmp = mc_filter[0];
- mc_filter[0] = __cpu_to_le32(swab32(mc_filter[1]));
- mc_filter[1] = __cpu_to_le32(swab32(tmp));
+ tmp[0] = __cpu_to_le32(swab32(mc_filter[1]));
+ tmp[1] = __cpu_to_le32(swab32(mc_filter[0]));
- pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(u32) * 2, mc_filter);
+ pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(tmp), tmp);
ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
netif_wake_queue(netdev);
- kfree(mc_filter);
}
static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] net/usb/r8152: adjust relative ocp function
- fix the conversion between cpu and __le32
- replace some pla_ocp and usb_ocp functions with generic_ocp function
Signed-off-by: Hayes Wang hayesw...@realtek.com
---
drivers/net/usb/r8152.c | 66 +
1 file changed, 23 insertions(+), 43 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ef033ab..11c51f2 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -514,37 +514,31 @@ int usb_ocp_write(struct r8152 *tp, u16 index, u16
byteen, u16 size, void *data)
static u32 ocp_read_dword(struct r8152 *tp, u16 type, u16 index)
{
- u32 data;
+ __le32 data;
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), data);
- else
- usb_ocp_read(tp, index, sizeof(data), data);
+ generic_ocp_read(tp, index, sizeof(data), data, type);
return __le32_to_cpu(data);
}
static void ocp_write_dword(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(data), data);
- else
- usb_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(data), data);
+ __le32 tmp = __cpu_to_le32(data);
+
+ generic_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(tmp), tmp, type);
}
static u16 ocp_read_word(struct r8152 *tp, u16 type, u16 index)
{
u32 data;
+ __le32 tmp;
u8 shift = index 2;
index = ~3;
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), data);
- else
- usb_ocp_read(tp, index, sizeof(data), data);
+ generic_ocp_read(tp, index, sizeof(tmp), tmp, type);
- data = __le32_to_cpu(data);
+ data = __le32_to_cpu(tmp);
data = (shift * 8);
data = 0x;
@@ -553,7 +547,8 @@ static u16 ocp_read_word(struct r8152 *tp, u16 type, u16
index)
static void ocp_write_word(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- u32 tmp, mask = 0x;
+ u32 mask = 0x;
+ __le32 tmp;
u16 byen = BYTE_EN_WORD;
u8 shift = index 2;
@@ -566,34 +561,25 @@ static void ocp_write_word(struct r8152 *tp, u16 type,
u16 index, u32 data)
index = ~3;
}
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(tmp), tmp);
- else
- usb_ocp_read(tp, index, sizeof(tmp), tmp);
+ generic_ocp_read(tp, index, sizeof(tmp), tmp, type);
- tmp = __le32_to_cpu(tmp) ~mask;
- tmp |= data;
- tmp = __cpu_to_le32(tmp);
+ data |= __le32_to_cpu(tmp) ~mask;
+ tmp = __cpu_to_le32(data);
- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, byen, sizeof(tmp), tmp);
- else
- usb_ocp_write(tp, index, byen, sizeof(tmp), tmp);
+ generic_ocp_write(tp, index, byen, sizeof(tmp), tmp, type);
}
static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16 index)
{
u32 data;
+ __le32 tmp;
u8 shift = index 3;
index = ~3;
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), data);
- else
- usb_ocp_read(tp, index, sizeof(data), data);
+ generic_ocp_read(tp, index, sizeof(tmp), tmp, type);
- data = __le32_to_cpu(data);
+ data = __le32_to_cpu(tmp);
data = (shift * 8);
data = 0xff;
@@ -602,7 +588,8 @@ static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16
index)
static void ocp_write_byte(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- u32 tmp, mask = 0xff;
+ u32 mask = 0xff;
+ __le32 tmp;
u16 byen = BYTE_EN_BYTE;
u8 shift = index 3;
@@ -615,19 +602,12 @@ static void ocp_write_byte(struct r8152 *tp, u16 type,
u16 index, u32 data)
index = ~3;
}
- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(tmp), tmp);
- else
- usb_ocp_read(tp, index, sizeof(tmp), tmp);
+ generic_ocp_read(tp, index, sizeof(tmp), tmp, type);
- tmp = __le32_to_cpu(tmp) ~mask;
- tmp |= data;
- tmp = __cpu_to_le32(tmp);
+ data |= __le32_to_cpu(tmp) ~mask;
+ tmp = __cpu_to_le32(data);
- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, byen, sizeof(tmp), tmp);
- else
- usb_ocp_write(tp, index, byen, sizeof(tmp), tmp);
+ generic_ocp_write(tp, index, byen, sizeof(tmp), tmp, type);
}
static void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3 net-next] net/usb: new driver for RTL8152
Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Friday, May 03, 2013 10:33 AM > To: Hayeswang > Cc: oli...@neukum.org; net...@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; nic_swsd > Subject: Re: [PATCH v3 net-next] net/usb: new driver for RTL8152 > > On Fri, May 03, 2013 at 10:01:25AM +0800, Hayes Wang wrote: > > Add new driver for supporting Realtek RTL8152 Based USB 2.0 > Ethernet Adapters > > > > Signed-off-by: Hayes Wang > > Cc: Realtek linux nic maintainers > > You removed the wording I had questions about last time, does > that mean > the previous wording was not correct, or that you no longer are > publicising the fact that this driver reads on some patents that you > own? > > You also failed to answer my previous questions, why? That description is about the patent for the hw design, not for software. It indicates that the design of the hw uses those patents from the other compnay. I have no idea about how to deal with those patents. What should I do for sending a patch? Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next] net/usb: new driver for RTL8152
Oliver Neukum [mailto:oneu...@suse.de]
> Sent: Friday, April 26, 2013 7:57 PM
> To: Hayeswang
> Cc: gre...@linuxfoundation.org; net...@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; nic_swsd
> Subject: Re: [PATCH net-next] net/usb: new driver for RTL8152
>
[...]
> > +static inline void set_ethernet_addr(struct r8152 *tp)
> > +{
> > + struct net_device *dev = tp->netdev;
> > + u8 node_id[8] = {0};
> > +
> > + if (pla_ocp_read(tp, PLA_IDR, sizeof(node_id), node_id) < 0)
>
> DMA coherency rules. No buffers on the stack.
Excuse me. I don't understand what you mean. I reference the rtl8150.c and it
uses the same way. Besides, when I check the other drivers, I find the data
pointer of the parameter of the usb_control_msg() may be a pointer of a local
variable from the other functions.
[...]
> > +static void rtl_work_func_t(struct work_struct *work)
> > +{
> > + struct r8152 *tp = container_of(work, struct r8152,
> schedule.work);
> > +
> > + if (!netif_running(tp->netdev))
> > + goto out1;
> > +
> > + if (test_bit(RTL8152_UNPLUG, >flags))
> > + goto out1;
> > +
> > + set_carrier(tp);
> > +
> > + if (test_bit(RTL8152_SET_RX_MODE, >flags))
> > + _rtl8152_set_rx_mode(tp->netdev);
> > +
> > + schedule_delayed_work(>schedule, HZ);
>
> Why does this need to run permanently?
It is used to periodically check the speed, link status, and any other tasks
which need to be finished.
[...]
> > +static int rtl8152_close(struct net_device *netdev)
> > +{
> > + struct r8152 *tp = netdev_priv(netdev);
> > + int res = 0;
> > +
> > + cancel_delayed_work_sync(>schedule);
>
> Looks like a race. What makes sure the work isn't rescheduled?
netif_running would be checked.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next] net/usb: new driver for RTL8152
Oliver Neukum [mailto:oneu...@suse.de]
Sent: Friday, April 26, 2013 7:57 PM
To: Hayeswang
Cc: gre...@linuxfoundation.org; net...@vger.kernel.org;
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; nic_swsd
Subject: Re: [PATCH net-next] net/usb: new driver for RTL8152
[...]
+static inline void set_ethernet_addr(struct r8152 *tp)
+{
+ struct net_device *dev = tp-netdev;
+ u8 node_id[8] = {0};
+
+ if (pla_ocp_read(tp, PLA_IDR, sizeof(node_id), node_id) 0)
DMA coherency rules. No buffers on the stack.
Excuse me. I don't understand what you mean. I reference the rtl8150.c and it
uses the same way. Besides, when I check the other drivers, I find the data
pointer of the parameter of the usb_control_msg() may be a pointer of a local
variable from the other functions.
[...]
+static void rtl_work_func_t(struct work_struct *work)
+{
+ struct r8152 *tp = container_of(work, struct r8152,
schedule.work);
+
+ if (!netif_running(tp-netdev))
+ goto out1;
+
+ if (test_bit(RTL8152_UNPLUG, tp-flags))
+ goto out1;
+
+ set_carrier(tp);
+
+ if (test_bit(RTL8152_SET_RX_MODE, tp-flags))
+ _rtl8152_set_rx_mode(tp-netdev);
+
+ schedule_delayed_work(tp-schedule, HZ);
Why does this need to run permanently?
It is used to periodically check the speed, link status, and any other tasks
which need to be finished.
[...]
+static int rtl8152_close(struct net_device *netdev)
+{
+ struct r8152 *tp = netdev_priv(netdev);
+ int res = 0;
+
+ cancel_delayed_work_sync(tp-schedule);
Looks like a race. What makes sure the work isn't rescheduled?
netif_running would be checked.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3 net-next] net/usb: new driver for RTL8152
Greg KH [mailto:gre...@linuxfoundation.org] Sent: Friday, May 03, 2013 10:33 AM To: Hayeswang Cc: oli...@neukum.org; net...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; nic_swsd Subject: Re: [PATCH v3 net-next] net/usb: new driver for RTL8152 On Fri, May 03, 2013 at 10:01:25AM +0800, Hayes Wang wrote: Add new driver for supporting Realtek RTL8152 Based USB 2.0 Ethernet Adapters Signed-off-by: Hayes Wang hayesw...@realtek.com Cc: Realtek linux nic maintainers nic_s...@realtek.com You removed the wording I had questions about last time, does that mean the previous wording was not correct, or that you no longer are publicising the fact that this driver reads on some patents that you own? You also failed to answer my previous questions, why? That description is about the patent for the hw design, not for software. It indicates that the design of the hw uses those patents from the other compnay. I have no idea about how to deal with those patents. What should I do for sending a patch? Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 net-next 6/8] r8169: add a new chip for RTL8111G
Francois Romieu [mailto:rom...@fr.zoreil.com] > Sent: Wednesday, April 03, 2013 6:27 AM > To: Hayeswang > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 net-next 6/8] r8169: add a new chip > for RTL8111G > > Hayes Wang : > > Add a new chip for RTL8111G series. > > It does not need any of the workarounds in patch #5, right ? Excuse me, I don't sure what is your question. Do you mean if the patch "[PATCH v2 net-next 5/8] r8169: Update the RTL8111G parameters" is necessary before the current patch? According to the settings from our hw engineers, some settings of the new chip are the same with the previous 8111g chip using the new parameters. It would work without patch #5. However, it is necessary to be updated finally, so I add this patch after patch #5. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 net-next 6/8] r8169: add a new chip for RTL8111G
Francois Romieu [mailto:rom...@fr.zoreil.com] Sent: Wednesday, April 03, 2013 6:27 AM To: Hayeswang Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 net-next 6/8] r8169: add a new chip for RTL8111G Hayes Wang hayesw...@realtek.com : Add a new chip for RTL8111G series. It does not need any of the workarounds in patch #5, right ? Excuse me, I don't sure what is your question. Do you mean if the patch [PATCH v2 net-next 5/8] r8169: Update the RTL8111G parameters is necessary before the current patch? According to the settings from our hw engineers, some settings of the new chip are the same with the previous 8111g chip using the new parameters. It would work without patch #5. However, it is necessary to be updated finally, so I add this patch after patch #5. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 6/7] r8169: add a new chip for RTL8106E
Francois Romieu [mailto:rom...@fr.zoreil.com] > Sent: Tuesday, April 02, 2013 6:24 AM > To: Hayeswang > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next 6/7] r8169: add a new chip for RTL8106E > > Hayes Wang : > [...] > > - move rtl_set_rx_tx_desc_registers to avoid the tx/rx are enabled > > before setting desc registers. > > This is a wholesale change for the 810x family. > > Please explain why issuing rtl_set_rx_tx_desc_registers before writing > ChipCmd is not enough and feed it through a standalone commit. > According to the new initial flow of this new chip, the tx/rx would be enabled in rtl_hw_start_8168g_2 function. And this function is run before rtl_set_rx_tx_desc_registers. It would be a problem, so I move rtl_set_rx_tx_desc_registers to make sure that the descriptor address would be set before the tx/rx is enabled. It has no influence for the previous chips, and I think the following new chips would base on the new flow. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 5/7] r8169: add a new chip for RTL8111G
Francois Romieu [mailto:rom...@fr.zoreil.com] > Sent: Tuesday, April 02, 2013 6:23 AM > To: Hayeswang > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next 5/7] r8169: add a new chip for RTL8111G > [..] > There is close to zero added value for this stuff in the kernel. > You may as well move it completely into the firmware. Do you mean all of the phy settings? I have checked these settings with our hw engineers. These are not firmware. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 2/7] r8169: Update PHY settings of RTL8111G
Francois Romieu [mailto:rom...@fr.zoreil.com] > Sent: Tuesday, April 02, 2013 6:21 AM > To: Hayeswang > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next 2/7] r8169: Update PHY settings > of RTL8111G > > Hayes Wang : > > - Replace the current settings with rtl_writephy and rtl_readphy. > > For the hardware, the settings are same with previous ones. This > > make the setting method like the previous chips. > > - Add new PHY settings. > > Would you mind spliting it in two ? OK. [...] > > - if (r8168_phy_ocp_read(tp, 0xa466) & 0x0100) > > - rtl_w1w0_phy_ocp(tp, 0xc41a, 0x0002, 0x); > > - else > > - rtl_w1w0_phy_ocp(tp, 0xbcc4, 0x, 0x0002); > ^^ > This one was not right, was it ? No, it was not right. It seems a mistake for copying and pasting. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 2/7] r8169: Update PHY settings of RTL8111G
Francois Romieu [mailto:rom...@fr.zoreil.com] Sent: Tuesday, April 02, 2013 6:21 AM To: Hayeswang Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next 2/7] r8169: Update PHY settings of RTL8111G Hayes Wang hayesw...@realtek.com : - Replace the current settings with rtl_writephy and rtl_readphy. For the hardware, the settings are same with previous ones. This make the setting method like the previous chips. - Add new PHY settings. Would you mind spliting it in two ? OK. [...] - if (r8168_phy_ocp_read(tp, 0xa466) 0x0100) - rtl_w1w0_phy_ocp(tp, 0xc41a, 0x0002, 0x); - else - rtl_w1w0_phy_ocp(tp, 0xbcc4, 0x, 0x0002); ^^ This one was not right, was it ? No, it was not right. It seems a mistake for copying and pasting. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 5/7] r8169: add a new chip for RTL8111G
Francois Romieu [mailto:rom...@fr.zoreil.com] Sent: Tuesday, April 02, 2013 6:23 AM To: Hayeswang Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next 5/7] r8169: add a new chip for RTL8111G [..] There is close to zero added value for this stuff in the kernel. You may as well move it completely into the firmware. Do you mean all of the phy settings? I have checked these settings with our hw engineers. These are not firmware. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 6/7] r8169: add a new chip for RTL8106E
Francois Romieu [mailto:rom...@fr.zoreil.com] Sent: Tuesday, April 02, 2013 6:24 AM To: Hayeswang Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next 6/7] r8169: add a new chip for RTL8106E Hayes Wang hayesw...@realtek.com : [...] - move rtl_set_rx_tx_desc_registers to avoid the tx/rx are enabled before setting desc registers. This is a wholesale change for the 810x family. Please explain why issuing rtl_set_rx_tx_desc_registers before writing ChipCmd is not enough and feed it through a standalone commit. According to the new initial flow of this new chip, the tx/rx would be enabled in rtl_hw_start_8168g_2 function. And this function is run before rtl_set_rx_tx_desc_registers. It would be a problem, so I move rtl_set_rx_tx_desc_registers to make sure that the descriptor address would be set before the tx/rx is enabled. It has no influence for the previous chips, and I think the following new chips would base on the new flow. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: r8169 auto speed down issue
Francois Romieu [mailto:rom...@fr.zoreil.com] > Sent: Friday, March 29, 2013 3:21 PM > To: Hayeswang > Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; > bowgot...@google.com; Ryankao > Subject: Re: r8169 auto speed down issue > [...] > > I don't get your point. Can you reformulate ? Sorry for my unclear descriptor. I just think a case that the nic suspends or shutdowns without cable plugging. Then, the cable is plugged again. If the nic speed down to 10M and the link partner force 100M, the issue appears again. If the nic doesn't speed down for normal link partner, it requires more power when the linking recovers. Finally, I determine to set the speed to 10M when the link partner supports 10M. And for the other case, setting the speed to 100M. This avoids the giga nic to keep the speed to 1000M, and could fix this issue. However, I wonder if there is a switch which forces the speed to giga. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: r8169 auto speed down issue
Francois Romieu [mailto:rom...@fr.zoreil.com] Sent: Friday, March 29, 2013 3:21 PM To: Hayeswang Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; bowgot...@google.com; Ryankao Subject: Re: r8169 auto speed down issue [...] I don't get your point. Can you reformulate ? Sorry for my unclear descriptor. I just think a case that the nic suspends or shutdowns without cable plugging. Then, the cable is plugged again. If the nic speed down to 10M and the link partner force 100M, the issue appears again. If the nic doesn't speed down for normal link partner, it requires more power when the linking recovers. Finally, I determine to set the speed to 10M when the link partner supports 10M. And for the other case, setting the speed to 100M. This avoids the giga nic to keep the speed to 1000M, and could fix this issue. However, I wonder if there is a switch which forces the speed to giga. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: r8169 auto speed down issue
Francois Romieu [mailto:rom...@fr.zoreil.com] [...] > Your description suggests that testing against the link > partner ability > to work at 10M instead of testing for tp->link_ok could be > good enough. > > Does it make sense ? > Furthermore, should it not speed down without linking, even though the cable would be plugged after suspending or shutdowning? Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: r8169 auto speed down issue
Francois Romieu [mailto:rom...@fr.zoreil.com] [...] Your description suggests that testing against the link partner ability to work at 10M instead of testing for tp-link_ok could be good enough. Does it make sense ? Furthermore, should it not speed down without linking, even though the cable would be plugged after suspending or shutdowning? Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/5] r8169: Modify the method for setting firmware
Francois Romieu [mailto:rom...@fr.zoreil.com]
[...]
>
> Did you check that none of rtl_nic/rtl8168d-{1, 2}.fw uses
> PHY_READ_EFUSE ?
>
I have made sure that none of the current firmwares use PHY_READ_EFUSE.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/5] r8169: Modify the method for setting firmware
Francois Romieu [mailto:rom...@fr.zoreil.com]
[...]
Did you check that none of rtl_nic/rtl8168d-{1, 2}.fw uses
PHY_READ_EFUSE ?
I have made sure that none of the current firmwares use PHY_READ_EFUSE.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving
Francois Romieu [mailto:rom...@fr.zoreil.com]
[...]
> > +static void r810x_aldps_disable(struct rtl8169_private *tp)
> > +{
> > + rtl_writephy(tp, 0x1f, 0x);
> > + rtl_writephy(tp, 0x18, 0x0310);
> > + msleep(100);
> > +}
>
> rtl8402_hw_phy_config used a msleep(20). Meguesses it won't
> hurt, right ?
No, it won't hurt. The delay make suer there is enough time to pause ALDPS.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/2] r8169: enable ALDPS for power saving
Francois Romieu [mailto:rom...@fr.zoreil.com] [...] > It would be nice to state these things in the commit message, namely: > - ALDPS should never be enabled for the RTL8105e > - none of the firmware-free chipsets support ALDPS > - neither do the RTL8168d/8111d Excuse me. I don't understand why the RTL8105e shouldn't enable ALDPS. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/2] r8169: enable ALDPS for power saving
Francois Romieu [mailto:rom...@fr.zoreil.com] [...] It would be nice to state these things in the commit message, namely: - ALDPS should never be enabled for the RTL8105e - none of the firmware-free chipsets support ALDPS - neither do the RTL8168d/8111d Excuse me. I don't understand why the RTL8105e shouldn't enable ALDPS. Best Regards, Hayes -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving
Francois Romieu [mailto:rom...@fr.zoreil.com]
[...]
+static void r810x_aldps_disable(struct rtl8169_private *tp)
+{
+ rtl_writephy(tp, 0x1f, 0x);
+ rtl_writephy(tp, 0x18, 0x0310);
+ msleep(100);
+}
rtl8402_hw_phy_config used a msleep(20). Meguesses it won't
hurt, right ?
No, it won't hurt. The delay make suer there is enough time to pause ALDPS.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/2] r8169: enable ALDPS for power saving
Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Tuesday, October 23, 2012 3:28 AM
> To: Hayeswang
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
> j...@google.com
> Subject: Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving
>
[...]
> > @@ -687,6 +687,7 @@ enum features {
> > RTL_FEATURE_WOL = (1 << 0),
> > RTL_FEATURE_MSI = (1 << 1),
> > RTL_FEATURE_GMII= (1 << 2),
> > + RTL_FEATURE_EXTENDED= (1 << 3),
>
> Is there a specific reason why it is not named RTL_FEATURE_ALDPS ?
>
> RTL_FEATURE_EXTENDED is anything but enlightning.
>
The flag is determined if the firmware is loaded. It doesn't mean to enable
ALDPS. Besides ALDPS, the firmware includes the other features about power
saving, performance, hw behavior, and so on. Thus, I think it is the extended
feature. Any suggestion?
[...]
> > @@ -6391,6 +6433,8 @@ static void
> rtl8169_net_suspend(struct net_device *dev)
> > {
> > struct rtl8169_private *tp = netdev_priv(dev);
> >
> > + tp->features &= ~RTL_FEATURE_EXTENDED;
> > +
>
> The commit message does not explain this part.
>
> What are you trying to achieve ?
I don't sure if the firmware code still exists and works after hibernation. I
clear the flag for safe. It would be set again after loading firmware. It is
used to make sure to enable ALDPS with firmware.
>
> After this patch the driver would look like:
> 1. disable ALDPS before setting firmware (unmodified by patch)
>RTL_GIGA_MAC_VER_29 "RTL8105e"
>RTL_GIGA_MAC_VER_30 "RTL8105e"
>RTL_GIGA_MAC_VER_37 "RTL8402"
>RTL_GIGA_MAC_VER_39 "RTL8106e"
>
> 2. apply_firmware (unmodified by patch)
>RTL_GIGA_MAC_VER_25 "RTL8168d/8111d"
>RTL_GIGA_MAC_VER_26 "RTL8168d/8111d"
>RTL_GIGA_MAC_VER_29 "RTL8105e"
>RTL_GIGA_MAC_VER_30 "RTL8105e"
>RTL_GIGA_MAC_VER_32 "RTL8168e/8111e"
>RTL_GIGA_MAC_VER_33 "RTL8168e/8111e"
>RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl"
>RTL_GIGA_MAC_VER_35 "RTL8168f/8111f"
>RTL_GIGA_MAC_VER_36 "RTL8168f/8111f"
>RTL_GIGA_MAC_VER_37 "RTL8402"
>RTL_GIGA_MAC_VER_38 "RTL8411"
>RTL_GIGA_MAC_VER_39 "RTL8106e"
>RTL_GIGA_MAC_VER_40 "RTL8168g/8111g"
>
> 3. enable ALDPS after firmware
>RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl"
>RTL_GIGA_MAC_VER_35 "RTL8168f/8111f"
>RTL_GIGA_MAC_VER_36 "RTL8168f/8111f"
>RTL_GIGA_MAC_VER_37 "RTL8402"
>RTL_GIGA_MAC_VER_38 "RTL8411"
>RTL_GIGA_MAC_VER_39 "RTL8106e"
>RTL_GIGA_MAC_VER_40 "RTL8168g/8111g"
>
> The disable/enable ALDPS code is not trivially balanced.
>
> Do we exactly perform the required ALDPS operations ? Nothing more,
> nothing less ?
>
Because the different design between the giga nic and 10/100M nic, the behavior
would be different. For example, you couldn't disable the ALDPS of the giga nic
directly just like the 10/100M (8136 series) one. The giga nic would disable
ALDPS automatically when loading firmware, but the 8136 series wouldn't.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next 1/2] r8169: enable ALDPS for power saving
Francois Romieu [mailto:rom...@fr.zoreil.com]
Sent: Tuesday, October 23, 2012 3:28 AM
To: Hayeswang
Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
j...@google.com
Subject: Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving
[...]
@@ -687,6 +687,7 @@ enum features {
RTL_FEATURE_WOL = (1 0),
RTL_FEATURE_MSI = (1 1),
RTL_FEATURE_GMII= (1 2),
+ RTL_FEATURE_EXTENDED= (1 3),
Is there a specific reason why it is not named RTL_FEATURE_ALDPS ?
RTL_FEATURE_EXTENDED is anything but enlightning.
The flag is determined if the firmware is loaded. It doesn't mean to enable
ALDPS. Besides ALDPS, the firmware includes the other features about power
saving, performance, hw behavior, and so on. Thus, I think it is the extended
feature. Any suggestion?
[...]
@@ -6391,6 +6433,8 @@ static void
rtl8169_net_suspend(struct net_device *dev)
{
struct rtl8169_private *tp = netdev_priv(dev);
+ tp-features = ~RTL_FEATURE_EXTENDED;
+
The commit message does not explain this part.
What are you trying to achieve ?
I don't sure if the firmware code still exists and works after hibernation. I
clear the flag for safe. It would be set again after loading firmware. It is
used to make sure to enable ALDPS with firmware.
After this patch the driver would look like:
1. disable ALDPS before setting firmware (unmodified by patch)
RTL_GIGA_MAC_VER_29 RTL8105e
RTL_GIGA_MAC_VER_30 RTL8105e
RTL_GIGA_MAC_VER_37 RTL8402
RTL_GIGA_MAC_VER_39 RTL8106e
2. apply_firmware (unmodified by patch)
RTL_GIGA_MAC_VER_25 RTL8168d/8111d
RTL_GIGA_MAC_VER_26 RTL8168d/8111d
RTL_GIGA_MAC_VER_29 RTL8105e
RTL_GIGA_MAC_VER_30 RTL8105e
RTL_GIGA_MAC_VER_32 RTL8168e/8111e
RTL_GIGA_MAC_VER_33 RTL8168e/8111e
RTL_GIGA_MAC_VER_34 RTL8168evl/8111evl
RTL_GIGA_MAC_VER_35 RTL8168f/8111f
RTL_GIGA_MAC_VER_36 RTL8168f/8111f
RTL_GIGA_MAC_VER_37 RTL8402
RTL_GIGA_MAC_VER_38 RTL8411
RTL_GIGA_MAC_VER_39 RTL8106e
RTL_GIGA_MAC_VER_40 RTL8168g/8111g
3. enable ALDPS after firmware
RTL_GIGA_MAC_VER_34 RTL8168evl/8111evl
RTL_GIGA_MAC_VER_35 RTL8168f/8111f
RTL_GIGA_MAC_VER_36 RTL8168f/8111f
RTL_GIGA_MAC_VER_37 RTL8402
RTL_GIGA_MAC_VER_38 RTL8411
RTL_GIGA_MAC_VER_39 RTL8106e
RTL_GIGA_MAC_VER_40 RTL8168g/8111g
The disable/enable ALDPS code is not trivially balanced.
Do we exactly perform the required ALDPS operations ? Nothing more,
nothing less ?
Because the different design between the giga nic and 10/100M nic, the behavior
would be different. For example, you couldn't disable the ALDPS of the giga nic
directly just like the 10/100M (8136 series) one. The giga nic would disable
ALDPS automatically when loading firmware, but the 8136 series wouldn't.
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH net-next] r8169: Remove rtl_ocpdr_cond
Francois Romieu [mailto:rom...@fr.zoreil.com] [...] > > W/o firmware does not seem to make a difference. > > # ping -qf -l 4 -s 81 -c 60 10.0.3.1 > PING 10.0.3.1 (10.0.3.1) 81(109) bytes of data. > > --- 10.0.3.1 ping statistics --- > 60 packets transmitted, 60 received, 0% packet loss, time 153ms > rtt min/avg/max/mdev = 0.047/0.064/0.117/0.016 ms, pipe 4, > ipg/ewma 2.607/0.058 ms # ping -qf -l 4 -s 81 -c 60 192.168.94.20 PING 192.168.94.20 (192.168.94.20) 81(109) bytes of data. --- 192.168.94.20 ping statistics --- 60 packets transmitted, 57 received, 5% packet loss, time 1ms rtt min/avg/max/mdev = 0.028/0.040/0.101/0.011 ms, pipe 4, ipg/ewma 0.021/0.035 ms > # ping -qf -l 4 -s 82 -c 60 10.0.3.1 > > PING 10.0.3.1 (10.0.3.1) 82(110) bytes of data. > > --- 10.0.3.1 ping statistics --- > 60 packets transmitted, 60 received, 0% packet loss, time 3ms > rtt min/avg/max/mdev = 0.195/0.210/0.281/0.018 ms, pipe 4, > ipg/ewma 0.057/0.205 ms # ping -qf -l 4 -s 82 -c 60 192.168.94.20 PING 192.168.94.20 (192.168.94.20) 82(110) bytes of data. --- 192.168.94.20 ping statistics --- 60 packets transmitted, 60 received, 0% packet loss, time 2ms rtt min/avg/max/mdev = 0.151/0.173/0.247/0.020 ms, pipe 4, ipg/ewma 0.048/0.168 ms > It would translate into a 127/128 cutoff after inclusion of the FCS. > > Any idea ? > The attatched file is my log. It seems fine. a.log Description: Binary data
RE: [PATCH net-next] r8169: Remove rtl_ocpdr_cond
Francois Romieu [mailto:rom...@fr.zoreil.com] [...] W/o firmware does not seem to make a difference. # ping -qf -l 4 -s 81 -c 60 10.0.3.1 PING 10.0.3.1 (10.0.3.1) 81(109) bytes of data. --- 10.0.3.1 ping statistics --- 60 packets transmitted, 60 received, 0% packet loss, time 153ms rtt min/avg/max/mdev = 0.047/0.064/0.117/0.016 ms, pipe 4, ipg/ewma 2.607/0.058 ms # ping -qf -l 4 -s 81 -c 60 192.168.94.20 PING 192.168.94.20 (192.168.94.20) 81(109) bytes of data. --- 192.168.94.20 ping statistics --- 60 packets transmitted, 57 received, 5% packet loss, time 1ms rtt min/avg/max/mdev = 0.028/0.040/0.101/0.011 ms, pipe 4, ipg/ewma 0.021/0.035 ms # ping -qf -l 4 -s 82 -c 60 10.0.3.1 PING 10.0.3.1 (10.0.3.1) 82(110) bytes of data. --- 10.0.3.1 ping statistics --- 60 packets transmitted, 60 received, 0% packet loss, time 3ms rtt min/avg/max/mdev = 0.195/0.210/0.281/0.018 ms, pipe 4, ipg/ewma 0.057/0.205 ms # ping -qf -l 4 -s 82 -c 60 192.168.94.20 PING 192.168.94.20 (192.168.94.20) 82(110) bytes of data. --- 192.168.94.20 ping statistics --- 60 packets transmitted, 60 received, 0% packet loss, time 2ms rtt min/avg/max/mdev = 0.151/0.173/0.247/0.020 ms, pipe 4, ipg/ewma 0.048/0.168 ms It would translate into a 127/128 cutoff after inclusion of the FCS. Any idea ? The attatched file is my log. It seems fine. a.log Description: Binary data
