| Message ID | 1330433084-18586-3-git-send-email-peppe.cavallaro@st.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2012-02-28 at 13:44 +0100, Giuseppe CAVALLARO wrote: > This patch adds two new functions to detect if Energy-Efficient > Ethernet (EEE) is supported and the way to enable/disable it. > > Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com> > --- > include/linux/ethtool.h | 7 +++++++ > net/core/ethtool.c | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 39 insertions(+), 0 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index da5b2de..fdea3c7 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -43,6 +43,7 @@ struct ethtool_cmd { > __u8 eth_tp_mdix; > __u8 reserved2; > __u32 lp_advertising; /* Features the link partner advertises */ > + __u32 eee; /* Energy-Efficient Etehrnet */ > __u32 reserved[2]; > }; You can't change the size of this structure. > @@ -874,6 +875,8 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) > * and flag of the device. > * @get_dump_data: Get dump data. > * @set_dump: Set dump specific flags to the device. > + * @get_eee: Get Energy-Efficient Ethernet (EEE) supported and status. > + * @set_eee: Set EEE status (enable/disable). > * > * All operations are optional (i.e. the function pointer may be set > * to %NULL) and callers must take this into account. Callers must > @@ -937,6 +940,8 @@ struct ethtool_ops { > struct ethtool_dump *, void *); > int (*set_dump)(struct net_device *, struct ethtool_dump *); > > + int (*get_eee) (struct net_device *, struct ethtool_value *); > + int (*set_eee) (struct net_device *, struct ethtool_value *); [...] Why are there new operations as well as a new field to ethtool_cmd? Is the value a boolean or a bitmask? Since EEE is an auto-negotiated feature, why aren't you defining flags for the supported/advertising/lp_advertising fields? Ben.
Hello Ben, On 2/28/2012 4:45 PM, Ben Hutchings wrote: > On Tue, 2012-02-28 at 13:44 +0100, Giuseppe CAVALLARO wrote: >> This patch adds two new functions to detect if Energy-Efficient >> Ethernet (EEE) is supported and the way to enable/disable it. >> >> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com> >> --- >> include/linux/ethtool.h | 7 +++++++ >> net/core/ethtool.c | 32 ++++++++++++++++++++++++++++++++ >> 2 files changed, 39 insertions(+), 0 deletions(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index da5b2de..fdea3c7 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -43,6 +43,7 @@ struct ethtool_cmd { >> __u8 eth_tp_mdix; >> __u8 reserved2; >> __u32 lp_advertising; /* Features the link partner advertises */ >> + __u32 eee; /* Energy-Efficient Etehrnet */ >> __u32 reserved[2]; >> }; > > You can't change the size of this structure. hmm, sorry but it's likely I missed something. Let me review the code and so I come back on this. > >> @@ -874,6 +875,8 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) >> * and flag of the device. >> * @get_dump_data: Get dump data. >> * @set_dump: Set dump specific flags to the device. >> + * @get_eee: Get Energy-Efficient Ethernet (EEE) supported and status. >> + * @set_eee: Set EEE status (enable/disable). >> * >> * All operations are optional (i.e. the function pointer may be set >> * to %NULL) and callers must take this into account. Callers must >> @@ -937,6 +940,8 @@ struct ethtool_ops { >> struct ethtool_dump *, void *); >> int (*set_dump)(struct net_device *, struct ethtool_dump *); >> >> + int (*get_eee) (struct net_device *, struct ethtool_value *); >> + int (*set_eee) (struct net_device *, struct ethtool_value *); > [...] > > Why are there new operations as well as a new field to ethtool_cmd? > > Is the value a boolean or a bitmask? the former. These are for enabling/disabling the EEE and also to get the status. > Since EEE is an auto-negotiated feature, why aren't you defining flags > for the supported/advertising/lp_advertising fields? I added these because the stmmac needs to have a timer to enter in LPI tx mode. So this can be stopped by using ethtool. I didn't define flags for supported/advertising/lp_advertising because these are fixed in the RO MMD register and it makes no sense to modify them (hmm, I think :-)). Regards Peppe > > Ben. > -- 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 Wed, 2012-02-29 at 17:12 +0100, Giuseppe CAVALLARO wrote: > Hello Ben, > > On 2/28/2012 4:45 PM, Ben Hutchings wrote: > > On Tue, 2012-02-28 at 13:44 +0100, Giuseppe CAVALLARO wrote: [...] > >> @@ -937,6 +940,8 @@ struct ethtool_ops { > >> struct ethtool_dump *, void *); > >> int (*set_dump)(struct net_device *, struct ethtool_dump *); > >> > >> + int (*get_eee) (struct net_device *, struct ethtool_value *); > >> + int (*set_eee) (struct net_device *, struct ethtool_value *); > > [...] > > > > Why are there new operations as well as a new field to ethtool_cmd? > > > > Is the value a boolean or a bitmask? > > the former. > These are for enabling/disabling the EEE and also to get the status. > > > Since EEE is an auto-negotiated feature, why aren't you defining flags > > for the supported/advertising/lp_advertising fields? > > I added these because the stmmac needs to have a timer to enter in LPI > tx mode. So this can be stopped by using ethtool. > I didn't define flags for supported/advertising/lp_advertising because > these are fixed in the RO MMD register and it makes no sense to modify > them (hmm, I think :-)). It is certainly necessary to distinguish: a. PHY is advertising EEE from b. Link partner is advertising EEE or c. EEE will be used (= a && b) I haven't had anything to do with PHY drivers for a while but it looks like you should be able to map MDIO register 3.20 to supported flags, 7.60 to advertising and 7.61 to lp_advertising. Even if your particular device doesn't support these MDIO registers, other devices will. So I think you should define the flags and then if you don't have that fine control you should check in your set_settings implementation that the advertising value either has all the EEE mode flags clear or has all the modes the hardware supports set. Ben.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index da5b2de..fdea3c7 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -43,6 +43,7 @@ struct ethtool_cmd { __u8 eth_tp_mdix; __u8 reserved2; __u32 lp_advertising; /* Features the link partner advertises */ + __u32 eee; /* Energy-Efficient Etehrnet */ __u32 reserved[2]; }; @@ -874,6 +875,8 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) * and flag of the device. * @get_dump_data: Get dump data. * @set_dump: Set dump specific flags to the device. + * @get_eee: Get Energy-Efficient Ethernet (EEE) supported and status. + * @set_eee: Set EEE status (enable/disable). * * All operations are optional (i.e. the function pointer may be set * to %NULL) and callers must take this into account. Callers must @@ -937,6 +940,8 @@ struct ethtool_ops { struct ethtool_dump *, void *); int (*set_dump)(struct net_device *, struct ethtool_dump *); + int (*get_eee) (struct net_device *, struct ethtool_value *); + int (*set_eee) (struct net_device *, struct ethtool_value *); }; #endif /* __KERNEL__ */ @@ -1010,6 +1015,8 @@ 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_GEEE 0x00000041 /* Get EEE */ +#define ETHTOOL_SEEE 0x00000042 /* Set EEE */ /* compatibility with older code */ #define SPARC_ETH_GSET ETHTOOL_GSET diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 3f79db1..088b621 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -714,6 +714,32 @@ 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_value 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_value edata; + + if (!dev->ethtool_ops->set_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) @@ -1368,6 +1394,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;
This patch adds two new functions to detect if Energy-Efficient Ethernet (EEE) is supported and the way to enable/disable it. Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com> --- include/linux/ethtool.h | 7 +++++++ net/core/ethtool.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 0 deletions(-)