[PATCH 00/10] net: random cleanups

[PATCH 00/10] net: random cleanups

[Michał Mirosław]

This is a set of random cleanups accumulated while digging through the
network device drivers. Only patches that don't depend on removing
old ethtool ops are included here.

The series consists of patches that can be applied independently from
others from the set. The exception is patch #7 that depends on removing
NETIF_F_NO_CSUM references in patches 1, 2 and 5.

---

Michał Mirosław (10):
  net: sctp: fix checksum marking for outgoing packets
  net: m68k/nfeth: Remove wrong usage of dev->flags
  net: vlan: remove reduntant check in ndo_fix_features callback
  net: cleanup vlan_features setting in register_netdev
  net: Disable NOCACHE_COPY by default
  net: remove NETIF_F_ALL_TX_OFFLOADS
  net: remove NETIF_F_NO_CSUM feature
  net: unexport netdev_fix_features()
  net: remove /sys/class/net/*/features
  net: remove SK_ROUTE_CAPS from meta ematch

 arch/m68k/emu/nfeth.c                |    1 -
 drivers/ieee802154/fakehard.c        |    2 +-
 drivers/misc/sgi-xp/xpnet.c          |    2 +-
 drivers/net/bonding/bond_main.c      |    8 ++++----
 drivers/net/can/dev.c                |    2 +-
 drivers/net/can/slcan.c              |    2 +-
 drivers/net/dummy.c                  |    2 +-
 drivers/net/ifb.c                    |    2 +-
 drivers/net/loopback.c               |    2 +-
 drivers/net/veth.c                   |    2 +-
 include/linux/netdevice.h            |   10 +---------
 include/linux/skbuff.h               |    1 -
 include/linux/tc_ematch/tc_em_meta.h |    2 +-
 net/8021q/vlan_dev.c                 |   11 +++++++----
 net/bridge/br_device.c               |    4 ++--
 net/core/dev.c                       |   33 +++++++++------------------------
 net/core/ethtool.c                   |    2 +-
 net/core/net-sysfs.c                 |    2 --
 net/sched/em_meta.c                  |    7 -------
 net/sctp/output.c                    |   19 ++++++++-----------
 20 files changed, 41 insertions(+), 75 deletions(-)

-- 
1.7.5.4

[PATCH 01/10] net: sctp: fix checksum marking for outgoing packets

[Michał Mirosław]

Packets to devices without NETIF_F_SCTP_CSUM (including NETIF_F_NO_CSUM)
should be properly checksummed because the packets can be diverted or
rerouted after construction. This still leaves packets diverted from
NETIF_F_SCTP_CSUM-enabled devices with broken checksums. Fixing this
needs implementing software offload fallback in networking core.

For users of sctp_checksum_disable, skb->ip_summed should be left as
CHECKSUM_NONE and not CHECKSUM_UNNECESSARY as per include/linux/skbuff.h.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/sctp/output.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/net/sctp/output.c b/net/sctp/output.c
index b4f3cf0..08b3cea 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -500,23 +500,20 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	 * Note: Adler-32 is no longer applicable, as has been replaced
 	 * by CRC32-C as described in <draft-ietf-tsvwg-sctpcsum-02.txt>.
 	 */
