[net-next,1/3] Added kernel support in EEE Ethtool commands

This patch extends the kernel's ethtool interface by adding support
for 2 new EEE commands - get_eee and set_eee.

Thanks goes to Giuseppe Cavallaro for his original patch adding this support.

Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 include/linux/ethtool.h |   19 +++++++++++++++++++
 net/core/ethtool.c      |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 0 deletions(-)

Hello Yuval,

On 6/5/2012 8:39 AM, Yuval Mintz wrote:
> This patch extends the kernel's ethtool interface by adding support
> for 2 new EEE commands - get_eee and set_eee.
> 
> Thanks goes to Giuseppe Cavallaro for his original patch adding this support.

Thank to you for having re-worked/re-stored the code I had proposed and
sorry if I was not able to post my patches in time.
Indeed, I had already done some progresses on EEE so I could re-base it
against your work as soon as reviewed.

For example, I could especially contribute on the eee get/set for the
PHY part. I had already sent the patches for phy_device.c (Ben's already
helped me on these) and I am happy to rework them.

Then I could test and verify all (ethtool + phy + user-space
application) with the stmmac d.d. on my platforms.

[snip]

> +	__u32	tx_lpi_enabled;
> +	__u32	tx_lpi_timer;

Is the tx_lpi_timer field for the MAC driver as we discussed in the
past? If yes, so I can use it for the stmmac too.

Peppe

