On Wed, 20 Oct 2004 20:52:55 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> This is bogus. The subsequent if clause is what allows the size to
> exceed mss_cache_std.
>
> TSO without NETIF_F_SG is not worth it.
Indeed. I'm going to enforce this at device registration time
as in the patch at the end of this email.
BTW, we allow mucking of all of these SG, TSO, CSUM settings
via ethtool yet the "X needs Y" rules are not enforced. I
can't think of an easy way to do this without touching a lot
of drivers. Perhaps something like:
int ethtool_feature_change(struct net_device *dev, u32 flag_bit, int on_off);
So drivers/net/tg3.c:tg3_set_tx() might then look like this:
static int tg3_set_tx_csum(struct net_device *dev, u32 data)
{
struct tg3 *tp = netdev_priv(dev);
int err;
if (tp->tg3_flags & TG3_FLAG_BROKEN_CHECKSUMS) {
if (data != 0)
return -EINVAL;
return 0;
}
else
dev->features &= ~NETIF_F_IP_CSUM;
return ethtool_feature_change(dev, NETIF_F_IP_CSUM, data);
}
And ethtool_feature_change() might be implemented like so:
int ethtool_feature_change(struct net_device *dev, u32 flag_bit, int on_off)
{
/* If checksumming is being disabled, both SG and
* TSO must be disabled as well.
*/
if (!on_off &&
(flag_bit & (NETIF_F_IP_CSUM |
NETIF_F_HW_CSUM |
NETIF_F_NO_CSUM))) {
dev->features &= ~(NETIF_F_SG | NETIF_F_TSO);
}
/* If SG is being disabled, TSO must be disabled
* as well.
*/
if (!on_off && (flag_bit & NETIF_F_SG))
dev->features &= ~NETIF_F_TSO;
/* TSO requires SG */
if (on_off &&
(flag_bit & NETIF_F_TSO) &&
!(dev->features & NETIF_F_SG))
return -EINVAL;
/* SG requires CSUM */
if (on_off &&
(flag_bit & NETIF_F_SG) &&
!(dev->features & (NETIF_F_IP_CSUM |
NETIF_F_HW_CSUM |
NETIF_F_NO_CSUM)))
return -EINVAL;
if (on_off)
dev->features |= flag_bit;
else
dev->features &= ~flag_bit;
return 0;
}
And then we add usage of this thing to the various drivers
and the generic implementation of the appropriate ethtool
ops in net/core/ethtool.c
It just occured to me that instead of manually clearing
dev->feature bits we should invoke the appropriate ethtool
op to accomplish that just in case the device needs to do
something implementation specific when disabling said features.
This implies that ethtool_feature_change() should be invoked
without any device locks set to prevent deadlocks.
Comments?
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2004/10/20 16:16:33-07:00 davem@nuts.davemloft.net
# [NET]: TSO requires SG, enforce this at device registry.
#
# Signed-off-by: David S. Miller <davem@davemloft.net>
#
# net/core/dev.c
# 2004/10/20 16:16:03-07:00 davem@nuts.davemloft.net +8 -0
# [NET]: TSO requires SG, enforce this at device registry.
#
diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c 2004-10-20 16:16:47 -07:00
+++ b/net/core/dev.c 2004-10-20 16:16:47 -07:00
@@ -2871,6 +2871,14 @@
dev->features &= ~NETIF_F_SG;
}
+ /* TSO requires that SG is present as well. */
+ if ((dev->features & NETIF_F_TSO) &&
+ !(dev->features & NETIF_F_SG)) {
+ printk("%s: Dropping NETIF_F_TSO since no SG feature.\n",
+ dev->name);
+ dev->features &= ~NETIF_F_TSO;
+ }
+
/*
* nil rebuild_header routine,
* that should be never called and used as just bug trap.
-
: send the line "unsubscribe linux-net" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