-	if (!sctp_checksum_disable &&
-	    !(dst->dev->features & (NETIF_F_NO_CSUM | NETIF_F_SCTP_CSUM))) {
-		__u32 crc32 = sctp_start_cksum((__u8 *)sh, cksum_buf_len);
+	if (!sctp_checksum_disable) {
+		if (!(dst->dev->features & NETIF_F_SCTP_CSUM)) {
+			__u32 crc32 = sctp_start_cksum((__u8 *)sh, cksum_buf_len);
 
-		/* 3) Put the resultant value into the checksum field in the
-		 *    common header, and leave the rest of the bits unchanged.
-		 */
-		sh->checksum = sctp_end_cksum(crc32);
-	} else {
-		if (dst->dev->features & NETIF_F_SCTP_CSUM) {
+			/* 3) Put the resultant value into the checksum field in the
+			 *    common header, and leave the rest of the bits unchanged.
+			 */
+			sh->checksum = sctp_end_cksum(crc32);
+		} else {
 			/* no need to seed pseudo checksum for SCTP */
 			nskb->ip_summed = CHECKSUM_PARTIAL;
 			nskb->csum_start = (skb_transport_header(nskb) -
 			                    nskb->head);
 			nskb->csum_offset = offsetof(struct sctphdr, checksum);
-		} else {
-			nskb->ip_summed = CHECKSUM_UNNECESSARY;
 		}
 	}
 
-- 
1.7.5.4

Re: [PATCH 01/10] net: sctp: fix checksum marking for outgoing packets

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 02:10:29 +0200 (CEST)

> Packets to devices without NETIF_F_SCTP_CSUM (including NETIF_F_NO_CSUM)
> should be properly checksummed because the packets can be diverted or
> rerouted after construction. This still leaves packets diverted from
> NETIF_F_SCTP_CSUM-enabled devices with broken checksums. Fixing this
> needs implementing software offload fallback in networking core.
> 
> For users of sctp_checksum_disable, skb->ip_summed should be left as
> CHECKSUM_NONE and not CHECKSUM_UNNECESSARY as per include/linux/skbuff.h.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

This is not the way to fix this.

We need to add the proper software fallback when the SKB device
changes to one with SCTP/NO_CSUM to one that does not.

[PATCH 06/10] net: remove NETIF_F_ALL_TX_OFFLOADS

[Michał Mirosław]

There is no software fallback implemented for SCTP or FCoE checksumming,
and so it should not be passed on by software devices like bridge or bonding.

For VLAN devices, this is different. First, the driver for underlying device
should be prepared to get offloaded packets even when the feature is disabled
(especially if it advertises it in vlan_features). Second, devices under
VLANs do not get replaced without tearing down the VLAN first.

This fixes a mess I accidentally introduced while converting bonding to
ndo_fix_features.

NETIF_F_SOFT_FEATURES are removed from BOND_VLAN_FEATURES because they
are unused as of commit 712ae51afd.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/bonding/bond_main.c |    6 +++---
 include/linux/netdevice.h       |    6 ------
 net/8021q/vlan_dev.c            |    6 +++++-
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 61265f7..b9eaf5c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1415,9 +1415,9 @@ out:
 	return features;
 }
 
-#define BOND_VLAN_FEATURES	(NETIF_F_ALL_TX_OFFLOADS | \
-				 NETIF_F_SOFT_FEATURES | \
-				 NETIF_F_LRO)
+#define BOND_VLAN_FEATURES	(NETIF_F_ALL_CSUM | NETIF_F_SG | \
+				 NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \
+				 NETIF_F_HIGHDMA | NETIF_F_LRO)
 
 static void bond_compute_features(struct bonding *bond)
 {
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7538237..faa415d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1084,12 +1084,6 @@ struct net_device {
 #define NETIF_F_ALL_FCOE	(NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
 				 NETIF_F_FSO)
 
-#define NETIF_F_ALL_TX_OFFLOADS	(NETIF_F_ALL_CSUM | NETIF_F_SG | \
-				 NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \
-				 NETIF_F_HIGHDMA | \
-				 NETIF_F_SCTP_CSUM | \
-				 NETIF_F_ALL_FCOE)
-
 	/*
 	 * If one device supports one of these features, then enable them
 	 * for all in netdev_increment_features.
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 7f67f2e..3db056b 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -530,7 +530,11 @@ static int vlan_dev_init(struct net_device *dev)
 					  (1<<__LINK_STATE_DORMANT))) |
 		      (1<<__LINK_STATE_PRESENT);
 
-	dev->hw_features = NETIF_F_ALL_TX_OFFLOADS;
+	dev->hw_features = NETIF_F_ALL_CSUM | NETIF_F_SG |
+			   NETIF_F_FRAGLIST | NETIF_F_ALL_TSO |
+			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CSUM |
+			   NETIF_F_ALL_FCOE;
+
 	dev->features |= real_dev->vlan_features | NETIF_F_LLTX;
 	dev->gso_max_size = real_dev->gso_max_size;
 
-- 
1.7.5.4

Re: [PATCH 06/10] net: remove NETIF_F_ALL_TX_OFFLOADS

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 02:10:29 +0200 (CEST)

> There is no software fallback implemented for SCTP or FCoE checksumming,
> and so it should not be passed on by software devices like bridge or bonding.
> 
> For VLAN devices, this is different. First, the driver for underlying device
> should be prepared to get offloaded packets even when the feature is disabled
> (especially if it advertises it in vlan_features). Second, devices under
> VLANs do not get replaced without tearing down the VLAN first.
> 
> This fixes a mess I accidentally introduced while converting bonding to
> ndo_fix_features.
> 
> NETIF_F_SOFT_FEATURES are removed from BOND_VLAN_FEATURES because they
> are unused as of commit 712ae51afd.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

This is not the answer.

We have to implement software fallbacks for the checksum offloads.

That is easier than having this ever growing set of exceptions for
feature bit propagation, which at best will end up being a steaming
pile of unmaintainable poo.

Re: [PATCH 06/10] net: remove NETIF_F_ALL_TX_OFFLOADS

[Michał Mirosław]

On Thu, Jul 14, 2011 at 02:43:14PM -0700, David Miller wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Date: Thu, 14 Jul 2011 02:10:29 +0200 (CEST)
> 
> > There is no software fallback implemented for SCTP or FCoE checksumming,
> > and so it should not be passed on by software devices like bridge or bonding.
> > 
> > For VLAN devices, this is different. First, the driver for underlying device
> > should be prepared to get offloaded packets even when the feature is disabled
> > (especially if it advertises it in vlan_features). Second, devices under
> > VLANs do not get replaced without tearing down the VLAN first.
> > 
> > This fixes a mess I accidentally introduced while converting bonding to
> > ndo_fix_features.
> > 
> > NETIF_F_SOFT_FEATURES are removed from BOND_VLAN_FEATURES because they
> > are unused as of commit 712ae51afd.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> This is not the answer.
> 
> We have to implement software fallbacks for the checksum offloads.
> 
> That is easier than having this ever growing set of exceptions for
> feature bit propagation, which at best will end up being a steaming
> pile of unmaintainable poo.

I agree about the fallbacks, but this patch is about fixing a regression
that's going to hit 3.0. Now all bonding devices always advertise e.g.
SCTP_CSUM. Sorry if I was unclear about this.

Best Regards,
Michał Mirosław

Re: [PATCH 06/10] net: remove NETIF_F_ALL_TX_OFFLOADS

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 23:54:01 +0200

> I agree about the fallbacks, but this patch is about fixing a regression
> that's going to hit 3.0. Now all bonding devices always advertise e.g.
> SCTP_CSUM. Sorry if I was unclear about this.

Why are you mixing cleanups and bug fixes?

[PATCH 1/1 net] net: fix bonding advertising offloads it can't support

[Michał Mirosław]

There is no software fallback implemented for SCTP or FCoE checksumming,
and so it should not be passed on by software devices like bridge or
bonding.

For VLAN devices, this is different. First, the driver for underlying
device should be prepared to get offloaded packets even when the feature
is disabled (especially if it advertises it in vlan_features). Second,
devices under VLANs do not get replaced without tearing down the VLAN
first.

This fixes a mess I accidentally introduced while converting bonding to
ndo_fix_features in b2a103e6d0.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/bonding/bond_main.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index eafe44a..63c22b0 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1428,9 +1428,9 @@ out:
 	return features;
 }
 
-#define BOND_VLAN_FEATURES	(NETIF_F_ALL_TX_OFFLOADS | \
-				 NETIF_F_SOFT_FEATURES | \
-				 NETIF_F_LRO)
+#define BOND_VLAN_FEATURES	(NETIF_F_ALL_CSUM | NETIF_F_SG | \
+				 NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \
+				 NETIF_F_HIGHDMA | NETIF_F_LRO)
 
 static void bond_compute_features(struct bonding *bond)
 {
-- 
1.7.5.4

Re: [PATCH 1/1 net] net: fix bonding advertising offloads it can't support

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Fri, 15 Jul 2011 00:34:50 +0200 (CEST)

> There is no software fallback implemented for SCTP or FCoE checksumming,
> and so it should not be passed on by software devices like bridge or
> bonding.
> 
> For VLAN devices, this is different. First, the driver for underlying
> device should be prepared to get offloaded packets even when the feature
> is disabled (especially if it advertises it in vlan_features). Second,
> devices under VLANs do not get replaced without tearing down the VLAN
> first.
> 
> This fixes a mess I accidentally introduced while converting bonding to
> ndo_fix_features in b2a103e6d0.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

I already applied this, and the SCTP patch, to net-2.6

[PATCH 02/10] net: m68k/nfeth: Remove wrong usage of dev->flags

[Michał Mirosław]

Remove wrong setting of dev->flags. NETIF_F_NO_CSUM maps to IFF_DEBUG
there, so looks like a mistake.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 arch/m68k/emu/nfeth.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/m68k/emu/nfeth.c b/arch/m68k/emu/nfeth.c
index 3c55312..c5748bb 100644
--- a/arch/m68k/emu/nfeth.c
+++ b/arch/m68k/emu/nfeth.c
@@ -205,7 +205,6 @@ static struct net_device * __init nfeth_probe(int unit)
 	dev->irq = nfEtherIRQ;
 	dev->netdev_ops = &nfeth_netdev_ops;
 
-	dev->flags |= NETIF_F_NO_CSUM;
 	memcpy(dev->dev_addr, mac, ETH_ALEN);
 
 	priv = netdev_priv(dev);
-- 
1.7.5.4

Re: [PATCH 02/10] net: m68k/nfeth: Remove wrong usage of dev->flags

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 02:10:29 +0200 (CEST)

> Remove wrong setting of dev->flags. NETIF_F_NO_CSUM maps to IFF_DEBUG
> there, so looks like a mistake.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.

[PATCH 03/10] net: vlan: remove reduntant check in ndo_fix_features callback

[Michał Mirosław]

Use the fact that ORing with zero is a no-op.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/8021q/vlan_dev.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index d8f45ba..7f67f2e 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -593,8 +593,7 @@ static u32 vlan_dev_fix_features(struct net_device *dev, u32 features)
 	features &= real_dev->features;
 	features &= real_dev->vlan_features;
 
-	if (old_features & NETIF_F_SOFT_FEATURES)
-		features |= old_features & NETIF_F_SOFT_FEATURES;
+	features |= old_features & NETIF_F_SOFT_FEATURES;
 
 	if (dev_ethtool_get_rx_csum(real_dev))
 		features |= NETIF_F_RXCSUM;
-- 
1.7.5.4

Re: [PATCH 03/10] net: vlan: remove reduntant check in ndo_fix_features callback

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 02:10:29 +0200 (CEST)

> Use the fact that ORing with zero is a no-op.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied, I was going to suggest this when Shan Wei's bug fix went
in :-)

[PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[Michał Mirosław]

There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/ieee802154/fakehard.c   |    2 +-
 drivers/misc/sgi-xp/xpnet.c     |    2 +-
 drivers/net/bonding/bond_main.c |    2 +-
 drivers/net/can/dev.c           |    2 +-
 drivers/net/can/slcan.c         |    2 +-
 drivers/net/dummy.c             |    2 +-
 drivers/net/ifb.c               |    2 +-
 drivers/net/loopback.c          |    2 +-
 drivers/net/veth.c              |    2 +-
 include/linux/netdevice.h       |    3 +--
 include/linux/skbuff.h          |    1 -
 net/bridge/br_device.c          |    4 ++--
 net/core/dev.c                  |   12 +-----------
 net/core/ethtool.c              |    2 +-
 14 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/drivers/ieee802154/fakehard.c b/drivers/ieee802154/fakehard.c
index eb0e2cc..73d4531 100644
--- a/drivers/ieee802154/fakehard.c
+++ b/drivers/ieee802154/fakehard.c
@@ -343,7 +343,7 @@ static void ieee802154_fake_setup(struct net_device *dev)
 {
 	dev->addr_len		= IEEE802154_ADDR_LEN;
 	memset(dev->broadcast, 0xff, IEEE802154_ADDR_LEN);
-	dev->features		= NETIF_F_NO_CSUM;
+	dev->features		= NETIF_F_HW_CSUM;
 	dev->needed_tailroom	= 2; /* FCS */
 	dev->mtu		= 127;
 	dev->tx_queue_len	= 10;
