| Message ID | 1434378443-23029-2-git-send-email-simon.guinot@sequanux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 15, 2015 at 04:27:22PM +0200, Simon Guinot wrote: > This patch introduces the tx_csum_limit DT property. This allows to > configure the maximum frame size for which the Ethernet controller is > able to perform TCP/IP checksumming. If MTU is set to a value greater > than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO > are disabled. > > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> > Cc: <stable@vger.kernel.org> # v3.8+ Sorry, I just tripped over this. This looks like a patch adding a new _feature_ to this ethernet driver. It isn't a regression fix, and it isn't a bug fix. Why are you wanting to get it into stable kernels, which are supposed to only have bug and regression fixes applied?
Russell, On Mon, 15 Jun 2015 15:36:01 +0100, Russell King - ARM Linux wrote: > On Mon, Jun 15, 2015 at 04:27:22PM +0200, Simon Guinot wrote: > > This patch introduces the tx_csum_limit DT property. This allows to > > configure the maximum frame size for which the Ethernet controller is > > able to perform TCP/IP checksumming. If MTU is set to a value greater > > than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO > > are disabled. > > > > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> > > Cc: <stable@vger.kernel.org> # v3.8+ > > Sorry, I just tripped over this. This looks like a patch adding a new > _feature_ to this ethernet driver. It isn't a regression fix, and it > isn't a bug fix. Why are you wanting to get it into stable kernels, > which are supposed to only have bug and regression fixes applied? Look at the cover letter, it is fixing a bug. Thomas
Dear Simon Guinot, On Mon, 15 Jun 2015 16:27:22 +0200, Simon Guinot wrote: > This patch introduces the tx_csum_limit DT property. This allows to > configure the maximum frame size for which the Ethernet controller is > able to perform TCP/IP checksumming. If MTU is set to a value greater > than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO > are disabled. > > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> > Cc: <stable@vger.kernel.org> # v3.8+ > --- > .../bindings/net/marvell-armada-370-neta.txt | 3 +++ > drivers/net/ethernet/marvell/mvneta.c | 25 +++++++++++++++++++++- > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > index 750d577e8083..db48c83ff0f5 100644 > --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > @@ -8,6 +8,9 @@ Required properties: > - phy-mode: See ethernet.txt file in the same directory > - clocks: a pointer to the reference clock for this device. > > +Optional properties: > +- tx_csum_limit: max tx packet size for hardware checksum. To be honest, I'd prefer to have a different compatible string to identify the two different versions of the hardware block. The current armada-370-neta would limit the HW checksumming features to packets smaller than 1600 bytes, while a new armada-xp-neta would not have this limit. Yet another case where we should have used "armada-<soc>-neta", "armada-370-neta" in the .dtsi files for each SoC so that such modification do not require changing the Device Trees. Best regards, Thomas
On Mon, Jun 15, 2015 at 04:49:52PM +0200, Thomas Petazzoni wrote: > Dear Simon Guinot, > > On Mon, 15 Jun 2015 16:27:22 +0200, Simon Guinot wrote: > > This patch introduces the tx_csum_limit DT property. This allows to > > configure the maximum frame size for which the Ethernet controller is > > able to perform TCP/IP checksumming. If MTU is set to a value greater > > than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO > > are disabled. > > > > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> > > Cc: <stable@vger.kernel.org> # v3.8+ > > --- > > .../bindings/net/marvell-armada-370-neta.txt | 3 +++ > > drivers/net/ethernet/marvell/mvneta.c | 25 +++++++++++++++++++++- > > 2 files changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > > index 750d577e8083..db48c83ff0f5 100644 > > --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > > +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt > > @@ -8,6 +8,9 @@ Required properties: > > - phy-mode: See ethernet.txt file in the same directory > > - clocks: a pointer to the reference clock for this device. > > > > +Optional properties: > > +- tx_csum_limit: max tx packet size for hardware checksum. > > To be honest, I'd prefer to have a different compatible string to > identify the two different versions of the hardware block. > > The current armada-370-neta would limit the HW checksumming features to > packets smaller than 1600 bytes, while a new armada-xp-neta would not > have this limit. This was also my first idea. But by doing this, we take the risk of losing the HW checksumming feature with jumbo frames on some currently working Armada XP setups. This may happen for example if a user is able to update the kernel but not the on-board DTB. In order to fix a feature on a SoC, we are breaking the DTB-kernel compatibility for the very same feature on an another SoC. I am not sure it is OK. Are you comfortable with that ? By adding a new optional property, at least we ensure that the things will still work the same way with Armada-XP SoCs (for every DTB-Linux combination). Please, let me know if you still want to introduce a compatible string for Armada-XP. Simon > > Yet another case where we should have used "armada-<soc>-neta", > "armada-370-neta" in the .dtsi files for each SoC so that such > modification do not require changing the Device Trees. > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
Hello, On Mon, 15 Jun 2015 17:54:41 +0200, Simon Guinot wrote: > > The current armada-370-neta would limit the HW checksumming features to > > packets smaller than 1600 bytes, while a new armada-xp-neta would not > > have this limit. > > This was also my first idea. But by doing this, we take the risk of > losing the HW checksumming feature with jumbo frames on some currently > working Armada XP setups. This may happen for example if a user is able > to update the kernel but not the on-board DTB. In order to fix a feature > on a SoC, we are breaking the DTB-kernel compatibility for the very same > feature on an another SoC. I am not sure it is OK. > > Are you comfortable with that ? It's either that, or keep a not-ideal solution for this problem. I'm not a strong believer in the need of enforcing the DT as a stable ABI, and I am not aware of anyone using Marvell platforms with a DTB that isn't changed together with the kernel. So my preference goes to fixing the problem properly, rather than doing non-ideal solutions. Though this is just my own opinion, and I am not a maintainer of the mach-mvebu platform. Thomas
Thomas, Simon, On Tue, Jun 16, 2015 at 12:00:34PM +0200, Thomas Petazzoni wrote: > On Mon, 15 Jun 2015 17:54:41 +0200, Simon Guinot wrote: > > > The current armada-370-neta would limit the HW checksumming features to > > > packets smaller than 1600 bytes, while a new armada-xp-neta would not > > > have this limit. > > > > This was also my first idea. But by doing this, we take the risk of > > losing the HW checksumming feature with jumbo frames on some currently > > working Armada XP setups. This may happen for example if a user is able > > to update the kernel but not the on-board DTB. In order to fix a feature > > on a SoC, we are breaking the DTB-kernel compatibility for the very same > > feature on an another SoC. I am not sure it is OK. Frankly, this isn't a realistic scenario. We've said from day one that if the dtb is provided with the board that it needs to be updateable. For exactly these kinds of situations. Also, Thomas' assessment is correct, everyone we've ever spoken to is keeping the dtb in sync with the kernel. As long as a board with the old dtb boots a newer kernel without crashing, then it's fine. afaict in this situation, the updated driver should limit HW checksumming for packets >1600 bytes if the compatible string is 'armada-370-neta'. Regardless of actual SoC underneath. If the driver gets 'armada-xp-neta' then there is no checksum limit. Users with an Armada XP SoC and an old dtb will need to upgrade the dtb in order to make use of HW checksumming on jumbo packets with newer kernels. Seems sane to me. thx, Jason.
diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt index 750d577e8083..db48c83ff0f5 100644 --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt @@ -8,6 +8,9 @@ Required properties: - phy-mode: See ethernet.txt file in the same directory - clocks: a pointer to the reference clock for this device. +Optional properties: +- tx_csum_limit: max tx packet size for hardware checksum. + Example: ethernet@d0070000 { diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index ce5f7f9cff06..1a724e0a2a26 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -310,6 +310,7 @@ struct mvneta_port { unsigned int link; unsigned int duplex; unsigned int speed; + unsigned int tx_csum_limit; int use_inband_status:1; }; @@ -2502,8 +2503,10 @@ static int mvneta_change_mtu(struct net_device *dev, int mtu) dev->mtu = mtu; - if (!netif_running(dev)) + if (!netif_running(dev)) { + netdev_update_features(dev); return 0; + } /* The interface is running, so we have to force a * reallocation of the queues @@ -2532,9 +2535,26 @@ static int mvneta_change_mtu(struct net_device *dev, int mtu) mvneta_start_dev(pp); mvneta_port_up(pp); + netdev_update_features(dev); + return 0; } +static netdev_features_t mvneta_fix_features(struct net_device *dev, + netdev_features_t features) +{ + struct mvneta_port *pp = netdev_priv(dev); + + if (pp->tx_csum_limit && dev->mtu > pp->tx_csum_limit) { + features &= ~(NETIF_F_IP_CSUM | NETIF_F_TSO); + netdev_info(dev, + "Disable IP checksum for MTU greater than %dB\n", + pp->tx_csum_limit); + } + + return features; +} + /* Get mac address */ static void mvneta_get_mac_addr(struct mvneta_port *pp, unsigned char *addr) { @@ -2856,6 +2876,7 @@ static const struct net_device_ops mvneta_netdev_ops = { .ndo_set_rx_mode = mvneta_set_rx_mode, .ndo_set_mac_address = mvneta_set_mac_addr, .ndo_change_mtu = mvneta_change_mtu, + .ndo_fix_features = mvneta_fix_features, .ndo_get_stats64 = mvneta_get_stats64, .ndo_do_ioctl = mvneta_ioctl, }; @@ -3101,6 +3122,8 @@ static int mvneta_probe(struct platform_device *pdev) } } + of_property_read_u32(dn, "tx_csum_limit", &pp->tx_csum_limit); + pp->tx_ring_size = MVNETA_MAX_TXD; pp->rx_ring_size = MVNETA_MAX_RXD;
This patch introduces the tx_csum_limit DT property. This allows to configure the maximum frame size for which the Ethernet controller is able to perform TCP/IP checksumming. If MTU is set to a value greater than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO are disabled. Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> Cc: <stable@vger.kernel.org> # v3.8+ --- .../bindings/net/marvell-armada-370-neta.txt | 3 +++ drivers/net/ethernet/marvell/mvneta.c | 25 +++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-)