Re: [PATCH] ethtool: command line support for lro

["Kok, Auke" <auke-jan.h.kok@intel.com>]

Ben Hutchings wrote:
> Add lro support to command in similar manner to TSO, GSO, etc.
> The file ethtool-copy.h is updated to be sanitised version of
> ethtool.h from 2.6.25-rc4 (ie make headers_install)
> 
> Based on patch by Stephen Hemminger
> <http://article.gmane.org/gmane.linux.network/88124>.
> 
> My changes:
> - Changed error return codes for setting LRO to be unique.
> - Report failure to get device flags, consistent with other offload settings.
> - Fixed code spacing.
> 
> Tested with the sfc driver.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> diff --git a/ethtool-copy.h b/ethtool-copy.h
> index 3a63224..87c7c9a 100644
> --- a/ethtool-copy.h
> +++ b/ethtool-copy.h
> @@ -39,7 +39,8 @@ struct ethtool_drvinfo {
>  	char	bus_info[ETHTOOL_BUSINFO_LEN];	/* Bus info for this IF. */
>  				/* For PCI devices, use pci_name(pci_dev). */
>  	char	reserved1[32];
> -	char	reserved2[16];
> +	char	reserved2[12];
> +	__u32	n_priv_flags;	/* number of flags valid in ETHTOOL_GPFLAGS */
>  	__u32	n_stats;	/* number of u64's from ETHTOOL_GSTATS */
>  	__u32	testinfo_len;
>  	__u32	eedump_len;	/* Size of data from ETHTOOL_GEEPROM (bytes) */
> @@ -219,6 +220,7 @@ struct ethtool_pauseparam {
>  enum ethtool_stringset {
>  	ETH_SS_TEST		= 0,
>  	ETH_SS_STATS,
> +	ETH_SS_PRIV_FLAGS,
>  };
>  
>  /* for passing string sets for data tagging */
> @@ -256,125 +258,19 @@ struct ethtool_perm_addr {
>  	__u8	data[0];
>  };
>  
> -#ifdef __KERNEL__
> -
> -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);
> -int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data);
> -int ethtool_op_set_tx_ipv6_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_tso(struct net_device *dev);
> -int ethtool_op_set_tso(struct net_device *dev, u32 data);
> -int ethtool_op_get_perm_addr(struct net_device *dev, 
> -			     struct ethtool_perm_addr *addr, u8 *data);
> -u32 ethtool_op_get_ufo(struct net_device *dev);
> -int ethtool_op_set_ufo(struct net_device *dev, u32 data);
> -
> -/**
> - * &ethtool_ops - Alter and report network device settings
> - * get_settings: Get device-specific settings
> - * set_settings: Set device-specific settings
> - * get_drvinfo: Report driver information
> - * get_regs: Get device registers
> - * get_wol: Report whether Wake-on-Lan is enabled
> - * set_wol: Turn Wake-on-Lan on or off
> - * get_msglevel: Report driver message level
> - * set_msglevel: Set driver message level
> - * nway_reset: Restart autonegotiation
> - * get_link: Get link status
> - * get_eeprom: Read data from the device EEPROM
> - * set_eeprom: Write data to the device EEPROM
> - * get_coalesce: Get interrupt coalescing parameters
> - * set_coalesce: Set interrupt coalescing parameters
> - * get_ringparam: Report ring sizes
> - * set_ringparam: Set ring sizes
> - * get_pauseparam: Report pause parameters
> - * set_pauseparam: Set pause paramters
> - * get_rx_csum: Report whether receive checksums are turned on or off
> - * set_rx_csum: Turn receive checksum on or off
> - * get_tx_csum: Report whether transmit checksums are turned on or off
> - * set_tx_csum: Turn transmit checksums on or off
> - * get_sg: Report whether scatter-gather is enabled
> - * set_sg: Turn scatter-gather on or off
> - * get_tso: Report whether TCP segmentation offload is enabled
> - * set_tso: Turn TCP segmentation offload on or off
> - * get_ufo: Report whether UDP fragmentation offload is enabled
> - * set_ufo: Turn UDP fragmentation offload on or off
> - * self_test: Run specified self-tests
> - * get_strings: Return a set of strings that describe the requested objects 
> - * phys_id: Identify the device
> - * get_stats: Return statistics about the device
> - * get_perm_addr: Gets the permanent hardware address
> - * 
> - * Description:
> +/* boolean flags controlling per-interface behavior characteristics.
> + * When reading, the flag indicates whether or not a certain behavior
> + * is enabled/present.  When writing, the flag indicates whether
> + * or not the driver should turn on (set) or off (clear) a behavior.
>   *
> - * get_settings:
> - *	@get_settings is passed an &ethtool_cmd to fill in.  It returns
> - *	an negative errno or zero.
> - *
> - * set_settings:
> - *	@set_settings is passed an &ethtool_cmd and should attempt to set
> - *	all the settings this device supports.  It may return an error value
> - *	if something goes wrong (otherwise 0).
> - *
> - * get_eeprom:
> - *	Should fill in the magic field.  Don't need to check len for zero
> - *	or wraparound.  Fill in the data argument with the eeprom values
> - *	from offset to offset + len.  Update len to the amount read.
> - *	Returns an error or zero.
> - *
> - * set_eeprom:
> - *	Should validate the magic field.  Don't need to check len for zero
> - *	or wraparound.  Update len to the amount written.  Returns an error
> - *	or zero.
> + * Some behaviors may read-only (unconditionally absent or present).
> + * If such is the case, return EINVAL in the set-flags operation if the
> + * flag differs from the read-only value.
>   */
> -struct ethtool_ops {
> -	int	(*get_settings)(struct net_device *, struct ethtool_cmd *);
> -	int	(*set_settings)(struct net_device *, struct ethtool_cmd *);
> -	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
> -	int	(*get_regs_len)(struct net_device *);
> -	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);
> -	int	(*nway_reset)(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_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_tso)(struct net_device *);
> -	int	(*set_tso)(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);
> -	int	(*get_stats_count)(struct net_device *);
> -	void	(*get_ethtool_stats)(struct net_device *, struct ethtool_stats *, u64 *);
> -	int	(*get_perm_addr)(struct net_device *, struct ethtool_perm_addr *, u8 *);
> -	int	(*begin)(struct net_device *);
> -	void	(*complete)(struct net_device *);
> -	u32     (*get_ufo)(struct net_device *);
> -	int     (*set_ufo)(struct net_device *, u32);
> +enum ethtool_flags {
> +	ETH_FLAG_LRO		= (1 << 15),	/* LRO is enabled */
>  };
> -#endif /* __KERNEL__ */
> +
>  
>  /* CMDs currently supported */
>  #define ETHTOOL_GSET		0x00000001 /* Get settings. */
> @@ -414,6 +310,10 @@ struct ethtool_ops {
>  #define ETHTOOL_SUFO		0x00000022 /* Set UFO enable (ethtool_value) */
>  #define ETHTOOL_GGSO		0x00000023 /* Get GSO enable (ethtool_value) */
>  #define ETHTOOL_SGSO		0x00000024 /* Set GSO enable (ethtool_value) */
> +#define ETHTOOL_GFLAGS		0x00000025 /* Get flags bitmap(ethtool_value) */
> +#define ETHTOOL_SFLAGS		0x00000026 /* Set flags bitmap(ethtool_value) */
> +#define ETHTOOL_GPFLAGS		0x00000027 /* Get driver-private flags bitmap */
> +#define ETHTOOL_SPFLAGS		0x00000028 /* Set driver-private flags bitmap */
>  
>  /* compatibility with older code */
>  #define SPARC_ETH_GSET		ETHTOOL_GSET
> diff --git a/ethtool.c b/ethtool.c
> index a668b49..5f519bc 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -151,7 +151,9 @@ static struct option {
>  		"		[ sg on|off ]\n"
>  	        "		[ tso on|off ]\n"
>  	        "		[ ufo on|off ]\n"
> -	        "		[ gso on|off ]\n" },
> +		"		[ gso on|off ]\n"
> +		"               [ lro on|off ]\n"
> +    },
>      { "-i", "--driver", MODE_GDRV, "Show driver information" },
>      { "-d", "--register-dump", MODE_GREGS, "Do a register dump",
>  		"		[ raw on|off ]\n"
> @@ -200,6 +202,7 @@ static int off_sg_wanted = -1;
>  static int off_tso_wanted = -1;
>  static int off_ufo_wanted = -1;
>  static int off_gso_wanted = -1;
> +static int off_lro_wanted = -1;
>  
>  static struct ethtool_pauseparam epause;
>  static int gpause_changed = 0;
> @@ -310,6 +313,7 @@ static struct cmdline_info cmdline_offload[] = {
>  	{ "tso", CMDL_BOOL, &off_tso_wanted, NULL },
>  	{ "ufo", CMDL_BOOL, &off_ufo_wanted, NULL },
>  	{ "gso", CMDL_BOOL, &off_gso_wanted, NULL },
> +	{ "lro", CMDL_BOOL, &off_lro_wanted, NULL },
>  };
>  
>  static struct cmdline_info cmdline_pause[] = {
> @@ -1217,7 +1221,7 @@ static int dump_coalesce(void)
>  	return 0;
>  }
>  
> -static int dump_offload (int rx, int tx, int sg, int tso, int ufo, int gso)
> +static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso, int lro)
>  {
>  	fprintf(stdout,
>  		"rx-checksumming: %s\n"
> @@ -1225,13 +1229,15 @@ static int dump_offload (int rx, int tx, int sg, int tso, int ufo, int gso)
>  		"scatter-gather: %s\n"
>  		"tcp segmentation offload: %s\n"
>  		"udp fragmentation offload: %s\n"
> -		"generic segmentation offload: %s\n",
> +		"generic segmentation offload: %s\n"
> +		"large receive offload: %s\n",
>  		rx ? "on" : "off",
>  		tx ? "on" : "off",
>  		sg ? "on" : "off",
>  		tso ? "on" : "off",
>  		ufo ? "on" : "off",
> -		gso ? "on" : "off");
> +		gso ? "on" : "off",
> +		lro ? "on" : "off");
>  
>  	return 0;
>  }
> @@ -1495,7 +1501,8 @@ static int do_scoalesce(int fd, struct ifreq *ifr)
>  static int do_goffload(int fd, struct ifreq *ifr)
>  {
>  	struct ethtool_value eval;
> -	int err, allfail = 1, rx = 0, tx = 0, sg = 0, tso = 0, ufo = 0, gso = 0;
> +	int err, allfail = 1, rx = 0, tx = 0, sg = 0;
> +	int tso = 0, ufo = 0, gso = 0, lro = 0;
>  
>  	fprintf(stdout, "Offload parameters for %s:\n", devname);
>  
> @@ -1559,12 +1566,22 @@ static int do_goffload(int fd, struct ifreq *ifr)
>  		allfail = 0;
>  	}
>  
> +	eval.cmd = ETHTOOL_GFLAGS;
> +	ifr->ifr_data = (caddr_t)&eval;
> +	err = ioctl(fd, SIOCETHTOOL, ifr);
> +	if (err) {
> +		perror("Cannot get device flags");
> +	} else {
> +		lro = (eval.data & ETH_FLAG_LRO) != 0;
> +		allfail = 0;
> +	}
> +
>  	if (allfail) {
>  		fprintf(stdout, "no offload info available\n");
>  		return 83;
>  	}
>  
> -	return dump_offload(rx, tx, sg, tso, ufo, gso);
> +	return dump_offload(rx, tx, sg, tso, ufo, gso, lro);
>  }
>  
>  static int do_soffload(int fd, struct ifreq *ifr)
> @@ -1641,6 +1658,30 @@ static int do_soffload(int fd, struct ifreq *ifr)
>  			return 90;
>  		}
>  	}
> +	if (off_lro_wanted >= 0) {
> +		changed = 1;
> +		eval.cmd = ETHTOOL_GFLAGS;
> +		eval.data = 0;
> +		ifr->ifr_data = (caddr_t)&eval;
> +		err = ioctl(fd, SIOCETHTOOL, ifr);
> +		if (err) {
> +			perror("Cannot get device flag settings");
> +			return 91;
> +		}
> +
> +		eval.cmd = ETHTOOL_SFLAGS;
> +		if (off_lro_wanted == 1)
> +			eval.data |= ETH_FLAG_LRO;
> +		else
> +			eval.data &= ~ETH_FLAG_LRO;
> +
> +		err = ioctl(fd, SIOCETHTOOL, ifr);
> +		if (err) {
> +			perror("Cannot set large receive offload settings");
> +			return 92;
> +		}
> +	}
> +
>  	if (!changed) {
>  		fprintf(stdout, "no offload settings changed\n");
>  	}
> 

what is the status of this patch?

I don't like the fact that you included the removal of a __KERNEL__ section in
this patch - that should have been a separate patch IMHO :)

otherwise I would like to see this patch included.


Auke