diff --git a/drivers/misc/sgi-xp/xpnet.c b/drivers/misc/sgi-xp/xpnet.c
index 42f0673..3fac67a 100644
--- a/drivers/misc/sgi-xp/xpnet.c
+++ b/drivers/misc/sgi-xp/xpnet.c
@@ -576,7 +576,7 @@ xpnet_init(void)
 	 * report an error if the data is not retrievable and the
 	 * packet will be dropped.
 	 */
-	xpnet_device->features = NETIF_F_NO_CSUM;
+	xpnet_device->features = NETIF_F_HW_CSUM;
 
 	result = register_netdev(xpnet_device);
 	if (result != 0) {
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b9eaf5c..90844a9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4409,7 +4409,7 @@ static void bond_setup(struct net_device *bond_dev)
 				NETIF_F_HW_VLAN_RX |
 				NETIF_F_HW_VLAN_FILTER;
 
-	bond_dev->hw_features &= ~(NETIF_F_ALL_CSUM & ~NETIF_F_NO_CSUM);
+	bond_dev->hw_features &= ~(NETIF_F_ALL_CSUM & ~NETIF_F_HW_CSUM);
 	bond_dev->features |= bond_dev->hw_features;
 }
 
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index d0f8c7e..3f405b40 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -442,7 +442,7 @@ static void can_setup(struct net_device *dev)
 
 	/* New-style flags. */
 	dev->flags = IFF_NOARP;