> +	__u32	reserved[2];
> +};
> +
>  /**
>   * struct ethtool_modinfo - plugin module eeprom information
>   * @cmd: %ETHTOOL_GMODULEINFO
> @@ -945,6 +958,8 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>   * @get_module_info: Get the size and type of the eeprom contained within
>   *	a plug-in module.
>   * @get_module_eeprom: Get the eeprom information from the plug-in module
> + * @get_eee: Get Energy-Efficient (EEE) supported and status.
> + * @set_eee: Set EEE status (enable/disable) as well as LPI timers.
>   *
>   * All operations are optional (i.e. the function pointer may be set
>   * to %NULL) and callers must take this into account.  Callers must
> @@ -1011,6 +1026,8 @@ struct ethtool_ops {
>  				   struct ethtool_modinfo *);
>  	int     (*get_module_eeprom)(struct net_device *,
>  				     struct ethtool_eeprom *, u8 *);
> +	int	(*get_eee)(struct net_device *, struct ethtool_eee *);
> +	int	(*set_eee)(struct net_device *, struct ethtool_eee *);
>  
>  
>  };
> @@ -1089,6 +1106,8 @@ struct ethtool_ops {
>  #define ETHTOOL_GET_TS_INFO	0x00000041 /* Get time stamping and PHC info */
>  #define ETHTOOL_GMODULEINFO	0x00000042 /* Get plug-in module information */
>  #define ETHTOOL_GMODULEEEPROM	0x00000043 /* Get plug-in module eeprom */
> +#define ETHTOOL_GEEE		0x00000044 /* Get EEE settings */
> +#define ETHTOOL_SEEE		0x00000045 /* Set EEE settings */
>  
>  /* compatibility with older code */
>  #define SPARC_ETH_GSET		ETHTOOL_GSET
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 9c2afb4..7940bd3 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -729,6 +729,34 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
>  	return dev->ethtool_ops->set_wol(dev, &wol);
>  }
>  
> +static int ethtool_get_eee(struct net_device *dev, char __user *useraddr)
> +{
> +	struct ethtool_eee edata;
> +
> +	if (!dev->ethtool_ops->get_eee)
> +		return -EOPNOTSUPP;
> +
> +	dev->ethtool_ops->get_eee(dev, &edata);
> +
> +	if (copy_to_user(useraddr, &edata, sizeof(edata)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int ethtool_set_eee(struct net_device *dev, char __user *useraddr)
> +{
> +	struct ethtool_eee edata;
> +
> +	if (!dev->ethtool_ops->get_eee)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&edata, useraddr, sizeof(edata)))
> +		return -EFAULT;
> +
> +	return dev->ethtool_ops->set_eee(dev, &edata);
> +}
> +
>  static int ethtool_nway_reset(struct net_device *dev)
>  {
>  	if (!dev->ethtool_ops->nway_reset)
> @@ -1471,6 +1499,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>  		rc = ethtool_set_value_void(dev, useraddr,
>  				       dev->ethtool_ops->set_msglevel);
>  		break;
> +	case ETHTOOL_GEEE:
> +		rc = ethtool_get_eee(dev, useraddr);
> +		break;
> +	case ETHTOOL_SEEE:
> +		rc = ethtool_set_eee(dev, useraddr);
> +		break;
>  	case ETHTOOL_NWAY_RST:
>  		rc = ethtool_nway_reset(dev);
>  		break;

--
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 06/05/2012 11:00 AM, Giuseppe CAVALLARO wrote:

> Hello Yuval,
> 
> On 6/5/2012 8:39 AM, Yuval Mintz wrote:
>> This patch extends the kernel's ethtool interface by adding support
>> for 2 new EEE commands - get_eee and set_eee.
> 
>> +	__u32	tx_lpi_enabled;
>> +	__u32	tx_lpi_timer;
> 
> Is the tx_lpi_timer field for the MAC driver as we discussed in the
> past? If yes, so I can use it for the stmmac too.
> 
> Peppe
> 



Hi Peppe,

The short answer is yes.

The API does not specify whether the delay should be made in the MAC
or controller levels, but it should guarantee that if the nic is idle
it should wait that many microseconds prior to asserting its Tx lpi.

Thanks,
Yuval

--
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 6/5/2012 10:11 AM, Yuval Mintz wrote:
> On 06/05/2012 11:00 AM, Giuseppe CAVALLARO wrote:
> 
>> Hello Yuval,
>>
>> On 6/5/2012 8:39 AM, Yuval Mintz wrote:
>>> This patch extends the kernel's ethtool interface by adding support
>>> for 2 new EEE commands - get_eee and set_eee.
>>
>>> +	__u32	tx_lpi_enabled;
>>> +	__u32	tx_lpi_timer;
>>
>> Is the tx_lpi_timer field for the MAC driver as we discussed in the
>> past? If yes, so I can use it for the stmmac too.
>>
>> Peppe
>>
> 
> 
> 
> Hi Peppe,
> 
> The short answer is yes.
> 
> The API does not specify whether the delay should be made in the MAC
> or controller levels, but it should guarantee that if the nic is idle
> it should wait that many microseconds prior to asserting its Tx lpi.

Perfect! So I'll give you the patches for phy and stmmac asap.

Peppe

> 
> Thanks,
> Yuval
> 
> 

--
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 Tue, 2012-06-05 at 09:39 +0300, Yuval Mintz wrote:
> This patch extends the kernel's ethtool interface by adding support
> for 2 new EEE commands - get_eee and set_eee.
> 
> Thanks goes to Giuseppe Cavallaro for his original patch adding this support.
> 
> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> ---
>  include/linux/ethtool.h |   19 +++++++++++++++++++
>  net/core/ethtool.c      |   34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index e17fa71..527de4c 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -136,6 +136,19 @@ struct ethtool_eeprom {
>  	__u8	data[0];
>  };
>  
> +/* EEE settings */
> +struct ethtool_eee {
> +	__u32	cmd;
> +	__u32	supported;
> +	__u32	advertised;
> +	__u32	lp_advertised;
> +	__u32	eee_active;
> +	__u32	eee_enabled;
> +	__u32	tx_lpi_enabled;
> +	__u32	tx_lpi_timer;
> +	__u32	reserved[2];
> +};

This needs a kernel-doc comment explaining exactly what each of the
fields means.

[...]
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -729,6 +729,34 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
>  	return dev->ethtool_ops->set_wol(dev, &wol);
>  }
>  
> +static int ethtool_get_eee(struct net_device *dev, char __user *useraddr)
> +{
> +	struct ethtool_eee edata;
> +
> +	if (!dev->ethtool_ops->get_eee)
> +		return -EOPNOTSUPP;

We *must* initialise all of edata and we should not leave it to the
driver to do.  Otherwise we will be copying back uninitialised data to
userland which (1) might reveal sensitive information stored on the
stack by previous function calls (2) sets the cmd field to the wrong
value (3) sets the reserved fields to random values, making them
unusable for expansion.

> +	dev->ethtool_ops->get_eee(dev, &edata);

Missing error check.

> +	if (copy_to_user(useraddr, &edata, sizeof(edata)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
[...]