Re: [net-2.6 PATCH 9/10] Neterion: New driver: Ethtool related

[Ben Hutchings <bhutchings@solarflare.com>]

On Sat, 2009-03-14 at 00:22 -0800, Ramkrishna Vepa wrote:
> This patch implements all ethtool related entry point functions for the driver.
> 
> Signed-off-by: Sivakumar Subramani <sivakumar.subramani@neterion.com>
> Signed-off-by: Rastapur Santosh <santosh.rastapur@neterion.com>
> Signed-off-by: Ramkrishna Vepa <ram.vepa@neterion.com>
> ---
> diff -urpN patch_8/drivers/net/vxge/vxge-ethtool.c patch_9/drivers/net/vxge/vxge-ethtool.c
> --- patch_8/drivers/net/vxge/vxge-ethtool.c	1969-12-31 16:00:00.000000000 -0800
> +++ patch_9/drivers/net/vxge/vxge-ethtool.c	2009-03-13 00:15:35.000000000 -0700
[...
> +/*
> + * vxge_ethtool_sset - Sets different link parameters.
> + * @dev: device pointer.
> + * @info: pointer to the structure with parameters given by ethtool to set
> + * link information.
> + *
> + * The function sets different link parameters provided by the user onto
> + * the NIC.
> + * Return value:
> + * 0 on success.
> + */
> +
> +static int vxge_ethtool_sset(struct net_device *dev, struct ethtool_cmd *info)
> +{
> +	if ((info->autoneg == AUTONEG_ENABLE) ||
> +	    (info->speed != SPEED_10000) || (info->duplex != DUPLEX_FULL))
> +		return -EINVAL;
> +
> +	if (netif_running(dev)) {
> +		vxge_close(dev);
> +		vxge_open(dev);

Why?

> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * vxge_ethtool_gset - Return link specific information.
> + * @dev: device pointer.
> + * @info: pointer to the structure with parameters given by ethtool
> + * to return link information.
> + *
> + * Returns link specific information like speed, duplex etc.. to ethtool.
> + * Return value :
> + * return 0 on success.
> + */
> +int vxge_ethtool_gset(struct net_device *dev, struct ethtool_cmd
> +*info)
> +{
> +	info->supported = (SUPPORTED_10000baseT_Full | SUPPORTED_FIBRE);
> +
> +#ifdef ADVERTISED_10000baseT_Full
> +	info->advertising = ADVERTISED_10000baseT_Full;
> +#else
> +	info->advertising = SUPPORTED_10000baseT_Full;
> +#endif
> +
> +#ifdef ADVERTISED_FIBRE
> +	info->advertising  |= ADVERTISED_FIBRE;
> +#else
> +	info->advertising  |= SUPPORTED_FIBRE;
> +#endif

Do not use #ifdef here.  You can be sure that these definitions aren't
going away.

[...]
> +/*
> + * vxge_ethtool_gdrvinfo - Returns driver specific information.
> + * @dev: device pointer.
> + * @info: pointer to the structure with parameters given by ethtool to
> + * return driver information.
> + *
> + * Returns driver specefic information like name, version etc.. to ethtool.
> + */
> +static void vxge_ethtool_gdrvinfo(struct net_device *dev,
> +			struct ethtool_drvinfo *info)
> +{
> +	struct vxgedev *vdev;
> +	char version[80];
> +	vdev = (struct vxgedev *)netdev_priv(dev);
> +	sprintf(version, "%s", DRV_VERSION);

What is the point of this intermediate copy?

> +	strncpy(info->driver, VXGE_DRIVER_NAME, sizeof(VXGE_DRIVER_NAME));
> +	strncpy(info->version, version, sizeof(version));
> +	strncpy(info->fw_version, vdev->fw_version, VXGE_HW_FW_VERSION_LEN);
> +	strncpy(info->bus_info, pci_name(vdev->pdev), sizeof(info->bus_info));

Use strlcpy() to ensure null-termination.

[...]
> +static int vxge_ethtool_idnic(struct net_device *dev, u32 data)
> +{
> +	u32 port[3] = {0, 1, 2};
> +	int i;
> +	struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> +	struct __hw_device  *hldev = (struct __hw_device  *)
> +			pci_get_drvdata(vdev->pdev);
> +
> +	if (vdev->id_timer.function == NULL) {
> +		init_timer(&vdev->id_timer);
> +		vdev->id_timer.function = vxge_phy_id;
> +		vdev->id_timer.data = (unsigned long) vdev;
> +	}
> +	mod_timer(&vdev->id_timer, jiffies);
> +	set_current_state(TASK_INTERRUPTIBLE);
> +	if (data)
> +		schedule_timeout(data * HZ);
> +	else
> +		schedule_timeout(MAX_SCHEDULE_TIMEOUT);

Just schedule() will do.

[...]
> +static u32 vxge_ethtool_op_get_tso(struct net_device *dev)
> +{
> +	return (dev->features & NETIF_F_TSO) != 0;
> +}

This is the same as ethtool_op_get_tso; just point to that.

[...]
> +int vxge_ethtool_self_test_count(struct net_device *dev)
> +{
> +	return VXGE_TEST_LEN;
> +}
> +
> +static int vxge_ethtool_get_stats_count(struct net_device *dev)
> +{
> +	struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> +	return VXGE_TITLE_LEN + (vdev->no_of_vpath *
> +		VXGE_HW_VPATH_STATS_LEN) +
> +		(vdev->max_config_port * VXGE_HW_AGGR_STATS_LEN) +
> +		(vdev->max_config_port * VXGE_HW_PORT_STATS_LEN) +
> +		(vdev->no_of_vpath * VXGE_HW_VPATH_TX_STATS_LEN) +
> +		(vdev->no_of_vpath * VXGE_HW_VPATH_RX_STATS_LEN) +
> +
> +		(vdev->no_of_vpath * VXGE_SW_STATS_LEN) + DRIVER_STAT_LEN;
> +
> +}

These operations are deprecated; you should implement get_strings_count
instead.

> +int vxge_ethtool_op_set_tx_csum(struct net_device *dev, u32 data)
> +{
> +	if (data)
> +		dev->features |= NETIF_F_HW_CSUM;
> +	else
> +		dev->features &= ~NETIF_F_HW_CSUM;
> +
> +	return 0;
> +}

This is the same as ethtool_op_set_tx_hw_csum; just point to that.

[...]
> +struct ethtool_ops netdev_ethtool_ops = {
> +	.get_settings = vxge_ethtool_gset,
> +	.set_settings = vxge_ethtool_sset,
> +	.get_drvinfo = vxge_ethtool_gdrvinfo,
> +	.get_regs_len = vxge_ethtool_get_regs_len,
> +	.get_regs = vxge_ethtool_gregs,
> +	.get_link = ethtool_op_get_link,
> +
> +	.get_eeprom_len = vxge_get_eeprom_len,
> +
> +	.get_eeprom = NULL,
> +	.set_eeprom = NULL,

So what is the point of reporting the EEPROM length?

> +	.get_pauseparam = vxge_ethtool_getpause_data,
> +	.set_pauseparam = vxge_ethtool_setpause_data,
> +	.get_rx_csum = vxge_get_rx_csum,
> +	.set_rx_csum = vxge_set_rx_csum,
> +	.get_tx_csum = ethtool_op_get_tx_csum,
> +	.set_tx_csum = vxge_ethtool_op_set_tx_csum,
> +	.get_sg = ethtool_op_get_sg,
> +	.set_sg = ethtool_op_set_sg,
> +
> +	.get_tso = vxge_ethtool_op_get_tso,
> +	.set_tso = vxge_ethtool_op_set_tso,
> +
> +	.self_test_count = vxge_ethtool_self_test_count,
> +	.self_test = NULL,
[...]

Don't claim to have tests you don't actually implement.

Ben.

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