| Message ID | 1308285007-11302-2-git-send-email-greearb@candelatech.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
2011/6/17 <greearb@candelatech.com>: > From: Ben Greear <greearb@candelatech.com> > > This will allow us to enable/disable having the Ethernet > frame checksum appended to the skb. Enabling this is > useful when sniffing packets. > > In particular, this can be used to test logic that allows > a NIC to receive all frames, even ones with bad checksums. You want to silently run away from extending dev->features field? ;-) Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-06-16 at 21:30 -0700, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > This will allow us to enable/disable having the Ethernet > frame checksum appended to the skb. Enabling this is > useful when sniffing packets. > > In particular, this can be used to test logic that allows > a NIC to receive all frames, even ones with bad checksums. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > :100644 100644 439b173... e5e8747... M include/linux/ethtool.h > :100644 100644 fd14116... b36bac7... M net/core/ethtool.c > include/linux/ethtool.h | 5 +++++ > net/core/ethtool.c | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 43 insertions(+), 0 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 439b173..e5e8747 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -955,6 +955,8 @@ struct ethtool_ops { > int (*get_dump_data)(struct net_device *, > struct ethtool_dump *, void *); > int (*set_dump)(struct net_device *, struct ethtool_dump *); > + int (*set_save_fcs)(struct net_device *, u32); > + int (*get_save_fcs)(struct net_device *, u32 *); These need to be described in the kernel-doc. Why do these function names use 'save_fcs' whereas the command names use 'RXFCS'? > }; > #endif /* __KERNEL__ */ > @@ -1029,6 +1031,9 @@ struct ethtool_ops { > #define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */ > #define ETHTOOL_GET_DUMP_FLAG 0x0000003f /* Get dump settings */ > #define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */ > +#define ETHTOOL_GETRXFCS 0x00000041 /* Get RX Save Frame Checksum */ > +#define ETHTOOL_SETRXFCS 0x00000042 /* Set RX Save Frame Checksum */ > + > > /* compatibility with older code */ > #define SPARC_ETH_GSET ETHTOOL_GSET > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index fd14116..b36bac7 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -1927,6 +1927,38 @@ out: > return ret; > } > > +static int ethtool_get_rx_fcs(struct net_device *dev, void __user *useraddr) > +{ > + struct ethtool_value edata; > + int rv = 0; > + > + if (!dev->ethtool_ops->get_save_fcs) > + return -EOPNOTSUPP; > + > + rv = dev->ethtool_ops->get_save_fcs(dev, &edata.data); > + if (rv < 0) > + return rv; > + > + if (copy_to_user(useraddr, &edata, sizeof(edata))) > + return -EFAULT; > + return 0; > +} > + > + > +static int ethtool_set_rx_fcs(struct net_device *dev, void __user *useraddr) > +{ > + struct ethtool_value id; > + > + if (!dev->ethtool_ops->set_save_fcs) > + return -EOPNOTSUPP; > + > + if (copy_from_user(&id, useraddr, sizeof(id))) > + return -EFAULT; > + > + return dev->ethtool_ops->set_save_fcs(dev, id.data); > +} > + > + > /* The main entry point in this file. Called from net/core/dev.c */ > > int dev_ethtool(struct net *net, struct ifreq *ifr) > @@ -2152,6 +2184,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) > case ETHTOOL_GET_DUMP_DATA: > rc = ethtool_get_dump_data(dev, useraddr); > break; > + case ETHTOOL_SETRXFCS: > + rc = ethtool_set_rx_fcs(dev, useraddr); > + break; > + case ETHTOOL_GETRXFCS: > + rc = ethtool_get_rx_fcs(dev, useraddr); > + break; You can use ethtool_{get,set}_value() rather than adding new trivial functions. And as Michal says, this could reasonably be a feature not an entirely separate flag. I'm not sure it's that important to have debugging flags in features, but I also don't want to have 2 commands per flag... Ben. > default: > rc = -EOPNOTSUPP; > }
On 06/17/2011 05:20 AM, Ben Hutchings wrote: > On Thu, 2011-06-16 at 21:30 -0700, greearb@candelatech.com wrote: >> From: Ben Greear<greearb@candelatech.com> >> >> This will allow us to enable/disable having the Ethernet >> frame checksum appended to the skb. Enabling this is >> useful when sniffing packets. >> >> In particular, this can be used to test logic that allows >> a NIC to receive all frames, even ones with bad checksums. >> >> Signed-off-by: Ben Greear<greearb@candelatech.com> >> --- >> :100644 100644 439b173... e5e8747... M include/linux/ethtool.h >> :100644 100644 fd14116... b36bac7... M net/core/ethtool.c >> include/linux/ethtool.h | 5 +++++ >> net/core/ethtool.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 43 insertions(+), 0 deletions(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index 439b173..e5e8747 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -955,6 +955,8 @@ struct ethtool_ops { >> int (*get_dump_data)(struct net_device *, >> struct ethtool_dump *, void *); >> int (*set_dump)(struct net_device *, struct ethtool_dump *); >> + int (*set_save_fcs)(struct net_device *, u32); >> + int (*get_save_fcs)(struct net_device *, u32 *); > > These need to be described in the kernel-doc. > > Why do these function names use 'save_fcs' whereas the command names use > 'RXFCS'? > >> }; >> #endif /* __KERNEL__ */ >> @@ -1029,6 +1031,9 @@ struct ethtool_ops { >> #define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */ >> #define ETHTOOL_GET_DUMP_FLAG 0x0000003f /* Get dump settings */ >> #define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */ >> +#define ETHTOOL_GETRXFCS 0x00000041 /* Get RX Save Frame Checksum */ >> +#define ETHTOOL_SETRXFCS 0x00000042 /* Set RX Save Frame Checksum */ >> + >> >> /* compatibility with older code */ >> #define SPARC_ETH_GSET ETHTOOL_GSET >> diff --git a/net/core/ethtool.c b/net/core/ethtool.c >> index fd14116..b36bac7 100644 >> --- a/net/core/ethtool.c >> +++ b/net/core/ethtool.c >> @@ -1927,6 +1927,38 @@ out: >> return ret; >> } >> >> +static int ethtool_get_rx_fcs(struct net_device *dev, void __user *useraddr) >> +{ >> + struct ethtool_value edata; >> + int rv = 0; >> + >> + if (!dev->ethtool_ops->get_save_fcs) >> + return -EOPNOTSUPP; >> + >> + rv = dev->ethtool_ops->get_save_fcs(dev,&edata.data); >> + if (rv< 0) >> + return rv; >> + >> + if (copy_to_user(useraddr,&edata, sizeof(edata))) >> + return -EFAULT; >> + return 0; >> +} >> + >> + >> +static int ethtool_set_rx_fcs(struct net_device *dev, void __user *useraddr) >> +{ >> + struct ethtool_value id; >> + >> + if (!dev->ethtool_ops->set_save_fcs) >> + return -EOPNOTSUPP; >> + >> + if (copy_from_user(&id, useraddr, sizeof(id))) >> + return -EFAULT; >> + >> + return dev->ethtool_ops->set_save_fcs(dev, id.data); >> +} >> + >> + >> /* The main entry point in this file. Called from net/core/dev.c */ >> >> int dev_ethtool(struct net *net, struct ifreq *ifr) >> @@ -2152,6 +2184,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) >> case ETHTOOL_GET_DUMP_DATA: >> rc = ethtool_get_dump_data(dev, useraddr); >> break; >> + case ETHTOOL_SETRXFCS: >> + rc = ethtool_set_rx_fcs(dev, useraddr); >> + break; >> + case ETHTOOL_GETRXFCS: >> + rc = ethtool_get_rx_fcs(dev, useraddr); >> + break; > > You can use ethtool_{get,set}_value() rather than adding new trivial > functions. I'll look at that. > > And as Michal says, this could reasonably be a feature not an entirely > separate flag. I'm not sure it's that important to have debugging flags > in features, but I also don't want to have 2 commands per flag... I'm not sure it really counts as a feature, and I've no desire to tangle with the effort to extend features beyond 32 bits. Maybe we could have a new ethtool command that took a struct with two args, so we can "set flag-foo val" instead of 'enable-flag-foo' and 'disable-flag-foo'? That should be only one additional method for the drivers to implement, and can be used for the other patches I have planned as well. Thanks, Ben
On Fri, 2011-06-17 at 08:33 -0700, Ben Greear wrote: > On 06/17/2011 05:20 AM, Ben Hutchings wrote: > > On Thu, 2011-06-16 at 21:30 -0700, greearb@candelatech.com wrote: > >> From: Ben Greear<greearb@candelatech.com> > >> > >> This will allow us to enable/disable having the Ethernet > >> frame checksum appended to the skb. Enabling this is > >> useful when sniffing packets. > >> > >> In particular, this can be used to test logic that allows > >> a NIC to receive all frames, even ones with bad checksums. [...] > > And as Michal says, this could reasonably be a feature not an entirely > > separate flag. I'm not sure it's that important to have debugging flags > > in features, but I also don't want to have 2 commands per flag... > > I'm not sure it really counts as a feature, and I've no desire to tangle > with the effort to extend features beyond 32 bits. There are several people wanting to add new flags, so between you I'm sure you can manage it. > Maybe we could have a new > ethtool command that took a struct with two args, so we can "set flag-foo val" > instead of 'enable-flag-foo' and 'disable-flag-foo'? > > That should be only one additional method for the drivers to implement, and > can be used for the other patches I have planned as well. Whereas implementing an extra feature toggle now requires zero extra methods. Ben.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 439b173..e5e8747 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -955,6 +955,8 @@ struct ethtool_ops { int (*get_dump_data)(struct net_device *, struct ethtool_dump *, void *); int (*set_dump)(struct net_device *, struct ethtool_dump *); + int (*set_save_fcs)(struct net_device *, u32); + int (*get_save_fcs)(struct net_device *, u32 *); }; #endif /* __KERNEL__ */ @@ -1029,6 +1031,9 @@ struct ethtool_ops { #define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */ #define ETHTOOL_GET_DUMP_FLAG 0x0000003f /* Get dump settings */ #define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */ +#define ETHTOOL_GETRXFCS 0x00000041 /* Get RX Save Frame Checksum */ +#define ETHTOOL_SETRXFCS 0x00000042 /* Set RX Save Frame Checksum */ + /* compatibility with older code */ #define SPARC_ETH_GSET ETHTOOL_GSET diff --git a/net/core/ethtool.c b/net/core/ethtool.c index fd14116..b36bac7 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1927,6 +1927,38 @@ out: return ret; } +static int ethtool_get_rx_fcs(struct net_device *dev, void __user *useraddr) +{ + struct ethtool_value edata; + int rv = 0; + + if (!dev->ethtool_ops->get_save_fcs) + return -EOPNOTSUPP; + + rv = dev->ethtool_ops->get_save_fcs(dev, &edata.data); + if (rv < 0) + return rv; + + if (copy_to_user(useraddr, &edata, sizeof(edata))) + return -EFAULT; + return 0; +} + + +static int ethtool_set_rx_fcs(struct net_device *dev, void __user *useraddr) +{ + struct ethtool_value id; + + if (!dev->ethtool_ops->set_save_fcs) + return -EOPNOTSUPP; + + if (copy_from_user(&id, useraddr, sizeof(id))) + return -EFAULT; + + return dev->ethtool_ops->set_save_fcs(dev, id.data); +} + + /* The main entry point in this file. Called from net/core/dev.c */ int dev_ethtool(struct net *net, struct ifreq *ifr) @@ -2152,6 +2184,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) case ETHTOOL_GET_DUMP_DATA: rc = ethtool_get_dump_data(dev, useraddr); break; + case ETHTOOL_SETRXFCS: + rc = ethtool_set_rx_fcs(dev, useraddr); + break; + case ETHTOOL_GETRXFCS: + rc = ethtool_get_rx_fcs(dev, useraddr); + break; default: rc = -EOPNOTSUPP; }