| Message ID | 1333704559-11251-2-git-send-email-peppe.cavallaro@st.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2012-04-06 at 11:29 +0200, 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. > > As Ben said, 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) > > The logic behind this code, is that .get_eee will pass > to the user-space if the EEE is actually used and available (so point c). > The .set_eee should used to force the MAC to disable/enable the EEE (if > actually supported by MAC+PHY). [...] What I meant is that userland should be able to find out (a), and, *separately*, either (b) or (c). That is, there must be at least 2 separate flags for this. In fact, I explicitly requested you define supported/advertising/lp_advertising bitmasks for EEE link modes just like we have for autonegotiation. But you've made no substantive changes in response to my review, aside from dropping the added field in ethtool_cmd. What you're submitting just isn't good enough for a generic interface, as the ethtool API is supposed to be. It's not even a good interface to your driver. Ben.
On 4/13/2012 12:26 AM, Ben Hutchings wrote: > On Fri, 2012-04-06 at 11:29 +0200, 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. >> >> As Ben said, 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) >> >> The logic behind this code, is that .get_eee will pass >> to the user-space if the EEE is actually used and available (so point c). >> The .set_eee should used to force the MAC to disable/enable the EEE (if >> actually supported by MAC+PHY). > [...] > > What I meant is that userland should be able to find out (a), and, > *separately*, either (b) or (c). That is, there must be at least 2 > separate flags for this. In fact, I explicitly requested you define > supported/advertising/lp_advertising bitmasks for EEE link modes just > like we have for autonegotiation. But you've made no substantive > changes in response to my review, aside from dropping the added field in > ethtool_cmd. Sorry Ben but I believed that (c) was enough. > What you're submitting just isn't good enough for a generic interface, > as the ethtool API is supposed to be. It's not even a good interface to > your driver. yes! I'll rework this and provide new patches asap. 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
Hello Ben, On 4/16/2012 7:41 AM, Giuseppe CAVALLARO wrote: [snip] >> What I meant is that userland should be able to find out (a), and, >> *separately*, either (b) or (c). That is, there must be at least 2 >> separate flags for this. In fact, I explicitly requested you define >> supported/advertising/lp_advertising bitmasks for EEE link modes just >> like we have for autonegotiation. But you've made no substantive >> changes in response to my review, aside from dropping the added field in >> ethtool_cmd. > > Sorry Ben but I believed that (c) was enough. > >> What you're submitting just isn't good enough for a generic interface, >> as the ethtool API is supposed to be. It's not even a good interface to >> your driver. > > yes! I'll rework this and provide new patches asap. sorry if I disturb you but I want to be sure to avoid to forget something else in the next EEE patches (avoiding to continuously disturb you). I'm changing the code for getting/setting the EEE capability and trying to follow your suggestions. The "get" will show the following things; this is a bit different of the points "a" "b" and "c" we had discussed. Maybe, this could also be a more complete (*) . The ethtool (see output below as example) could report the phy (supported/advertised/lp_advertised) and mac eee capabilities separately. The "set" will be useful for some eth devices (like the stmmac) that can stop/enable internally the eee capability (at mac level). What do you think? Regards Peppe ---- # ./ethtool eth0 Settings for eth0: [snip] Current message level: 0x0000003f (63) drv probe link timer ifdown ifup Link detected: yes Energy-Efficient Ethernet: ------------------------- MAC supports: yes |-> related to MAC side | phy supports modes: ... |-> from MMD 3.20 | phy advertising modes: ... |-> from MMD 7.60 | LP advertising modes: ... |-> from MMD 7.61 | -------------------------- (*) PS. The "..." above means that we can actually dump: 100BASE-TX EEE etc for each advertising modes and also for phy support (reg 3.20). > -- > 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 > -- 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
Hi Peppe, > The "set" will be useful for some eth devices (like the stmmac) that can > stop/enable internally the eee capability (at mac level). If you're already implementing this interface, don't you think it might be prudent to create an implementation that can do more than enable/disable the interface? I think users would like a method for configuring some of the EEE's variables, mainly controlling the timers affecting the generation of an LPI request, as such control might have a direct consequence on the effectiveness of their energy savings (less time for generation ==> better energy savings, with a possible latency penalty). 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 Thu, 2012-04-19 at 16:48 +0300, Yuval Mintz wrote: > Hi Peppe, > > > The "set" will be useful for some eth devices (like the stmmac) that can > > stop/enable internally the eee capability (at mac level). > > If you're already implementing this interface, don't you think it might be > prudent to create an implementation that can do more than enable/disable > the interface? [...] It's not necessary for anyone to *implement* all of this now, but the interface should certainly cover any settings that users may reasonably want to read and configure. As with most ethtool 'set' operations, any implementation (driver) can disallow changing any or all settings (-EOPNOTSUPP or -EINVAL) if it's difficult or impossible to implement them. Ben.
On Thu, 2012-04-19 at 14:58 +0200, Giuseppe CAVALLARO wrote: > Hello Ben, > > On 4/16/2012 7:41 AM, Giuseppe CAVALLARO wrote: > [snip] > >> What I meant is that userland should be able to find out (a), and, > >> *separately*, either (b) or (c). That is, there must be at least 2 > >> separate flags for this. In fact, I explicitly requested you define > >> supported/advertising/lp_advertising bitmasks for EEE link modes just > >> like we have for autonegotiation. But you've made no substantive > >> changes in response to my review, aside from dropping the added field in > >> ethtool_cmd. > > > > Sorry Ben but I believed that (c) was enough. > > > >> What you're submitting just isn't good enough for a generic interface, > >> as the ethtool API is supposed to be. It's not even a good interface to > >> your driver. > > > > yes! I'll rework this and provide new patches asap. > > sorry if I disturb you but I want to be sure to avoid to forget > something else in the next EEE patches (avoiding to continuously disturb > you). > > I'm changing the code for getting/setting the EEE capability and trying > to follow your suggestions. > > The "get" will show the following things; this is a bit different of the > points "a" "b" and "c" we had discussed. Maybe, this could also be a > more complete (*) . > The ethtool (see output below as example) could report the phy > (supported/advertised/lp_advertised) and mac eee capabilities separately. Sounds reasonable. > The "set" will be useful for some eth devices (like the stmmac) that can > stop/enable internally the eee capability (at mac level). I don't know much about EEE, but shouldn't the driver take care of configuring the MAC for this whenever the PHY is set to advertise EEE capability? > What do you think? > > Regards > Peppe > > ---- > > # ./ethtool eth0 > Settings for eth0: > > [snip] > > Current message level: 0x0000003f (63) > drv probe link timer ifdown ifup > Link detected: yes > Energy-Efficient Ethernet: ------------------------- > MAC supports: yes |-> related to MAC side | > phy supports modes: ... |-> from MMD 3.20 | > phy advertising modes: ... |-> from MMD 7.60 | > LP advertising modes: ... |-> from MMD 7.61 | > -------------------------- > (*) > PS. The "..." above means that we can actually dump: 100BASE-TX EEE etc > for each advertising modes and also for phy support (reg 3.20). Yes, that's the sort of information I would expect to see (but try to be consistent with the wording used for AN). The EEE advertising mask presumably should be changeable similarly to the AN advertising mask ('ethtool -s <devname> eeeadv <mask>'). But I don't know how the two advertising masks interact. Is one supposed to be a subset of the other? Currently ethtool automatically changes the AN advertising mask in response to a speed/duplex change; should it also try to change the EEE advertising mask? Ben.
Hello Ben On 4/19/2012 5:30 PM, Ben Hutchings wrote: [snip] >> I'm changing the code for getting/setting the EEE capability and trying >> to follow your suggestions. >> >> The "get" will show the following things; this is a bit different of the >> points "a" "b" and "c" we had discussed. Maybe, this could also be a >> more complete (*) . >> The ethtool (see output below as example) could report the phy >> (supported/advertised/lp_advertised) and mac eee capabilities separately. > > Sounds reasonable. > >> The "set" will be useful for some eth devices (like the stmmac) that can >> stop/enable internally the eee capability (at mac level). > > I don't know much about EEE, but shouldn't the driver take care of > configuring the MAC for this whenever the PHY is set to advertise EEE > capability? Yes indeed this can be done at driver level. So could I definitely remove it from ethtool? What do you suggest? In case of the stmmac I could add a specific driver option via sys to enable/disable the eee and set timer. >> [snip] >> >> Current message level: 0x0000003f (63) >> drv probe link timer ifdown ifup >> Link detected: yes >> Energy-Efficient Ethernet: ------------------------- >> MAC supports: yes |-> related to MAC side | >> phy supports modes: ... |-> from MMD 3.20 | >> phy advertising modes: ... |-> from MMD 7.60 | >> LP advertising modes: ... |-> from MMD 7.61 | >> -------------------------- >> (*) >> PS. The "..." above means that we can actually dump: 100BASE-TX EEE etc >> for each advertising modes and also for phy support (reg 3.20). > > Yes, that's the sort of information I would expect to see (but try to be > consistent with the wording used for AN).: e.g. SUPPORTED_100baseT_EEE ... ADVERTISED_<...> > The EEE advertising mask presumably should be changeable similarly to > the AN advertising mask ('ethtool -s <devname> eeeadv <mask>'). But I > don't know how the two advertising masks interact. Is one supposed to > be a subset of the other? Currently ethtool automatically changes the > AN advertising mask in response to a speed/duplex change; should it also > try to change the EEE advertising mask? I've just verified the IEEE (Table 45–150a—EEE advertisement register (Register 7.60) bit definitions) and sorry for my delay in reply but I was in trouble because looking at the registers for the phy (I am using) the reg 7.60 was in RO and I couldn't understand how to set the mask. I confirm that the Adv reg from the std is R/W and the mask as you suggest could be set according to the speed. The EEE should work on duplex mode only. I wonder so if if the final patch I should have no new option for the ethtool command and EEE info are directly passed from the kernel like speed and duplex when call get_settings. 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 Thu, 2012-04-26 at 09:48 +0200, Giuseppe CAVALLARO wrote: > Hello Ben > > On 4/19/2012 5:30 PM, Ben Hutchings wrote: > [snip] > >> I'm changing the code for getting/setting the EEE capability and trying > >> to follow your suggestions. > >> > >> The "get" will show the following things; this is a bit different of the > >> points "a" "b" and "c" we had discussed. Maybe, this could also be a > >> more complete (*) . > >> The ethtool (see output below as example) could report the phy > >> (supported/advertised/lp_advertised) and mac eee capabilities separately. > > > > Sounds reasonable. > > > >> The "set" will be useful for some eth devices (like the stmmac) that can > >> stop/enable internally the eee capability (at mac level). > > > > I don't know much about EEE, but shouldn't the driver take care of > > configuring the MAC for this whenever the PHY is set to advertise EEE > > capability? > > Yes indeed this can be done at driver level. So could I definitely > remove it from ethtool? What do you suggest? > > In case of the stmmac I could add a specific driver option via sys to > enable/disable the eee and set timer. Generally, ethtool doesn't distinguish MAC and PHY settings because they have to be configured consistently for the device to do anything useful. If there is some good use for enabling EEE in the MAC and not the PHY, or vice versa, then this should be exposed in the ethtool interface. But if not then I don't believe it needs to be in either an ethtool or a driver-specific interface. > >> [snip] > >> > >> Current message level: 0x0000003f (63) > >> drv probe link timer ifdown ifup > >> Link detected: yes > >> Energy-Efficient Ethernet: ------------------------- > >> MAC supports: yes |-> related to MAC side | > >> phy supports modes: ... |-> from MMD 3.20 | > >> phy advertising modes: ... |-> from MMD 7.60 | > >> LP advertising modes: ... |-> from MMD 7.61 | > >> -------------------------- > >> (*) > >> PS. The "..." above means that we can actually dump: 100BASE-TX EEE etc > >> for each advertising modes and also for phy support (reg 3.20). > > > > Yes, that's the sort of information I would expect to see (but try to be > > consistent with the wording used for AN).: > > e.g. SUPPORTED_100baseT_EEE ... ADVERTISED_<...> I meant the wording used in the ethtool output: 'supported', 'advertised', 'link partner advertised' rather than 'phy supports', 'phy advertising', 'LP advertising'. > > The EEE advertising mask presumably should be changeable similarly to > > the AN advertising mask ('ethtool -s <devname> eeeadv <mask>'). But I > > don't know how the two advertising masks interact. Is one supposed to > > be a subset of the other? Currently ethtool automatically changes the > > AN advertising mask in response to a speed/duplex change; should it also > > try to change the EEE advertising mask? > > I've just verified the IEEE (Table 45–150a—EEE advertisement register > (Register 7.60) bit definitions) and sorry for my delay in reply but I > was in trouble because looking at the registers for the phy (I am using) > the reg 7.60 was in RO and I couldn't understand how to set the mask. > I confirm that the Adv reg from the std is R/W and the mask as you > suggest could be set according to the speed. > The EEE should work on duplex mode only. > > I wonder so if if the final patch I should have no new option for the > ethtool command and EEE info are directly passed from the kernel like > speed and duplex when call get_settings. Are you suggesting to define EEE mode flags in the existing supported, advertising and lp_advertising masks? Ben.
On 4/26/2012 7:17 PM, Ben Hutchings wrote: > On Thu, 2012-04-26 at 09:48 +0200, Giuseppe CAVALLARO wrote: >> Hello Ben >> >> On 4/19/2012 5:30 PM, Ben Hutchings wrote: >> [snip] >>>> I'm changing the code for getting/setting the EEE capability and trying >>>> to follow your suggestions. >>>> >>>> The "get" will show the following things; this is a bit different of the >>>> points "a" "b" and "c" we had discussed. Maybe, this could also be a >>>> more complete (*) . >>>> The ethtool (see output below as example) could report the phy >>>> (supported/advertised/lp_advertised) and mac eee capabilities separately. >>> >>> Sounds reasonable. >>> >>>> The "set" will be useful for some eth devices (like the stmmac) that can >>>> stop/enable internally the eee capability (at mac level). >>> >>> I don't know much about EEE, but shouldn't the driver take care of >>> configuring the MAC for this whenever the PHY is set to advertise EEE >>> capability? >> >> Yes indeed this can be done at driver level. So could I definitely >> remove it from ethtool? What do you suggest? >> >> In case of the stmmac I could add a specific driver option via sys to >> enable/disable the eee and set timer. > > Generally, ethtool doesn't distinguish MAC and PHY settings because they > have to be configured consistently for the device to do anything useful. > If there is some good use for enabling EEE in the MAC and not the PHY, > or vice versa, then this should be exposed in the ethtool interface. > But if not then I don't believe it needs to be in either an ethtool or a > driver-specific interface. Thanks Ben for this clarification: in case of the stmmac the option is useful to stop a timer to enter in lpi state for the tx. So it's worth having that and from ethtool. > >>>> [snip] >>>> >>>> Current message level: 0x0000003f (63) >>>> drv probe link timer ifdown ifup >>>> Link detected: yes >>>> Energy-Efficient Ethernet: ------------------------- >>>> MAC supports: yes |-> related to MAC side | >>>> phy supports modes: ... |-> from MMD 3.20 | >>>> phy advertising modes: ... |-> from MMD 7.60 | >>>> LP advertising modes: ... |-> from MMD 7.61 | >>>> -------------------------- >>>> (*) >>>> PS. The "..." above means that we can actually dump: 100BASE-TX EEE etc >>>> for each advertising modes and also for phy support (reg 3.20). >>> >>> Yes, that's the sort of information I would expect to see (but try to be >>> consistent with the wording used for AN).: >> >> e.g. SUPPORTED_100baseT_EEE ... ADVERTISED_<...> > > I meant the wording used in the ethtool output: 'supported', > 'advertised', 'link partner advertised' rather than 'phy supports', > 'phy advertising', 'LP advertising'. ok :-) > >>> The EEE advertising mask presumably should be changeable similarly to >>> the AN advertising mask ('ethtool -s <devname> eeeadv <mask>'). But I >>> don't know how the two advertising masks interact. Is one supposed to >>> be a subset of the other? Currently ethtool automatically changes the >>> AN advertising mask in response to a speed/duplex change; should it also >>> try to change the EEE advertising mask? >> >> I've just verified the IEEE (Table 45–150a—EEE advertisement register >> (Register 7.60) bit definitions) and sorry for my delay in reply but I >> was in trouble because looking at the registers for the phy (I am using) >> the reg 7.60 was in RO and I couldn't understand how to set the mask. >> I confirm that the Adv reg from the std is R/W and the mask as you >> suggest could be set according to the speed. >> The EEE should work on duplex mode only. >> >> I wonder so if if the final patch I should have no new option for the >> ethtool command and EEE info are directly passed from the kernel like >> speed and duplex when call get_settings. > > Are you suggesting to define EEE mode flags in the existing supported, > advertising and lp_advertising masks? Yes but I was wrong, I can use the existing flags. 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 04/27/2012 05:11 PM, Giuseppe CAVALLARO wrote: > On 4/26/2012 7:17 PM, Ben Hutchings wrote: >> On Thu, 2012-04-26 at 09:48 +0200, Giuseppe CAVALLARO wrote: >>> Hello Ben >>> >>> On 4/19/2012 5:30 PM, Ben Hutchings wrote: >>> [snip] >>>>> I'm changing the code for getting/setting the EEE capability and trying >>>>> to follow your suggestions. >>>>> >>>>> The "get" will show the following things; this is a bit different of the >>>>> points "a" "b" and "c" we had discussed. Maybe, this could also be a >>>>> more complete (*) . >>>>> The ethtool (see output below as example) could report the phy >>>>> (supported/advertised/lp_advertised) and mac eee capabilities separately. >>>> Sounds reasonable. >>>> >>>>> The "set" will be useful for some eth devices (like the stmmac) that can >>>>> stop/enable internally the eee capability (at mac level). >>>> I don't know much about EEE, but shouldn't the driver take care of >>>> configuring the MAC for this whenever the PHY is set to advertise EEE >>>> capability? >>> Yes indeed this can be done at driver level. So could I definitely >>> remove it from ethtool? What do you suggest? >>> >>> In case of the stmmac I could add a specific driver option via sys to >>> enable/disable the eee and set timer. >> Generally, ethtool doesn't distinguish MAC and PHY settings because they >> have to be configured consistently for the device to do anything useful. >> If there is some good use for enabling EEE in the MAC and not the PHY, >> or vice versa, then this should be exposed in the ethtool interface. >> But if not then I don't believe it needs to be in either an ethtool or a >> driver-specific interface. > Thanks Ben for this clarification: in case of the stmmac the option is > useful to stop a timer to enter in lpi state for the tx. > So it's worth having that and from ethtool. How will a user turn off EEE support using this implementation? Are you suggesting a "set" that works similarly to the control of the pause parameters - that is, a user could either shutdown EEE or only Tx, which will mean to the driver "don't enter Tx LPI mode"? Keep in mind that if later an interface controlling the LPI timers would be added (as a measure of user control to the power saving vs. latency issue), it could make this 'partial' closure interface redundant. Perhaps "set" should only turn the EEE feature on/off entirely (adv. them or not, since clearly the link will have to be re-established afterwards), and we should have a different function that prevents entry into LPI mode in Tx - one whose functionality could later on be extended. Regards, 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 Sun, 2012-04-29 at 12:20 +0300, Yuval Mintz wrote: > On 04/27/2012 05:11 PM, Giuseppe CAVALLARO wrote: > > > On 4/26/2012 7:17 PM, Ben Hutchings wrote: > >> On Thu, 2012-04-26 at 09:48 +0200, Giuseppe CAVALLARO wrote: > >>> Hello Ben > >>> > >>> On 4/19/2012 5:30 PM, Ben Hutchings wrote: > >>> [snip] > >>>>> I'm changing the code for getting/setting the EEE capability and trying > >>>>> to follow your suggestions. > >>>>> > >>>>> The "get" will show the following things; this is a bit different of the > >>>>> points "a" "b" and "c" we had discussed. Maybe, this could also be a > >>>>> more complete (*) . > >>>>> The ethtool (see output below as example) could report the phy > >>>>> (supported/advertised/lp_advertised) and mac eee capabilities separately. > >>>> Sounds reasonable. > >>>> > >>>>> The "set" will be useful for some eth devices (like the stmmac) that can > >>>>> stop/enable internally the eee capability (at mac level). > >>>> I don't know much about EEE, but shouldn't the driver take care of > >>>> configuring the MAC for this whenever the PHY is set to advertise EEE > >>>> capability? > >>> Yes indeed this can be done at driver level. So could I definitely > >>> remove it from ethtool? What do you suggest? > >>> > >>> In case of the stmmac I could add a specific driver option via sys to > >>> enable/disable the eee and set timer. > >> Generally, ethtool doesn't distinguish MAC and PHY settings because they > >> have to be configured consistently for the device to do anything useful. > >> If there is some good use for enabling EEE in the MAC and not the PHY, > >> or vice versa, then this should be exposed in the ethtool interface. > >> But if not then I don't believe it needs to be in either an ethtool or a > >> driver-specific interface. > > Thanks Ben for this clarification: in case of the stmmac the option is > > useful to stop a timer to enter in lpi state for the tx. > > So it's worth having that and from ethtool. I think I finally get it. If we negotiate a 100BASE-TX link (or one of the various backplane modes) with EEE enabled, we allow the link partner to assert LPI but we might still not want to assert it in the transmit direction. Right? (Whereas for 1000BASE-T and 10GBASE-T this would be useless, since both sides must assert LPI before any transition can happen.) > How will a user turn off EEE support using this implementation? At the ethtool API level this would be done by clearing the EEE advertising mask. At the command-line level there could be a shortcut for this, just as you can use 'autoneg on' and 'autoneg off' rather than specifying a mask of link modes. > Are you suggesting a "set" that works similarly to the control of the pause > parameters - that is, a user could either shutdown EEE or only Tx, which > will mean to the driver "don't enter Tx LPI mode"? > > Keep in mind that if later an interface controlling the LPI timers would be > added (as a measure of user control to the power saving vs. latency issue), > it could make this 'partial' closure interface redundant. > > Perhaps "set" should only turn the EEE feature on/off entirely (adv. them or > not, since clearly the link will have to be re-established afterwards), and > we should have a different function that prevents entry into LPI mode in Tx > - one whose functionality could later on be extended. It sounds like this might as well be included, even if not all drivers/hardware would allow the values to be changed. So the command structure would have at least: 1. EEE link mode supported flags (get-only) 2. EEE link mode advertising flags (get/set) 3. Ditto for link partner (get-only) 4. TX LPI enable flag (get/set) 5. TX LPI timer values (get/set but driver may reject changes) But if it's not yet clear exactly what timer parameters will be useful, we could leave some reserved space and then later define them along with flags to indicate whether the driver understands them. Ben.
Hello Ben On 4/29/2012 11:56 PM, Ben Hutchings wrote: > On Sun, 2012-04-29 at 12:20 +0300, Yuval Mintz wrote: >> On 04/27/2012 05:11 PM, Giuseppe CAVALLARO wrote: >> >>> On 4/26/2012 7:17 PM, Ben Hutchings wrote: >>>> On Thu, 2012-04-26 at 09:48 +0200, Giuseppe CAVALLARO wrote: >>>>> Hello Ben >>>>> >>>>> On 4/19/2012 5:30 PM, Ben Hutchings wrote: >>>>> [snip] >>>>>>> I'm changing the code for getting/setting the EEE capability and trying >>>>>>> to follow your suggestions. >>>>>>> >>>>>>> The "get" will show the following things; this is a bit different of the >>>>>>> points "a" "b" and "c" we had discussed. Maybe, this could also be a >>>>>>> more complete (*) . >>>>>>> The ethtool (see output below as example) could report the phy >>>>>>> (supported/advertised/lp_advertised) and mac eee capabilities separately. >>>>>> Sounds reasonable. >>>>>> >>>>>>> The "set" will be useful for some eth devices (like the stmmac) that can >>>>>>> stop/enable internally the eee capability (at mac level). >>>>>> I don't know much about EEE, but shouldn't the driver take care of >>>>>> configuring the MAC for this whenever the PHY is set to advertise EEE >>>>>> capability? >>>>> Yes indeed this can be done at driver level. So could I definitely >>>>> remove it from ethtool? What do you suggest? >>>>> >>>>> In case of the stmmac I could add a specific driver option via sys to >>>>> enable/disable the eee and set timer. >>>> Generally, ethtool doesn't distinguish MAC and PHY settings because they >>>> have to be configured consistently for the device to do anything useful. >>>> If there is some good use for enabling EEE in the MAC and not the PHY, >>>> or vice versa, then this should be exposed in the ethtool interface. >>>> But if not then I don't believe it needs to be in either an ethtool or a >>>> driver-specific interface. >>> Thanks Ben for this clarification: in case of the stmmac the option is >>> useful to stop a timer to enter in lpi state for the tx. >>> So it's worth having that and from ethtool. > > I think I finally get it. If we negotiate a 100BASE-TX link (or one of > the various backplane modes) with EEE enabled, we allow the link partner > to assert LPI but we might still not want to assert it in the transmit > direction. Right? (Whereas for 1000BASE-T and 10GBASE-T this would be > useless, since both sides must assert LPI before any transition can > happen.) > >> How will a user turn off EEE support using this implementation? > > At the ethtool API level this would be done by clearing the EEE > advertising mask. At the command-line level there could be a shortcut > for this, just as you can use 'autoneg on' and 'autoneg off' rather than > specifying a mask of link modes. > >> Are you suggesting a "set" that works similarly to the control of the pause >> parameters - that is, a user could either shutdown EEE or only Tx, which >> will mean to the driver "don't enter Tx LPI mode"? >> >> Keep in mind that if later an interface controlling the LPI timers would be >> added (as a measure of user control to the power saving vs. latency issue), >> it could make this 'partial' closure interface redundant. >> >> Perhaps "set" should only turn the EEE feature on/off entirely (adv. them or >> not, since clearly the link will have to be re-established afterwards), and >> we should have a different function that prevents entry into LPI mode in Tx >> - one whose functionality could later on be extended. > > It sounds like this might as well be included, even if not all > drivers/hardware would allow the values to be changed. So the command > structure would have at least: > > 1. EEE link mode supported flags (get-only) > 2. EEE link mode advertising flags (get/set) > 3. Ditto for link partner (get-only) > 4. TX LPI enable flag (get/set) > 5. TX LPI timer values (get/set but driver may reject changes) Ok I'll try to rework all following the points above. Just a note for the timer and point 5 below. > But if it's not yet clear exactly what timer parameters will be useful, > we could leave some reserved space and then later define them along with > flags to indicate whether the driver understands them. I can use and test the LPI timer parameters that I intends, in case of the stmmac d.d., the values added in a mac core register. These two timers: 1) specify the minimum time for which the link-status from the PHY should be up. The default value 1 sec as defined in the IEEE standard. 2) specify the minimum time for which the MAC waits after it has stopped transmitting the LPI pattern to the PHY 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
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 560a247..d0150c8 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -920,6 +920,8 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) * @get_ts_info: Get the time stamping and PTP hardware clock capabilities. * Drivers supporting transmit time stamps in software should set this to * ethtool_op_get_ts_info(). + * @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 @@ -984,6 +986,8 @@ struct ethtool_ops { int (*set_dump)(struct net_device *, struct ethtool_dump *); int (*get_ts_info)(struct net_device *, struct ethtool_ts_info *); + int (*get_eee) (struct net_device *, struct ethtool_value *); + int (*set_eee) (struct net_device *, struct ethtool_value *); }; #endif /* __KERNEL__ */ @@ -1058,6 +1062,8 @@ struct ethtool_ops { #define ETHTOOL_GET_DUMP_FLAG 0x0000003f /* Get dump settings */ #define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */ #define ETHTOOL_GET_TS_INFO 0x00000041 /* Get time stamping and PHC info */ +#define ETHTOOL_GEEE 0x00000042 /* Get EEE */ +#define ETHTOOL_SEEE 0x00000043 /* 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 beacdd9..c2374f9 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -729,6 +729,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) @@ -1420,6 +1446,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;