ethtool.h should use userspace-accessible types
Subsystems (2)
PATCH 1/1 ethtool.h should use userspace-accessible types
Attached is a patch to 2.4's ethtool.h to use appropriate, userspace-accessible data types (__u8 and friends, rather than u8 and friends).
I've also posted a patch against ethtool-1.8 to the sf.net gkernel site that cleans-up ethtool-copy.h and the rest in the same manner.
My motivation for the patch is to allow a userspace program to cleanly call ethtool ioctls without the typedef'ing used within the ethtool package.
Please cc me with responses, as I am not subscribed to lkml.
Thanks,
E
===== include/linux/ethtool.h 1.18 vs edited =====
include/linux/ethtool.h +100 -100
@@ -15,24 +15,24 @@/* This should work for both 32 and 64 bit userland. */ struct ethtool_cmd { -u32 cmd; -u32 supported;/* Features this interface supports */ -u32 advertising;/* Features this interface advertises */ -u16 speed;/* The forced speed, 10Mb, 100Mb, gigabit */ -u8 duplex;/* Duplex, half or full */ -u8 port;/* Which connector port */ -u8 phy_address; -u8 transceiver;/* Which tranceiver to use */ -u8 autoneg;/* Enable or disable autonegotiation */ -u32 maxtxpkt;/* Tx pkts before generating tx int */ -u32 maxrxpkt;/* Rx pkts before generating rx int */ -u32 reserved[4 ]; +__u32 cmd; +__u32 supported;/* Features this interface supports */ +__u32 advertising;/* Features this interface advertises */ +__u16 speed;/* The forced speed, 10Mb, 100Mb, gigabit */ +__u8 duplex;/* Duplex, half or full */ +__u8 port;/* Which connector port */ +__u8 phy_address; +__u8 transceiver;/* Which tranceiver to use */ +__u8 autoneg;/* Enable or disable autonegotiation */ +__u32 maxtxpkt;/* Tx pkts before generating tx int */ +__u32 maxrxpkt;/* Rx pkts before generating rx int */ +__u32 reserved[4 ]; };#define ETHTOOL_BUSINFO_LEN 32/* these strings are set to whatever the driver author decides... */ struct ethtool_drvinfo { -u32 cmd; +__u32 cmd;char driver[32 ];/* driver short name, "tulip", "eepro100" */ char version[32 ];/* driver version string */ char fw_version[32 ];/* firmware version string, if applicable */
@@ -40,53 +40,53 @@/* For PCI devices, use pci_dev->slot_name. */ char reserved1[32 ];char reserved2[16 ]; -u32 n_stats;/* number of u64's from ETHTOOL_GSTATS */ -u32 testinfo_len; -u32 eedump_len;/* Size of data from ETHTOOL_GEEPROM (bytes) */ -u32 regdump_len;/* Size of data from ETHTOOL_GREGS (bytes) */ +__u32 n_stats;/* number of __u64's from ETHTOOL_GSTATS */ +__u32 testinfo_len; +__u32 eedump_len;/* Size of data from ETHTOOL_GEEPROM (bytes) */ +__u32 regdump_len;/* Size of data from ETHTOOL_GREGS (bytes) */ };#define SOPASS_MAX 6/* wake-on-lan settings */ struct ethtool_wolinfo { -u32 cmd; -u32 supported; -u32 wolopts; -u8 sopass[SOPASS_MAX ];/* SecureOn(tm) password */ +__u32 cmd; +__u32 supported; +__u32 wolopts; +__u8 sopass[SOPASS_MAX ];/* SecureOn(tm) password */ };/* for passing single values */ struct ethtool_value { -u32 cmd; -u32 data; +__u32 cmd; +__u32 data; };/* for passing big chunks of data */ struct ethtool_regs { -u32 cmd; -u32 version;/* driver-specific, indicates different chips/revs */ -u32 len;/* bytes */ -u8 data[0 ]; +__u32 cmd; +__u32 version;/* driver-specific, indicates different chips/revs */ +__u32 len;/* bytes */ +__u8 data[0 ]; };/* for passing EEPROM chunks */ struct ethtool_eeprom { -u32 cmd; -u32 magic; -u32 offset;/* in bytes */ -u32 len;/* in bytes */ -u8 data[0 ]; +__u32 cmd; +__u32 magic; +__u32 offset;/* in bytes */ +__u32 len;/* in bytes */ +__u8 data[0 ]; };/* for configuring coalescing parameters of chip */ struct ethtool_coalesce { -u32 cmd;/* ETHTOOL_{G,S}COALESCE */ +__u32 cmd;/* ETHTOOL_{G,S}COALESCE */ /* How many usecs to delay an RX interrupt after * a packet arrives. If 0, only rx_max_coalesced_frames * is used. */ -u32 rx_coalesce_usecs; +__u32 rx_coalesce_usecs;/* How many packets to delay an RX interrupt after * a packet arrives. If 0, only rx_coalesce_usecs is
@@ -94,21 +94,21 @@* to zero as this would cause RX interrupts to never be * generated. */ -u32 rx_max_coalesced_frames; +__u32 rx_max_coalesced_frames;/* Same as above two parameters, except that these values * apply while an IRQ is being serviced by the host. Not * all cards support this feature and the values are ignored * in that case. */ -u32 rx_coalesce_usecs_irq; -u32 rx_max_coalesced_frames_irq; +__u32 rx_coalesce_usecs_irq; +__u32 rx_max_coalesced_frames_irq;/* How many usecs to delay a TX interrupt after * a packet is sent. If 0, only tx_max_coalesced_frames * is used. */ -u32 tx_coalesce_usecs; +__u32 tx_coalesce_usecs;/* How many packets to delay a TX interrupt after * a packet is sent. If 0, only tx_coalesce_usecs is
@@ -116,22 +116,22 @@* to zero as this would cause TX interrupts to never be * generated. */ -u32 tx_max_coalesced_frames; +__u32 tx_max_coalesced_frames;/* Same as above two parameters, except that these values * apply while an IRQ is being serviced by the host. Not * all cards support this feature and the values are ignored * in that case. */ -u32 tx_coalesce_usecs_irq; -u32 tx_max_coalesced_frames_irq; +__u32 tx_coalesce_usecs_irq; +__u32 tx_max_coalesced_frames_irq;/* How many usecs to delay in-memory statistics * block updates. Some drivers do not have an in-memory * statistic block, and in such cases this value is ignored. * This value must not be zero. */ -u32 stats_block_coalesce_usecs; +__u32 stats_block_coalesce_usecs;/* Adaptive RX/TX coalescing is an algorithm implemented by * some drivers to improve latency under low packet rates and
@@ -140,18 +140,18 @@* not implemented by the driver causes these values to be * silently ignored. */ -u32 use_adaptive_rx_coalesce; -u32 use_adaptive_tx_coalesce; +__u32 use_adaptive_rx_coalesce; +__u32 use_adaptive_tx_coalesce;/* When the packet rate (measured in packets per second) * is below pkt_rate_low, the {rx,tx}_*_low parameters are * used. */ -u32 pkt_rate_low; -u32 rx_coalesce_usecs_low; -u32 rx_max_coalesced_frames_low; -u32 tx_coalesce_usecs_low; -u32 tx_max_coalesced_frames_low; +__u32 pkt_rate_low; +__u32 rx_coalesce_usecs_low; +__u32 rx_max_coalesced_frames_low; +__u32 tx_coalesce_usecs_low; +__u32 tx_max_coalesced_frames_low;/* When the packet rate is below pkt_rate_high but above * pkt_rate_low (both measured in packets per second) the
@@ -162,43 +162,43 @@* is above pkt_rate_high, the {rx,tx}_*_high parameters are * used. */ -u32 pkt_rate_high; -u32 rx_coalesce_usecs_high; -u32 rx_max_coalesced_frames_high; -u32 tx_coalesce_usecs_high; -u32 tx_max_coalesced_frames_high; +__u32 pkt_rate_high; +__u32 rx_coalesce_usecs_high; +__u32 rx_max_coalesced_frames_high; +__u32 tx_coalesce_usecs_high; +__u32 tx_max_coalesced_frames_high;/* How often to do adaptive coalescing packet rate sampling, * measured in seconds. Must not be zero. */ -u32 rate_sample_interval; +__u32 rate_sample_interval; };/* for configuring RX/TX ring parameters */ struct ethtool_ringparam { -u32 cmd;/* ETHTOOL_{G,S}RINGPARAM */ +__u32 cmd;/* ETHTOOL_{G,S}RINGPARAM */ /* Read only attributes. These indicate the maximum number * of pending RX/TX ring entries the driver will allow the * user to set. */ -u32 rx_max_pending; -u32 rx_mini_max_pending; -u32 rx_jumbo_max_pending; -u32 tx_max_pending; +__u32 rx_max_pending; +__u32 rx_mini_max_pending; +__u32 rx_jumbo_max_pending; +__u32 tx_max_pending;/* Values changeable by the user. The valid values are * in the range 1 to the "*_max_pending" counterpart above. */ -u32 rx_pending; -u32 rx_mini_pending; -u32 rx_jumbo_pending; -u32 tx_pending; +__u32 rx_pending; +__u32 rx_mini_pending; +__u32 rx_jumbo_pending; +__u32 tx_pending; };/* for configuring link flow control parameters */ struct ethtool_pauseparam { -u32 cmd;/* ETHTOOL_{G,S}PAUSEPARAM */ +__u32 cmd;/* ETHTOOL_{G,S}PAUSEPARAM */ /* If the link is being auto-negotiated (via ethtool_cmd.autoneg * being true) the user may set 'autonet' here non-zero to have the
@@ -210,9 +210,9 @@* then {rx,tx}_pause force the driver to use/not-use pause * flow control. */ -u32 autoneg; -u32 rx_pause; -u32 tx_pause; +__u32 autoneg; +__u32 rx_pause; +__u32 tx_pause; };#define ETH_GSTRING_LEN 32
@@ -223,10 +223,10 @@/* for passing string sets for data tagging */ struct ethtool_gstrings { -u32 cmd;/* ETHTOOL_GSTRINGS */ -u32 string_set;/* string set id e.c. ETH_SS_TEST, etc*/ -u32 len;/* number of strings in the string set */ -u8 data[0 ]; +__u32 cmd;/* ETHTOOL_GSTRINGS */ +__u32 string_set;/* string set id e.c. ETH_SS_TEST, etc*/ +__u32 len;/* number of strings in the string set */ +__u8 data[0 ]; };enum ethtool_test_flags {
@@ -236,28 +236,28 @@/* for requesting NIC test and getting results*/ struct ethtool_test { -u32 cmd;/* ETHTOOL_TEST */ -u32 flags;/* ETH_TEST_FL_xxx */ -u32 reserved; -u32 len;/* result length, in number of u64 elements */ -u64 data[0 ]; +__u32 cmd;/* ETHTOOL_TEST */ +__u32 flags;/* ETH_TEST_FL_xxx */ +__u32 reserved; +__u32 len;/* result length, in number of __u64 elements */ +__u64 data[0 ]; };/* for dumping NIC-specific statistics */ struct ethtool_stats { -u32 cmd;/* ETHTOOL_GSTATS */ -u32 n_stats;/* number of u64's being returned */ -u64 data[0 ]; +__u32 cmd;/* ETHTOOL_GSTATS */ +__u32 n_stats;/* number of __u64's being returned */ +__u64 data[0 ]; };struct net_device ;/* Some generic methods drivers may use in their ethtool_ops */ -u32 ethtool_op_get_link (struct net_device *dev); -u32 ethtool_op_get_tx_csum (struct net_device *dev); -int ethtool_op_set_tx_csum (struct net_device *dev,u32 data); -u32 ethtool_op_get_sg (struct net_device *dev); -int ethtool_op_set_sg (struct net_device *dev,u32 data); +__u32 ethtool_op_get_link (struct net_device *dev); +__u32 ethtool_op_get_tx_csum (struct net_device *dev); +int ethtool_op_set_tx_csum (struct net_device *dev,__u32 data); +__u32 ethtool_op_get_sg (struct net_device *dev); +int ethtool_op_set_sg (struct net_device *dev,__u32 data);/** * ðtool_ops - Alter and report network device settings
@@ -320,31 +320,31 @@void (*get_regs)(struct net_device *,struct ethtool_regs *,void *);void (*get_wol)(struct net_device *,struct ethtool_wolinfo *);int (*set_wol)(struct net_device *,struct ethtool_wolinfo *); -u32 (*get_msglevel)(struct net_device *); -void (*set_msglevel)(struct net_device *,u32 ); +__u32 (*get_msglevel)(struct net_device *); +void (*set_msglevel)(struct net_device *,__u32 );int (*nway_reset)(struct net_device *); -u32 (*get_link)(struct net_device *); +__u32 (*get_link)(struct net_device *);int (*get_eeprom_len)(struct net_device *); -int (*get_eeprom)(struct net_device *,struct ethtool_eeprom *,u8 *); -int (*set_eeprom)(struct net_device *,struct ethtool_eeprom *,u8 *); +int (*get_eeprom)(struct net_device *,struct ethtool_eeprom *,__u8 *); +int (*set_eeprom)(struct net_device *,struct ethtool_eeprom *,__u8 *);int (*get_coalesce)(struct net_device *,struct ethtool_coalesce *);int (*set_coalesce)(struct net_device *,struct ethtool_coalesce *);void (*get_ringparam)(struct net_device *,struct ethtool_ringparam *);int (*set_ringparam)(struct net_device *,struct ethtool_ringparam *);void (*get_pauseparam)(struct net_device *,struct ethtool_pauseparam *);int (*set_pauseparam)(struct net_device *,struct ethtool_pauseparam *); -u32 (*get_rx_csum)(struct net_device *); -int (*set_rx_csum)(struct net_device *,u32 ); -u32 (*get_tx_csum)(struct net_device *); -int (*set_tx_csum)(struct net_device *,u32 ); -u32 (*get_sg)(struct net_device *); -int (*set_sg)(struct net_device *,u32 ); +__u32 (*get_rx_csum)(struct net_device *); +int (*set_rx_csum)(struct net_device *,__u32 ); +__u32 (*get_tx_csum)(struct net_device *); +int (*set_tx_csum)(struct net_device *,__u32 ); +__u32 (*get_sg)(struct net_device *); +int (*set_sg)(struct net_device *,__u32 );int (*self_test_count)(struct net_device *); -void (*self_test)(struct net_device *,struct ethtool_test *,u64 *); -void (*get_strings)(struct net_device *,u32 stringset,u8 *); -int (*phys_id)(struct net_device *,u32 ); +void (*self_test)(struct net_device *,struct ethtool_test *,__u64 *); +void (*get_strings)(struct net_device *,__u32 stringset,__u8 *); +int (*phys_id)(struct net_device *,__u32 );int (*get_stats_count)(struct net_device *); -void (*get_ethtool_stats)(struct net_device *,struct ethtool_stats *,u64 *); +void (*get_ethtool_stats)(struct net_device *,struct ethtool_stats *,__u64 *); };/* CMDs currently supported */
Quoted 3 lines · Eric Brower
Attached is a patch to 2.4's
ethtool.hto use appropriate, userspace-accessible data types (__u8and friends, rather than u8 and friends).
if we *know* the width of them, why don't we just use C99 standard types in *both* places? I've never quite grokked why we need u8 and __u8 and all the variants, when we now have uint8_t. I mean, at least it's standardized.
Anyone have a logical answer?
Quoted 10 lines · Tim Hockin
Quoted 3 lines · Eric Brower
Attached is a patch to 2.4's
ethtool.hto use appropriate, userspace-accessible data types (__u8and friends, rather than u8 and friends).if we *know* the width of them, why don't we just use C99 standard types in *both* places? I've never quite grokked why we need u8 and
__u8and all the variants, when we now haveuint8_t. I mean, at least it's standardized.Anyone have a logical answer?
Yes.
u8 means an unsigned 8 bit variable within the kernel. __u8 means an unsigned 8 bit variable both within the kernel and in userspace. Use the __ forms when describing data structures or variables that cross the userspace/kernelspace boundry in order to get everything correct.
Becides, something like u8 is a zillion times saner than uint8_t, don't you think? :)
I remember saying that I would document this better a long time ago, as it comes up every other month or so. Sorry for not doing it yet...
thanks,
greg k-h
Quoted 5 lines · Greg KH
u8 means an unsigned 8 bit variable within the kernel.
__u8means an unsigned 8 bit variable both within the kernel and in userspace. Use the __ forms when describing data structures or variables that cross the userspace/kernelspace boundry in order to get everything correct.
The only problem is that this is a RETARDED precedent. __foo means the "internal" version of foo. Within the kernel __foo often means the version of foo without "the lock".
POSIX defines any type beginning with _ as part of the implementation. IMHO, no userspace app should *ever* need a type that starts with _ or __. It's just dumb. If the width matters, define it as a width specific type. If it doesn't define it as an int/short/whatever. The fact that it isn't obvious what is or is not kernel-only is an entirely separate issue.
Quoted 2 lines
Becides, something like u8 is a zillion times saner than
uint8_t, don't you think? :)
As much as uint8_t is a pain in the ass to type, at least it is clear and obvious and standard, don't you think? :)
Quoted 2 lines · Eric Brower
Attached is a patch to 2.4's
ethtool.hto use appropriate, userspace-accessible data types (__u8and friends, rather than u8 and friends).
Why there is no #ifdef __KERNEL__ in this header to make it userspace safe.
Quoted 2 lines
I've also posted a patch against ethtool-1.8 to the sf.net gkernel site that cleans-up
ethtool-copy.hand the rest in the same manner.
In user space the types from stdint.h likely should be used. So __u8 and friends looks wrong there as well.
Eric
Quoted 9 lines · Eric W. Biederman
Quoted 2 lines · Eric Brower
Attached is a patch to 2.4's
ethtool.hto use appropriate, userspace-accessible data types (__u8and friends, rather than u8 and friends).Why there is no #ifdef
__KERNEL__in this header to make it userspace safe.
Because it's not needed.
Jeff
Quoted 4 lines · Jeff Garzik
Quoted 3 lines · Eric W. Biederman
Quoted 1 line · Eric Brower
Attached is a patch to 2.4's
ethtool.hto use appropriate,
Quoted 8 lines
userspace-accessible
Quoted 3 lines
Quoted 1 line
data types (
__u8and friends, rather than u8 and friends).Why there is no #ifdef
__KERNEL__in this header to make it userspace safe.Because it's not needed.
I think we are in agreement.
My intent was to say: Why change the types when there is no #ifdef __KERNEL__ in the header. With no #ifdef __KERNEL__ it exports definitions that are private to the kernel making it not safe for userspace to use. With kernel private definitions in there it will generate name space pollution if included by user space.
Eric
Quoted 5 lines · Eric W. Biederman
My intent was to say: Why change the types when there is no #ifdef
__KERNEL__in the header. With no #ifdef__KERNEL__it exports definitions that are private to the kernel making it not safe for userspace to use. With kernel private definitions in there it will generate name space pollution if included by user space.
Presumably because it *is* included by userspace, because it defines the interface between the kernel and userspace; of course userspace will (does) include it.
Signature · 1 line
/Jonathan Lundell.
Quoted 10 lines · Jonathan Lundell
Quoted 5 lines · Eric W. Biederman
My intent was to say: Why change the types when there is no #ifdef
__KERNEL__in the header. With no #ifdef__KERNEL__it exports definitions that are private to the kernel making it not safe for userspace to use. With kernel private definitions in there it will generate name space pollution if included by user space.Presumably because it *is* included by userspace, because it defines the interface between the kernel and userspace; of course userspace will (does) include it.
Well that's a bug. You don't include kernel headers in userspace. Doing so has been deprecated for 8 years and 3 major releases.
Signature · 1 line
Matt Mackall : http://www.selenic.com : Linux development and consulting
Quoted 8 lines · Matt Mackall
Quoted 7 lines · Jonathan Lundell
Quoted 5 lines · Eric W. Biederman
My intent was to say: Why change the types when there is no #ifdef
__KERNEL__in the header. With no #ifdef__KERNEL__it exports definitions that are private to the kernel making it not safe for userspace to use. With kernel private definitions in there it will generate name space pollution if included by user space.
Quoted 6 lines
Quoted 3 lines
Presumably because it *is* included by userspace, because it defines the interface between the kernel and userspace; of course userspace will (does) include it.
Well that's a bug. You don't include kernel headers in userspace. Doing so has been deprecated for 8 years and 3 major releases.
Then it's a bug for the ethtool maintainer. ethtool.c #includes ethtool-copy.h, a renamed copy of the kernel header file.
I'm familiar with the deprecation and continue to be bewildered by it. To take this particular example, ethtool.h defines an interface between a kernel facility and a userspace client. What's the non-deprecated method of sharing the interface definition?
Signature · 1 line
/Jonathan Lundell.
Quoted 1 line · Eric Brower
Attached is a patch to 2.4's
ethtool.hto use appropriate, userspace-accessible data types (__u8and friends, rather than u8 and friends).
I'm happy with u8/u16/u32, so it stays that way :)
Jeff