-	dev->features = NETIF_F_NO_CSUM;
+	dev->features = NETIF_F_HW_CSUM;
 }
 
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 1b49df6..d8d9b3e 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -409,7 +409,7 @@ static void slc_setup(struct net_device *dev)
 
 	/* New-style flags. */
 	dev->flags		= IFF_NOARP;
-	dev->features           = NETIF_F_NO_CSUM;
+	dev->features           = NETIF_F_HW_CSUM;
 }
 
 /******************************************
diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index 39cf9b9..6b5c186 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -134,7 +134,7 @@ static void dummy_setup(struct net_device *dev)
 	dev->flags |= IFF_NOARP;
 	dev->flags &= ~IFF_MULTICAST;
 	dev->features	|= NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_TSO;
-	dev->features	|= NETIF_F_NO_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX;
+	dev->features	|= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX;
 	random_ether_addr(dev->dev_addr);
 }
 
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 6e82dd3..a3926a4 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -164,7 +164,7 @@ static const struct net_device_ops ifb_netdev_ops = {
 	.ndo_validate_addr = eth_validate_addr,
 };
 
-#define IFB_FEATURES (NETIF_F_NO_CSUM | NETIF_F_SG  | NETIF_F_FRAGLIST	| \
+#define IFB_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG  | NETIF_F_FRAGLIST	| \
 		      NETIF_F_TSO_ECN | NETIF_F_TSO | NETIF_F_TSO6	| \
 		      NETIF_F_HIGHDMA | NETIF_F_HW_VLAN_TX)
 
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 4ce9e5f..b71998d 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -169,7 +169,7 @@ static void loopback_setup(struct net_device *dev)
 	dev->features 		= NETIF_F_SG | NETIF_F_FRAGLIST
 		| NETIF_F_ALL_TSO
 		| NETIF_F_UFO
-		| NETIF_F_NO_CSUM
+		| NETIF_F_HW_CSUM
 		| NETIF_F_RXCSUM
 		| NETIF_F_HIGHDMA
 		| NETIF_F_LLTX
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 7f78db7..9d487d9 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -268,7 +268,7 @@ static void veth_setup(struct net_device *dev)
 	dev->features |= NETIF_F_LLTX;
 	dev->destructor = veth_dev_free;
 
-	dev->hw_features = NETIF_F_NO_CSUM | NETIF_F_SG | NETIF_F_RXCSUM;
+	dev->hw_features = NETIF_F_HW_CSUM | NETIF_F_SG | NETIF_F_RXCSUM;
 }
 
 /*
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index faa415d..ebace1b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1027,7 +1027,6 @@ struct net_device {
 
 #define NETIF_F_SG		1	/* Scatter/gather IO. */
 #define NETIF_F_IP_CSUM		2	/* Can checksum TCP/UDP over IPv4. */
-#define NETIF_F_NO_CSUM		4	/* Does not require checksum. F.e. loopack. */
 #define NETIF_F_HW_CSUM		8	/* Can checksum all the packets. */
 #define NETIF_F_IPV6_CSUM	16	/* Can checksum TCP/UDP over IPV6 */
 #define NETIF_F_HIGHDMA		32	/* Can DMA to high memory. */
@@ -1074,7 +1073,7 @@ struct net_device {
 				 NETIF_F_TSO6 | NETIF_F_UFO)
 
 
-#define NETIF_F_GEN_CSUM	(NETIF_F_NO_CSUM | NETIF_F_HW_CSUM)
+#define NETIF_F_GEN_CSUM	(NETIF_F_HW_CSUM)
 #define NETIF_F_V4_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
 #define NETIF_F_V6_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
 #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a24218c..ec107ac 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -81,7 +81,6 @@
  *	at device setup time.
  *	NETIF_F_HW_CSUM	- it is clever device, it is able to checksum
  *			  everything.
- *	NETIF_F_NO_CSUM - loopback or reliable single hop media.
  *	NETIF_F_IP_CSUM - device is dumb. It is able to csum only
  *			  TCP/UDP over IPv4. Sigh. Vendors like this
  *			  way by an unknown reason. Though, see comment above
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 32b8f9f..9d607ba 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -344,10 +344,10 @@ void br_dev_setup(struct net_device *dev)
 	dev->priv_flags = IFF_EBRIDGE;
 
 	dev->features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
-			NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_LLTX |
+			NETIF_F_GSO_MASK | NETIF_F_HW_CSUM | NETIF_F_LLTX |
 			NETIF_F_NETNS_LOCAL | NETIF_F_HW_VLAN_TX;
 	dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
-			   NETIF_F_GSO_MASK | NETIF_F_NO_CSUM |
+			   NETIF_F_GSO_MASK | NETIF_F_HW_CSUM |
 			   NETIF_F_HW_VLAN_TX;
 
 	br->dev = dev;
diff --git a/net/core/dev.c b/net/core/dev.c
index 7b9e2f4..09ce52c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5218,12 +5218,6 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
 		features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
 	}
 
-	if ((features & NETIF_F_NO_CSUM) &&
-	    (features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-		netdev_warn(dev, "mixed no checksumming and other settings.\n");
-		features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
-	}
-
 	/* Fix illegal SG+CSUM combinations. */
 	if ((features & NETIF_F_SG) &&
 	    !(features & NETIF_F_ALL_CSUM)) {
@@ -6215,17 +6209,13 @@ static int dev_cpu_callback(struct notifier_block *nfb,
  */
 u32 netdev_increment_features(u32 all, u32 one, u32 mask)
 {
-	if (mask & NETIF_F_GEN_CSUM)
+	if (mask & NETIF_F_HW_CSUM)
 		mask |= NETIF_F_ALL_CSUM;
 	mask |= NETIF_F_VLAN_CHALLENGED;
 
 	all |= one & (NETIF_F_ONE_FOR_ALL|NETIF_F_ALL_CSUM) & mask;
 	all &= one | ~NETIF_F_ALL_FOR_ALL;
 
-	/* If device needs checksumming, downgrade to it. */
-	if (all & (NETIF_F_ALL_CSUM & ~NETIF_F_NO_CSUM))
-		all &= ~NETIF_F_NO_CSUM;
-
 	/* If one device supports hw checksumming, set for all. */
 	if (all & NETIF_F_GEN_CSUM)
 		all &= ~(NETIF_F_ALL_CSUM & ~NETIF_F_GEN_CSUM);
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b7c12a6..3b5cee6 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -341,7 +341,7 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
 static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GSTRING_LEN] = {
 	/* NETIF_F_SG */              "tx-scatter-gather",
 	/* NETIF_F_IP_CSUM */         "tx-checksum-ipv4",
-	/* NETIF_F_NO_CSUM */         "tx-checksum-unneeded",
+	"",
 	/* NETIF_F_HW_CSUM */         "tx-checksum-ip-generic",
 	/* NETIF_F_IPV6_CSUM */       "tx-checksum-ipv6",
 	/* NETIF_F_HIGHDMA */         "highdma",
-- 
1.7.5.4

Re: [PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[Ben Hutchings]

On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote:
> There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM.
[...]

I think this is still a useful distinction, even the networking core
currently doesn't care about the difference.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[Stephen Hemminger]

On Thu, 14 Jul 2011 01:23:29 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote:
> > There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM.
> [...]
> 
> I think this is still a useful distinction, even the networking core
> currently doesn't care about the difference.
> 
> Ben.

It also is a kernel API change since this part is exposed
through ethtool calls.

Re: [PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[Ben Hutchings]

On Wed, 2011-07-13 at 17:48 -0700, Stephen Hemminger wrote:
> On Thu, 14 Jul 2011 01:23:29 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote:
> > > There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM.
> > [...]
> > 
> > I think this is still a useful distinction, even the networking core
> > currently doesn't care about the difference.
> > 
> > Ben.
> 
> It also is a kernel API change since this part is exposed
> through ethtool calls.

Users of ETHTOOL_{G,S}FEATURES are supposed to use the corresponding
string set to map names to bits (and vice versa).  It is OK to renumber
features, and to remove them if they are no longer implemented.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[Michał Mirosław]

On Wed, Jul 13, 2011 at 05:30:37PM -0700, Tom Herbert wrote:
> On Wed, Jul 13, 2011 at 5:23 PM, Ben Hutchings <bhutchings@solarflare.com>wrote:
> > On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote:
> > > There are no explicit users, so this is now equivalent to
> > NETIF_F_HW_CSUM.
> > [...]
> > I think this is still a useful distinction, even the networking core
> > currently doesn't care about the difference.
> Agreed.  It seems like this is the only way to distinguish virtual devices
> from HW devices (like we did with nocachecopy check).

You can't reliably detect virtual devices by this method. No tunnel devices
use this flag and it also doesn't detect e.g. IPsec being used on the route
(and no-cache copy should be disabled at least for software encryption).

That's why its turned off by default and should be enabled only when user
knows he will win some pps with it.

Best Regards,
Michał Mirosław

Re: [PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 22:56:23 +0200

> That's why its turned off by default and should be enabled only when user
> knows he will win some pps with it.

More people are going to lose than win by your change.

The nocopy feature helps more real situations than it hurts, the
existing default is the best.

Re: [PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[Michał Mirosław]

On Thu, Jul 14, 2011 at 02:31:21PM -0700, David Miller wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Date: Thu, 14 Jul 2011 22:56:23 +0200
> > That's why its turned off by default and should be enabled only when user
> > knows he will win some pps with it.
> More people are going to lose than win by your change.
> 
> The nocopy feature helps more real situations than it hurts, the
> existing default is the best.

I see. I still want to remove NO_CSUM (as I explained in other mail),
so would you accept replacing it with something more specific to
nocache-copy feature? READS_DATA maybe? That could be later added to
sk_route_caps whenever it's known for a route there will be need to
read packets' data.

Best Regards,
Michał Mirosław

Re: [PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Fri, 15 Jul 2011 00:44:45 +0200

> On Thu, Jul 14, 2011 at 02:31:21PM -0700, David Miller wrote:
>> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>> Date: Thu, 14 Jul 2011 22:56:23 +0200
>> > That's why its turned off by default and should be enabled only when user
>> > knows he will win some pps with it.
>> More people are going to lose than win by your change.
>> 
>> The nocopy feature helps more real situations than it hurts, the
>> existing default is the best.
> 
> I see. I still want to remove NO_CSUM (as I explained in other mail),
> so would you accept replacing it with something more specific to
> nocache-copy feature? READS_DATA maybe? That could be later added to
> sk_route_caps whenever it's known for a route there will be need to
> read packets' data.

I don't actually see what the problem is.

The code wants to conditionalize the nocache-copy feature based upon
whether hardware will checksum the packet or not.

And that's exactly what it's testing.

The reason, of course, is because it doesn't want to enable
nocache-copy if the cpu is just going to read the data back into it's
caches during the checksum.  But that's no reason to change the
flag name to have the word "read" instead of "checksum" in it.

Re: [PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[Michał Mirosław]

On Thu, Jul 14, 2011 at 05:05:06PM -0700, David Miller wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Date: Fri, 15 Jul 2011 00:44:45 +0200
> 
> > On Thu, Jul 14, 2011 at 02:31:21PM -0700, David Miller wrote:
> >> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >> Date: Thu, 14 Jul 2011 22:56:23 +0200
> >> > That's why its turned off by default and should be enabled only when user
> >> > knows he will win some pps with it.
> >> More people are going to lose than win by your change.
> >> 
> >> The nocopy feature helps more real situations than it hurts, the
> >> existing default is the best.
> > 
> > I see. I still want to remove NO_CSUM (as I explained in other mail),
> > so would you accept replacing it with something more specific to
> > nocache-copy feature? READS_DATA maybe? That could be later added to
> > sk_route_caps whenever it's known for a route there will be need to
> > read packets' data.
> 
> I don't actually see what the problem is.
> 
> The code wants to conditionalize the nocache-copy feature based upon
> whether hardware will checksum the packet or not.
> 
> And that's exactly what it's testing.
> 
> The reason, of course, is because it doesn't want to enable
> nocache-copy if the cpu is just going to read the data back into it's
> caches during the checksum.  But that's no reason to change the
> flag name to have the word "read" instead of "checksum" in it.

But the real condition is that CPU doesn't read the data. I doesn't matter
if reading is to calculate the checksum or parsing it. The change would
make this obvious.

BTW, 
Best Regards,
Michał Mirosław

Re: [PATCH 07/10] net: remove NETIF_F_NO_CSUM feature

[Michał Mirosław]

On Thu, Jul 14, 2011 at 01:23:29AM +0100, Ben Hutchings wrote:
> On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote:
> > There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM.
> [...]
> I think this is still a useful distinction, even the networking core
> currently doesn't care about the difference.

The problem is that all packets now can be redirected from NO_CSUM device
to other (or userspace). If some protocols just ignore checksum marking
altogether (like SCTP was doing) because of this flag, you get broken
packets that are hard to debug. It costs little to stay on the safe side
and calculate this additional u32 with checksum information, even if it
stays unused most of the time.

That's why I propose to remove NO_CSUM flag - to prevent someone to write
such a quietly broken code in the future.

Best Regards,
Michał Mirosław

[PATCH 04/10] net: cleanup vlan_features setting in register_netdev

[Michał Mirosław]

vlan_features contains features inherited from underlying device.
NETIF_SOFT_FEATURES are not inherited but belong to the vlan device
itself (ensured in vlan_dev_fix_features()).

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/core/dev.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 9ca1514..e57be02 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5488,12 +5488,9 @@ int register_netdevice(struct net_device *dev)
 		dev->features |= NETIF_F_NOCACHE_COPY;
 	}
 
-	/* Enable GSO, GRO and NETIF_F_HIGHDMA for vlans by default,
-	 * vlan_dev_fix_features() will do the features check,
-	 * so NETIF_F_HIGHDMA feature is enabled only if supported
-	 * by underlying device.
+	/* Make NETIF_F_HIGHDMA inheritable to VLAN devices.
 	 */
-	dev->vlan_features |= (NETIF_F_SOFT_FEATURES | NETIF_F_HIGHDMA);
+	dev->vlan_features |= NETIF_F_HIGHDMA;
 
 	ret = call_netdevice_notifiers(NETDEV_POST_INIT, dev);
 	ret = notifier_to_errno(ret);
-- 
1.7.5.4

Re: [PATCH 04/10] net: cleanup vlan_features setting in register_netdev

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 02:10:29 +0200 (CEST)

> vlan_features contains features inherited from underlying device.
> NETIF_SOFT_FEATURES are not inherited but belong to the vlan device
> itself (ensured in vlan_dev_fix_features()).
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.

[PATCH 05/10] net: Disable NOCACHE_COPY by default

[Michał Mirosław]

Disable NOCACHE_COPY by default as there's no reliable way to tell
in advance what device(s) will take the packet. Also, allow the
setting to be inherited by VLAN devices.

This is the last use of NETIF_F_NO_CSUM.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/core/dev.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index e57be02..7b9e2f4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5480,13 +5480,12 @@ int register_netdevice(struct net_device *dev)
 	dev->features |= NETIF_F_SOFT_FEATURES;
 	dev->wanted_features = dev->features & dev->hw_features;
 
-	/* Turn on no cache copy if HW is doing checksum */
+	/* Allow changing no-cache copy but disable it by default
+	 * as there's no reliable way to tell what device(s) will take
+	 * the packet. Allow the setting to be inherited to VLANs.
+	 */
 	dev->hw_features |= NETIF_F_NOCACHE_COPY;
-	if ((dev->features & NETIF_F_ALL_CSUM) &&
-	    !(dev->features & NETIF_F_NO_CSUM)) {
-		dev->wanted_features |= NETIF_F_NOCACHE_COPY;
-		dev->features |= NETIF_F_NOCACHE_COPY;
-	}
+	dev->vlan_features |= NETIF_F_NOCACHE_COPY;
 
 	/* Make NETIF_F_HIGHDMA inheritable to VLAN devices.
 	 */
-- 
1.7.5.4

Re: [PATCH 05/10] net: Disable NOCACHE_COPY by default

[Tom Herbert]

On Wed, Jul 13, 2011 at 5:10 PM, Michał Mirosław
<mirq-linux@rere.qmqm.pl> wrote:
>
> Disable NOCACHE_COPY by default as there's no reliable way to tell
> in advance what device(s) will take the packet. Also, allow the
> setting to be inherited by VLAN devices.
>

What problem is this addressing?  Is this just to get rid of the
NETIF_NO_CSUM bit, or is there an issue with nocachecopy being enabled
by default?

Tom

> This is the last use of NETIF_F_NO_CSUM.
>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  net/core/dev.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e57be02..7b9e2f4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5480,13 +5480,12 @@ int register_netdevice(struct net_device *dev)
>        dev->features |= NETIF_F_SOFT_FEATURES;
>        dev->wanted_features = dev->features & dev->hw_features;
>
> -       /* Turn on no cache copy if HW is doing checksum */
> +       /* Allow changing no-cache copy but disable it by default
> +        * as there's no reliable way to tell what device(s) will take
> +        * the packet. Allow the setting to be inherited to VLANs.
> +        */
>        dev->hw_features |= NETIF_F_NOCACHE_COPY;
> -       if ((dev->features & NETIF_F_ALL_CSUM) &&
> -           !(dev->features & NETIF_F_NO_CSUM)) {
> -               dev->wanted_features |= NETIF_F_NOCACHE_COPY;
> -               dev->features |= NETIF_F_NOCACHE_COPY;
> -       }
> +       dev->vlan_features |= NETIF_F_NOCACHE_COPY;
>
>        /* Make NETIF_F_HIGHDMA inheritable to VLAN devices.
>         */
> --
> 1.7.5.4
>

[PATCH 09/10] net: remove /sys/class/net/*/features

[Michał Mirosław]

The same information and more can be obtained by using ethtool
with ETHTOOL_GFEATURES.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/core/net-sysfs.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 33d2a1f..1683e5d 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -100,7 +100,6 @@ NETDEVICE_SHOW(addr_assign_type, fmt_dec);
 NETDEVICE_SHOW(addr_len, fmt_dec);
 NETDEVICE_SHOW(iflink, fmt_dec);
 NETDEVICE_SHOW(ifindex, fmt_dec);
-NETDEVICE_SHOW(features, fmt_hex);
 NETDEVICE_SHOW(type, fmt_dec);
 NETDEVICE_SHOW(link_mode, fmt_dec);
 
@@ -312,7 +311,6 @@ static struct device_attribute net_class_attributes[] = {
 	__ATTR(ifalias, S_IRUGO | S_IWUSR, show_ifalias, store_ifalias),
 	__ATTR(iflink, S_IRUGO, show_iflink, NULL),
 	__ATTR(ifindex, S_IRUGO, show_ifindex, NULL),
-	__ATTR(features, S_IRUGO, show_features, NULL),
 	__ATTR(type, S_IRUGO, show_type, NULL),
 	__ATTR(link_mode, S_IRUGO, show_link_mode, NULL),
 	__ATTR(address, S_IRUGO, show_address, NULL),
-- 
1.7.5.4

Re: [PATCH 09/10] net: remove /sys/class/net/*/features

[Stephen Hemminger]

On Thu, 14 Jul 2011 02:10:30 +0200 (CEST)
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:

> The same information and more can be obtained by using ethtool
> with ETHTOOL_GFEATURES.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Changing user visible API has to go through a longer process
such as putting it in feature-removal-schedule.txt for a while
(at couple of releases) before removing.

Re: [PATCH 09/10] net: remove /sys/class/net/*/features

[Ben Hutchings]

On Wed, 2011-07-13 at 17:50 -0700, Stephen Hemminger wrote:
> On Thu, 14 Jul 2011 02:10:30 +0200 (CEST)
> Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> 
> > The same information and more can be obtained by using ethtool
> > with ETHTOOL_GFEATURES.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> Changing user visible API has to go through a longer process
> such as putting it in feature-removal-schedule.txt for a while
> (at couple of releases) before removing.

We already established that this ABI has been changed repeatedly by
renumbering of features.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 09/10] net: remove /sys/class/net/*/features

[Stephen Hemminger]

On Thu, 14 Jul 2011 02:03:59 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Wed, 2011-07-13 at 17:50 -0700, Stephen Hemminger wrote:
> > On Thu, 14 Jul 2011 02:10:30 +0200 (CEST)
> > Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> > 
> > > The same information and more can be obtained by using ethtool
> > > with ETHTOOL_GFEATURES.
> > > 
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > 
> > Changing user visible API has to go through a longer process
> > such as putting it in feature-removal-schedule.txt for a while
> > (at couple of releases) before removing.
> 
> We already established that this ABI has been changed repeatedly by
> renumbering of features.

Ok, but we still should have maintained compatibly with mapping.

Re: [PATCH 09/10] net: remove /sys/class/net/*/features

[Michał Mirosław]

On Wed, Jul 13, 2011 at 05:50:37PM -0700, Stephen Hemminger wrote:
> On Thu, 14 Jul 2011 02:10:30 +0200 (CEST)
> Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> 
> > The same information and more can be obtained by using ethtool
> > with ETHTOOL_GFEATURES.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Changing user visible API has to go through a longer process
> such as putting it in feature-removal-schedule.txt for a while
> (at couple of releases) before removing.

Well, it's broken and prevents improving then features infrastructure.
Besides, it was never documented and meaing of bits it returns is
already kernel version dependent.

Best Regards,
Michał Mirosław

Re: [PATCH 09/10] net: remove /sys/class/net/*/features

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 02:10:30 +0200 (CEST)

> The same information and more can be obtained by using ethtool
> with ETHTOOL_GFEATURES.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.

[PATCH 10/10] net: remove SK_ROUTE_CAPS from meta ematch

[Michał Mirosław]

Remove it, as it indirectly exposes netdev features. It's not used in
iproute2 (2.6.38) - is anything else using its interface?

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 include/linux/tc_ematch/tc_em_meta.h |    2 +-
 net/sched/em_meta.c                  |    7 -------
 2 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/include/linux/tc_ematch/tc_em_meta.h b/include/linux/tc_ematch/tc_em_meta.h
index 7138962..b11f8ce 100644
--- a/include/linux/tc_ematch/tc_em_meta.h
+++ b/include/linux/tc_ematch/tc_em_meta.h
@@ -67,7 +67,7 @@ enum {
 	TCF_META_ID_SK_FORWARD_ALLOCS,
 	TCF_META_ID_SK_SNDBUF,
  	TCF_META_ID_SK_ALLOCS,
- 	TCF_META_ID_SK_ROUTE_CAPS,
+	__TCF_META_ID_SK_ROUTE_CAPS,	/* unimplemented but in ABI already */
  	TCF_META_ID_SK_HASH,
  	TCF_META_ID_SK_LINGERTIME,
  	TCF_META_ID_SK_ACK_BACKLOG,
diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c
index 49130e8..1363bf1 100644
--- a/net/sched/em_meta.c
+++ b/net/sched/em_meta.c
@@ -404,12 +404,6 @@ META_COLLECTOR(int_sk_alloc)
 	dst->value = (__force int) skb->sk->sk_allocation;
 }
 
-META_COLLECTOR(int_sk_route_caps)
-{
-	SKIP_NONLOCAL(skb);
-	dst->value = skb->sk->sk_route_caps;
-}
-
 META_COLLECTOR(int_sk_hash)
 {
 	SKIP_NONLOCAL(skb);
@@ -530,7 +524,6 @@ static struct meta_ops __meta_ops[TCF_META_TYPE_MAX + 1][TCF_META_ID_MAX + 1] =
 		[META_ID(SK_ERR_QLEN)]		= META_FUNC(int_sk_err_qlen),
 		[META_ID(SK_FORWARD_ALLOCS)]	= META_FUNC(int_sk_fwd_alloc),
 		[META_ID(SK_ALLOCS)]		= META_FUNC(int_sk_alloc),
-		[META_ID(SK_ROUTE_CAPS)]	= META_FUNC(int_sk_route_caps),
 		[META_ID(SK_HASH)]		= META_FUNC(int_sk_hash),
 		[META_ID(SK_LINGERTIME)]	= META_FUNC(int_sk_lingertime),
 		[META_ID(SK_ACK_BACKLOG)]	= META_FUNC(int_sk_ack_bl),
-- 
1.7.5.4

Re: [PATCH 10/10] net: remove SK_ROUTE_CAPS from meta ematch

[Stephen Hemminger]

On Thu, 14 Jul 2011 02:10:30 +0200 (CEST)
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:

> Remove it, as it indirectly exposes netdev features. It's not used in
> iproute2 (2.6.38) - is anything else using its interface?
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Once again, users need to be allowed to do arbitrary combination
of new and old releases of kernel and userspace.

Re: [PATCH 10/10] net: remove SK_ROUTE_CAPS from meta ematch

[jamal]

On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote:
> Remove it, as it indirectly exposes netdev features. It's not used in
> iproute2 (2.6.38) - is anything else using its interface?
> 

Breaks user ABI.

cheers,
jamal

Re: [PATCH 10/10] net: remove SK_ROUTE_CAPS from meta ematch

[Michał Mirosław]

On Wed, Jul 13, 2011 at 08:59:26PM -0400, jamal wrote:
> On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote:
> > Remove it, as it indirectly exposes netdev features. It's not used in
> > iproute2 (2.6.38) - is anything else using its interface?
> Breaks user ABI.

No iproute2 version uses it, its meaning have already changed several
times in kernel history and it prevents improving the netdev features
infrastructure. Note that other IDs are not changed - just this one is
turned into unsupported.

Best Regards,
Michał Mirosław

Re: [PATCH 10/10] net: remove SK_ROUTE_CAPS from meta ematch

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 22:50:05 +0200

> On Wed, Jul 13, 2011 at 08:59:26PM -0400, jamal wrote:
>> On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote:
>> > Remove it, as it indirectly exposes netdev features. It's not used in
>> > iproute2 (2.6.38) - is anything else using its interface?
>> Breaks user ABI.
> 
> No iproute2 version uses it, its meaning have already changed several
> times in kernel history and it prevents improving the netdev features
> infrastructure. Note that other IDs are not changed - just this one is
> turned into unsupported.

I totally agree.

We have to disconnect the layout and specific bit patterns in
netdev features from any userspace dependency.

Re: [PATCH 10/10] net: remove SK_ROUTE_CAPS from meta ematch

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 02:10:30 +0200 (CEST)

> Remove it, as it indirectly exposes netdev features. It's not used in
> iproute2 (2.6.38) - is anything else using its interface?
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.

[PATCH 08/10] net: unexport netdev_fix_features()

[Michał Mirosław]

It is not used anywhere except net/core/dev.c now.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 include/linux/netdevice.h |    1 -
 net/core/dev.c            |    3 +--
 2 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ebace1b..c91c643 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2535,7 +2535,6 @@ static inline u32 netdev_get_wanted_features(struct net_device *dev)
 	return (dev->features & ~dev->hw_features) | dev->wanted_features;
 }
 u32 netdev_increment_features(u32 all, u32 one, u32 mask);
-u32 netdev_fix_features(struct net_device *dev, u32 features);
 int __netdev_update_features(struct net_device *dev);
 void netdev_update_features(struct net_device *dev);
 void netdev_change_features(struct net_device *dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index 09ce52c..a2f58ea 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5209,7 +5209,7 @@ static void rollback_registered(struct net_device *dev)
 	list_del(&single);
 }
 
-u32 netdev_fix_features(struct net_device *dev, u32 features)
+static u32 netdev_fix_features(struct net_device *dev, u32 features)
 {
 	/* Fix illegal checksum combinations */
 	if ((features & NETIF_F_HW_CSUM) &&
@@ -5262,7 +5262,6 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
 
 	return features;
 }
-EXPORT_SYMBOL(netdev_fix_features);
 
 int __netdev_update_features(struct net_device *dev)
 {
-- 
1.7.5.4

Re: [PATCH 08/10] net: unexport netdev_fix_features()

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu, 14 Jul 2011 02:10:30 +0200 (CEST)

> It is not used anywhere except net/core/dev.c now.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.