| Message ID | 20210818155202.1278177-2-idosch@idosch.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: Add ability to control transceiver modules' power mode | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
On Wed, 18 Aug 2021 18:51:57 +0300 Ido Schimmel wrote: > +MODULE_SET > +========== > + > +Sets transceiver module parameters. > + > +Request contents: > + > + ====================================== ====== ========================== > + ``ETHTOOL_A_MODULE_HEADER`` nested request header > + ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` u8 power mode policy > + ====================================== ====== ========================== > + > +When set, the optional ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` attribute is used > +to set the transceiver module power policy enforced by the host. Possible > +values are: > + > +.. kernel-doc:: include/uapi/linux/ethtool.h > + :identifiers: ethtool_module_power_mode_policy > + > +For SFF-8636 modules, low power mode is forced by the host according to table > +6-10 in revision 2.10a of the specification. > + > +For CMIS modules, low power mode is forced by the host according to table 6-12 > +in revision 5.0 of the specification. > + > +To avoid changes to the operational state of the device, power mode policy can > +only be set when the device is administratively down. Would you mind explaining why? > +/** > + * enum ethtool_module_power_mode_policy - plug-in module power mode policy > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_LOW: Module is always in low power mode. Did you have a use case for this one or is it for completeness? Seems like user can just bring the port down if they want no carrier? My understanding was you primarily wanted the latter two, and those can be freely changed when netdev is running, right? > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH: Module is always in high power mode. > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP: Module is transitioned by the > + * host to high power mode when the first port using it is put > + * administratively up and to low power mode when the last port using it > + * is put administratively down. s/HIGH_ON_UP/AUTO/ ? high on up == low on down, right, seems arbitrary to pick one over the other
On Wed, Aug 18, 2021 at 03:32:41PM -0700, Jakub Kicinski wrote: > On Wed, 18 Aug 2021 18:51:57 +0300 Ido Schimmel wrote: > > +MODULE_SET > > +========== > > + > > +Sets transceiver module parameters. > > + > > +Request contents: > > + > > + ====================================== ====== ========================== > > + ``ETHTOOL_A_MODULE_HEADER`` nested request header > > + ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` u8 power mode policy > > + ====================================== ====== ========================== > > + > > +When set, the optional ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` attribute is used > > +to set the transceiver module power policy enforced by the host. Possible > > +values are: > > + > > +.. kernel-doc:: include/uapi/linux/ethtool.h > > + :identifiers: ethtool_module_power_mode_policy > > + > > +For SFF-8636 modules, low power mode is forced by the host according to table > > +6-10 in revision 2.10a of the specification. > > + > > +For CMIS modules, low power mode is forced by the host according to table 6-12 > > +in revision 5.0 of the specification. > > + > > +To avoid changes to the operational state of the device, power mode policy can > > +only be set when the device is administratively down. > > Would you mind explaining why? Part of the issue is we have two different sorts of policy mixed together. ETHTOOL_MODULE_POWER_MODE_POLICY_LOW and ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH change the state now. This could be a surprise to a user when there link disappears for the ETHTOOL_MODULE_POWER_MODE_POLICY_LOW case, when the interface is admin up. ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP however follows the state of the interface. So there should not be any surprises. I actually think ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP should be allowed at any time, just to make it easier to use. > > +/** > > + * enum ethtool_module_power_mode_policy - plug-in module power mode policy > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_LOW: Module is always in low power mode. > > Did you have a use case for this one or is it for completeness? Seems > like user can just bring the port down if they want no carrier? My > understanding was you primarily wanted the latter two, and those can > be freely changed when netdev is running, right? > > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH: Module is always in high power mode. > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP: Module is transitioned by the > > + * host to high power mode when the first port using it is put > > + * administratively up and to low power mode when the last port using it > > + * is put administratively down. > > s/HIGH_ON_UP/AUTO/ ? > high on up == low on down, right, seems arbitrary to pick one over the > other Should we also document what the default is? Seems like ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP is the generic network interface default, so maybe it should also be the default for SFPs? Andrew
On Wed, Aug 18, 2021 at 03:32:41PM -0700, Jakub Kicinski wrote: > On Wed, 18 Aug 2021 18:51:57 +0300 Ido Schimmel wrote: > > +MODULE_SET > > +========== > > + > > +Sets transceiver module parameters. > > + > > +Request contents: > > + > > + ====================================== ====== ========================== > > + ``ETHTOOL_A_MODULE_HEADER`` nested request header > > + ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` u8 power mode policy > > + ====================================== ====== ========================== > > + > > +When set, the optional ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` attribute is used > > +to set the transceiver module power policy enforced by the host. Possible > > +values are: > > + > > +.. kernel-doc:: include/uapi/linux/ethtool.h > > + :identifiers: ethtool_module_power_mode_policy > > + > > +For SFF-8636 modules, low power mode is forced by the host according to table > > +6-10 in revision 2.10a of the specification. > > + > > +For CMIS modules, low power mode is forced by the host according to table 6-12 > > +in revision 5.0 of the specification. > > + > > +To avoid changes to the operational state of the device, power mode policy can > > +only be set when the device is administratively down. > > Would you mind explaining why? Yes, it is more restrictive than it should be. The check can be relaxed to only disallow transition to low power mode when the device is administratively up. > > > +/** > > + * enum ethtool_module_power_mode_policy - plug-in module power mode policy > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_LOW: Module is always in low power mode. > > Did you have a use case for this one or is it for completeness? Seems > like user can just bring the port down if they want no carrier? My > understanding was you primarily wanted the latter two, and those can > be freely changed when netdev is running, right? In all the implementations I could find (3), the user interface is high/low (on/off). The proposed interface is more flexible as it contains both the 'high' / 'low' policies in addition to the more user friendly 'high-on-up' ('auto') policy. Given that keeping the 'low' policy does not complicate the implementation / maintenance and that it provides users with a similar interface to what they are used to from other implementations, I would like to keep it in addition to the other two policies. > > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH: Module is always in high power mode. > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP: Module is transitioned by the > > + * host to high power mode when the first port using it is put > > + * administratively up and to low power mode when the last port using it > > + * is put administratively down. > > s/HIGH_ON_UP/AUTO/ ? OK > high on up == low on down, right, seems arbitrary to pick one over the > other
On Thu, Aug 19, 2021 at 12:55:43AM +0200, Andrew Lunn wrote: > On Wed, Aug 18, 2021 at 03:32:41PM -0700, Jakub Kicinski wrote: > > On Wed, 18 Aug 2021 18:51:57 +0300 Ido Schimmel wrote: > > > +MODULE_SET > > > +========== > > > + > > > +Sets transceiver module parameters. > > > + > > > +Request contents: > > > + > > > + ====================================== ====== ========================== > > > + ``ETHTOOL_A_MODULE_HEADER`` nested request header > > > + ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` u8 power mode policy > > > + ====================================== ====== ========================== > > > + > > > +When set, the optional ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` attribute is used > > > +to set the transceiver module power policy enforced by the host. Possible > > > +values are: > > > + > > > +.. kernel-doc:: include/uapi/linux/ethtool.h > > > + :identifiers: ethtool_module_power_mode_policy > > > + > > > +For SFF-8636 modules, low power mode is forced by the host according to table > > > +6-10 in revision 2.10a of the specification. > > > + > > > +For CMIS modules, low power mode is forced by the host according to table 6-12 > > > +in revision 5.0 of the specification. > > > + > > > +To avoid changes to the operational state of the device, power mode policy can > > > +only be set when the device is administratively down. > > > > Would you mind explaining why? > > Part of the issue is we have two different sorts of policy mixed > together. > > ETHTOOL_MODULE_POWER_MODE_POLICY_LOW and > ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH change the state now. This could > be a surprise to a user when there link disappears for the > ETHTOOL_MODULE_POWER_MODE_POLICY_LOW case, when the interface is admin up. > > ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP however follows the state > of the interface. So there should not be any surprises. > > I actually think ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP should be > allowed at any time, just to make it easier to use. Yes > > > > +/** > > > + * enum ethtool_module_power_mode_policy - plug-in module power mode policy > > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_LOW: Module is always in low power mode. > > > > Did you have a use case for this one or is it for completeness? Seems > > like user can just bring the port down if they want no carrier? My > > understanding was you primarily wanted the latter two, and those can > > be freely changed when netdev is running, right? > > > > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH: Module is always in high power mode. > > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP: Module is transitioned by the > > > + * host to high power mode when the first port using it is put > > > + * administratively up and to low power mode when the last port using it > > > + * is put administratively down. > > > > s/HIGH_ON_UP/AUTO/ ? > > high on up == low on down, right, seems arbitrary to pick one over the > > other > > Should we also document what the default is? Seems like > ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP is the generic network > interface default, so maybe it should also be the default for SFPs? I will add a note in Documentation/networking/ethtool-netlink.rst that the default power mode policy is driver-dependent (can be queried) and that it can either be 'high' or 'auto'. > > Andrew
> > Should we also document what the default is? Seems like > > ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP is the generic network > > interface default, so maybe it should also be the default for SFPs? > > I will add a note in Documentation/networking/ethtool-netlink.rst that > the default power mode policy is driver-dependent (can be queried) and > that it can either be 'high' or 'auto'. Hi Ido That is kind of my question. Do you want the default driver defined, and varying between implementations, or do we want a clearly defined default? The stack has a mixture of both. An interface is admin down by default, but it is anybody guess how pause will be configured? By making it driver undefined, you cannot assume anything, and you require user space to always configure it. I don't have too strong an opinion, i'm more interested in what others say, those who have to live with this. Andrew
On Thu, Aug 19, 2021 at 03:13:10PM +0200, Andrew Lunn wrote: > > > Should we also document what the default is? Seems like > > > ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP is the generic network > > > interface default, so maybe it should also be the default for SFPs? > > > > I will add a note in Documentation/networking/ethtool-netlink.rst that > > the default power mode policy is driver-dependent (can be queried) and > > that it can either be 'high' or 'auto'. > > Hi Ido Hi Andrew, > > That is kind of my question. Do you want the default driver defined, > and varying between implementations, or do we want a clearly defined > default? > > The stack has a mixture of both. An interface is admin down by > default, but it is anybody guess how pause will be configured? > > By making it driver undefined, you cannot assume anything, and you > require user space to always configure it. > > I don't have too strong an opinion, i'm more interested in what others > say, those who have to live with this. I evaluated the link up times using a QSFP module [1] connected to my system. There is a 36% increase in link up times when using the 'auto' policy compared to the 'high' policy (default on all Mellanox systems). Very noticeable and very measurable. Couple the above with the fact that despite shipping millions of ports over the years, we are only now getting requests to control the power mode of transceivers and from a small number of users. In addition, any user space that is interested in changing the behavior, has the ability to query the default policy and override it in a vendor-agnostic way. Therefore, I'm strictly against changing the existing behavior. [1] https://www.mellanox.com/related-docs/prod_cables/PB_MFS1S00-VxxxE_200GbE_QSFP56_AOC.pdf > > Andrew
On Thu, 19 Aug 2021 10:47:03 +0300 Ido Schimmel wrote: > > > + * enum ethtool_module_power_mode_policy - plug-in module power mode policy > > > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_LOW: Module is always in low power mode. > > > > Did you have a use case for this one or is it for completeness? Seems > > like user can just bring the port down if they want no carrier? My > > understanding was you primarily wanted the latter two, and those can > > be freely changed when netdev is running, right? > > In all the implementations I could find (3), the user interface is > high/low (on/off). The proposed interface is more flexible as it > contains both the 'high' / 'low' policies in addition to the more user > friendly 'high-on-up' ('auto') policy. on/off is probably due to blindly setting the SFP bit. Our intention here is to set a policy with respect to the netdev state (or integrate the setting with the rest of the stack, if you will) not just control the bit. IMO we should leave the value out of the enum until the use case for it becomes clear. Adding it later is simple enough. > Given that keeping the 'low' policy does not complicate the > implementation / maintenance I'd argue it does. The netif_running() check is exactly to prevent carrier from going away, so it only makes sense if the low setting exists. We can switch between 'auto' and 'high' any time. > and that it provides users with a similar interface to what they are > used to from other implementations, I would like to keep it in > addition to the other two policies. IIUC the existing interfaces are build around the architecture where driver/fw do not control the SFP automatically IOW 'auto' does not exist. 'low' is there for disabling the SFP when interface is down. The interface where 'low' can't be set while the netdev is up is *already* not 1:1 with the out of tree APIs, right? I'd bet that if you convert users of existing APIs to map 'on' to (always-)high and 'off' to auto everyone will be happy.
On Thu, 19 Aug 2021 17:34:46 +0300 Ido Schimmel wrote: > > That is kind of my question. Do you want the default driver defined, > > and varying between implementations, or do we want a clearly defined > > default? > > > > The stack has a mixture of both. An interface is admin down by > > default, but it is anybody guess how pause will be configured? > > > > By making it driver undefined, you cannot assume anything, and you > > require user space to always configure it. > > > > I don't have too strong an opinion, i'm more interested in what others > > say, those who have to live with this. > > I evaluated the link up times using a QSFP module [1] connected to my > system. There is a 36% increase in link up times when using the 'auto' > policy compared to the 'high' policy (default on all Mellanox systems). > Very noticeable and very measurable. > > Couple the above with the fact that despite shipping millions of ports > over the years, we are only now getting requests to control the power > mode of transceivers and from a small number of users. > > In addition, any user space that is interested in changing the behavior, > has the ability to query the default policy and override it in a > vendor-agnostic way. > > Therefore, I'm strictly against changing the existing behavior. > > [1] https://www.mellanox.com/related-docs/prod_cables/PB_MFS1S00-VxxxE_200GbE_QSFP56_AOC.pdf Fine by me FWIW. Obviously in an ideal world we'd have uniform presets as part of 'what it means to be upstream Linux' but from practical standpoint where most features start out of tree having the requirement of uniformity will be an impediment preventing vendors from switching to upstream APIs. That's my personal opinion or should I say 'gut feeling', I could well be wrong.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, August 19, 2021 7:43 AM > To: Ido Schimmel <idosch@idosch.org> > Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; > davem@davemloft.net; mkubecek@suse.cz; pali@kernel.org; Keller, Jacob E > <jacob.e.keller@intel.com>; jiri@nvidia.com; vadimp@nvidia.com; > mlxsw@nvidia.com; Ido Schimmel <idosch@nvidia.com> > Subject: Re: [RFC PATCH net-next v2 1/6] ethtool: Add ability to control > transceiver modules' power mode > > On Thu, 19 Aug 2021 17:34:46 +0300 Ido Schimmel wrote: > > > That is kind of my question. Do you want the default driver defined, > > > and varying between implementations, or do we want a clearly defined > > > default? > > > > > > The stack has a mixture of both. An interface is admin down by > > > default, but it is anybody guess how pause will be configured? > > > > > > By making it driver undefined, you cannot assume anything, and you > > > require user space to always configure it. > > > > > > I don't have too strong an opinion, i'm more interested in what others > > > say, those who have to live with this. > > > > I evaluated the link up times using a QSFP module [1] connected to my > > system. There is a 36% increase in link up times when using the 'auto' > > policy compared to the 'high' policy (default on all Mellanox systems). > > Very noticeable and very measurable. > > > > Couple the above with the fact that despite shipping millions of ports > > over the years, we are only now getting requests to control the power > > mode of transceivers and from a small number of users. > > > > In addition, any user space that is interested in changing the behavior, > > has the ability to query the default policy and override it in a > > vendor-agnostic way. > > > > Therefore, I'm strictly against changing the existing behavior. > > > > [1] https://www.mellanox.com/related-docs/prod_cables/PB_MFS1S00- > VxxxE_200GbE_QSFP56_AOC.pdf > > Fine by me FWIW. Obviously in an ideal world we'd have uniform presets > as part of 'what it means to be upstream Linux' but from practical > standpoint where most features start out of tree having the requirement > of uniformity will be an impediment preventing vendors from switching to > upstream APIs. That's my personal opinion or should I say 'gut feeling', > I could well be wrong. I think it makes sense to push for uniform presets where we can. But in some cases where we already have a variety of differing behaviors this becomes difficult. I would agree in pushing for uniform defaults in cases where we're clearly adding new functionality. However in cases like this where there exists a wide spectrum of behaviors, it makes more sense to allow individual drivers to maintain the same behavior while reporting that up with the option to configure or change it. It's not ideal if we did everything from scratch, but it's the current reality. Thanks, Jake
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst index c86628e6a235..54a704370bfc 100644 --- a/Documentation/networking/ethtool-netlink.rst +++ b/Documentation/networking/ethtool-netlink.rst @@ -179,7 +179,7 @@ according to message purpose: Userspace to kernel: - ===================================== ================================ + ===================================== ================================= ``ETHTOOL_MSG_STRSET_GET`` get string set ``ETHTOOL_MSG_LINKINFO_GET`` get link settings ``ETHTOOL_MSG_LINKINFO_SET`` set link settings @@ -213,7 +213,9 @@ Userspace to kernel: ``ETHTOOL_MSG_MODULE_EEPROM_GET`` read SFP module EEPROM ``ETHTOOL_MSG_STATS_GET`` get standard statistics ``ETHTOOL_MSG_PHC_VCLOCKS_GET`` get PHC virtual clocks info - ===================================== ================================ + ``ETHTOOL_MSG_MODULE_SET`` set transceiver module parameters + ``ETHTOOL_MSG_MODULE_GET`` get transceiver module parameters + ===================================== ================================= Kernel to userspace: @@ -252,6 +254,7 @@ Kernel to userspace: ``ETHTOOL_MSG_MODULE_EEPROM_GET_REPLY`` read SFP module EEPROM ``ETHTOOL_MSG_STATS_GET_REPLY`` standard statistics ``ETHTOOL_MSG_PHC_VCLOCKS_GET_REPLY`` PHC virtual clocks info + ``ETHTOOL_MSG_MODULE_GET_REPLY`` transceiver module parameters ======================================== ================================= ``GET`` requests are sent by userspace applications to retrieve device @@ -1498,6 +1501,63 @@ Kernel response contents: ``ETHTOOL_A_PHC_VCLOCKS_INDEX`` s32 PHC index array ==================================== ====== ========================== +MODULE_GET +========== + +Gets transceiver module parameters. + +Request contents: + + ===================================== ====== ========================== + ``ETHTOOL_A_MODULE_HEADER`` nested request header + ===================================== ====== ========================== + +Kernel response contents: + + ====================================== ====== ========================== + ``ETHTOOL_A_MODULE_HEADER`` nested reply header + ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` u8 power mode policy + ``ETHTOOL_A_MODULE_POWER_MODE`` u8 operational power mode + ====================================== ====== ========================== + +The optional ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` attribute encodes the +transceiver module power mode policy enforced by the host. + +The optional ``ETHTHOOL_A_MODULE_POWER_MODE`` attribute encodes the operational +power mode policy of the transceiver module. It is only reported when a module +is plugged-in. Possible values are: + +.. kernel-doc:: include/uapi/linux/ethtool.h + :identifiers: ethtool_module_power_mode + +MODULE_SET +========== + +Sets transceiver module parameters. + +Request contents: + + ====================================== ====== ========================== + ``ETHTOOL_A_MODULE_HEADER`` nested request header + ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` u8 power mode policy + ====================================== ====== ========================== + +When set, the optional ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` attribute is used +to set the transceiver module power policy enforced by the host. Possible +values are: + +.. kernel-doc:: include/uapi/linux/ethtool.h + :identifiers: ethtool_module_power_mode_policy + +For SFF-8636 modules, low power mode is forced by the host according to table +6-10 in revision 2.10a of the specification. + +For CMIS modules, low power mode is forced by the host according to table 6-12 +in revision 5.0 of the specification. + +To avoid changes to the operational state of the device, power mode policy can +only be set when the device is administratively down. + Request translation =================== @@ -1597,4 +1657,6 @@ are netlink only. n/a ``ETHTOOL_MSG_CABLE_TEST_TDR_ACT`` n/a ``ETHTOOL_MSG_TUNNEL_INFO_GET`` n/a ``ETHTOOL_MSG_PHC_VCLOCKS_GET`` + n/a ``ETHTOOL_MSG_MODULE_GET`` + n/a ``ETHTOOL_MSG_MODULE_SET`` =================================== ===================================== diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 4711b96dae0c..07d40dc20ca4 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -405,6 +405,20 @@ struct ethtool_module_eeprom { u8 *data; }; +/** + * struct ethtool_module_power_mode_params - module power mode parameters + * @policy: The power mode policy enforced by the host for the plug-in module. + * @mode: The operational power mode of the plug-in module. Should be filled by + * device drivers on get operations. + * @mode_valid: Indicates the validity of the @mode field. Should be set by + * device drivers on get operations when a module is plugged-in. + */ +struct ethtool_module_power_mode_params { + enum ethtool_module_power_mode_policy policy; + enum ethtool_module_power_mode mode; + u8 mode_valid:1; +}; + /** * struct ethtool_ops - optional netdev operations * @cap_link_lanes_supported: indicates if the driver supports lanes @@ -570,6 +584,11 @@ struct ethtool_module_eeprom { * @get_eth_ctrl_stats: Query some of the IEEE 802.3 MAC Ctrl statistics. * @get_rmon_stats: Query some of the RMON (RFC 2819) statistics. * Set %ranges to a pointer to zero-terminated array of byte ranges. + * @get_module_power_mode: Get the power mode policy for the plug-in module + * used by the network device and its operational power mode, if + * plugged-in. + * @set_module_power_mode: Set the power mode policy for the plug-in module + * used by the network device. * * All operations are optional (i.e. the function pointer may be set * to %NULL) and callers must take this into account. Callers must @@ -689,6 +708,12 @@ struct ethtool_ops { void (*get_rmon_stats)(struct net_device *dev, struct ethtool_rmon_stats *rmon_stats, const struct ethtool_rmon_hist_range **ranges); + int (*get_module_power_mode)(struct net_device *dev, + struct ethtool_module_power_mode_params *params, + struct netlink_ext_ack *extack); + int (*set_module_power_mode)(struct net_device *dev, + const struct ethtool_module_power_mode_params *params, + struct netlink_ext_ack *extack); }; int ethtool_check_ops(const struct ethtool_ops *ops); diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 67aa7134b301..0a52ee560c3a 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -704,6 +704,31 @@ enum ethtool_stringset { ETH_SS_COUNT }; +/** + * enum ethtool_module_power_mode_policy - plug-in module power mode policy + * @ETHTOOL_MODULE_POWER_MODE_POLICY_LOW: Module is always in low power mode. + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH: Module is always in high power mode. + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP: Module is transitioned by the + * host to high power mode when the first port using it is put + * administratively up and to low power mode when the last port using it + * is put administratively down. + */ +enum ethtool_module_power_mode_policy { + ETHTOOL_MODULE_POWER_MODE_POLICY_LOW, + ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH, + ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP, +}; + +/** + * enum ethtool_module_power_mode - plug-in module power mode + * @ETHTOOL_MODULE_POWER_MODE_LOW: Module is in low power mode. + * @ETHTOOL_MODULE_POWER_MODE_HIGH: Module is in high power mode. + */ +enum ethtool_module_power_mode { + ETHTOOL_MODULE_POWER_MODE_LOW, + ETHTOOL_MODULE_POWER_MODE_HIGH, +}; + /** * struct ethtool_gstrings - string set for data tagging * @cmd: Command number = %ETHTOOL_GSTRINGS diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h index b3b93710eff7..c22f5c902060 100644 --- a/include/uapi/linux/ethtool_netlink.h +++ b/include/uapi/linux/ethtool_netlink.h @@ -47,6 +47,8 @@ enum { ETHTOOL_MSG_MODULE_EEPROM_GET, ETHTOOL_MSG_STATS_GET, ETHTOOL_MSG_PHC_VCLOCKS_GET, + ETHTOOL_MSG_MODULE_GET, + ETHTOOL_MSG_MODULE_SET, /* add new constants above here */ __ETHTOOL_MSG_USER_CNT, @@ -90,6 +92,8 @@ enum { ETHTOOL_MSG_MODULE_EEPROM_GET_REPLY, ETHTOOL_MSG_STATS_GET_REPLY, ETHTOOL_MSG_PHC_VCLOCKS_GET_REPLY, + ETHTOOL_MSG_MODULE_GET_REPLY, + ETHTOOL_MSG_MODULE_NTF, /* add new constants above here */ __ETHTOOL_MSG_KERNEL_CNT, @@ -831,6 +835,19 @@ enum { ETHTOOL_A_STATS_RMON_MAX = (__ETHTOOL_A_STATS_RMON_CNT - 1) }; +/* MODULE */ + +enum { + ETHTOOL_A_MODULE_UNSPEC, + ETHTOOL_A_MODULE_HEADER, /* nest - _A_HEADER_* */ + ETHTOOL_A_MODULE_POWER_MODE_POLICY, /* u8 */ + ETHTOOL_A_MODULE_POWER_MODE, /* u8 */ + + /* add new constants above here */ + __ETHTOOL_A_MODULE_CNT, + ETHTOOL_A_MODULE_MAX = (__ETHTOOL_A_MODULE_CNT - 1) +}; + /* generic netlink info */ #define ETHTOOL_GENL_NAME "ethtool" #define ETHTOOL_GENL_VERSION 1 diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile index 0a19470efbfb..b76432e70e6b 100644 --- a/net/ethtool/Makefile +++ b/net/ethtool/Makefile @@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o ethtool_nl-y := netlink.o bitset.o strset.o linkinfo.o linkmodes.o \ linkstate.o debug.o wol.o features.o privflags.o rings.o \ channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \ - tunnels.o fec.o eeprom.o stats.o phc_vclocks.o + tunnels.o fec.o eeprom.o stats.o phc_vclocks.o module.o diff --git a/net/ethtool/module.c b/net/ethtool/module.c new file mode 100644 index 000000000000..6fb7d84fabf7 --- /dev/null +++ b/net/ethtool/module.c @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <linux/ethtool.h> + +#include "netlink.h" +#include "common.h" +#include "bitset.h" + +struct module_req_info { + struct ethnl_req_info base; +}; + +struct module_reply_data { + struct ethnl_reply_data base; + struct ethtool_module_power_mode_params power; + u8 power_valid:1; +}; + +#define MODULE_REPDATA(__reply_base) \ + container_of(__reply_base, struct module_reply_data, base) + +/* MODULE_GET */ + +const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1] = { + [ETHTOOL_A_MODULE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy), +}; + +static int module_get_power_mode(struct net_device *dev, + struct module_reply_data *data, + struct netlink_ext_ack *extack) +{ + const struct ethtool_ops *ops = dev->ethtool_ops; + int ret; + + if (!ops->get_module_power_mode) + return 0; + + ret = ops->get_module_power_mode(dev, &data->power, extack); + if (ret < 0) + return ret; + + data->power_valid = true; + + return 0; +} + +static int module_prepare_data(const struct ethnl_req_info *req_base, + struct ethnl_reply_data *reply_base, + struct genl_info *info) +{ + struct module_reply_data *data = MODULE_REPDATA(reply_base); + struct netlink_ext_ack *extack = info ? info->extack : NULL; + struct net_device *dev = reply_base->dev; + int ret; + + ret = ethnl_ops_begin(dev); + if (ret < 0) + return ret; + + ret = module_get_power_mode(dev, data, extack); + if (ret < 0) + goto out_complete; + +out_complete: + ethnl_ops_complete(dev); + return ret; +} + +static int module_reply_size(const struct ethnl_req_info *req_base, + const struct ethnl_reply_data *reply_base) +{ + struct module_reply_data *data = MODULE_REPDATA(reply_base); + int len = 0; + + if (data->power_valid) + len += nla_total_size(sizeof(u8)); /* _MODULE_POWER_MODE_POLICY */ + + if (data->power_valid && data->power.mode_valid) + len += nla_total_size(sizeof(u8)); /* _MODULE_POWER_MODE */ + + return len; +} + +static int module_fill_reply(struct sk_buff *skb, + const struct ethnl_req_info *req_base, + const struct ethnl_reply_data *reply_base) +{ + const struct module_reply_data *data = MODULE_REPDATA(reply_base); + + if (data->power_valid && + nla_put_u8(skb, ETHTOOL_A_MODULE_POWER_MODE_POLICY, + data->power.policy)) + return -EMSGSIZE; + + if (data->power_valid && data->power.mode_valid && + nla_put_u8(skb, ETHTOOL_A_MODULE_POWER_MODE, data->power.mode)) + return -EMSGSIZE; + + return 0; +} + +const struct ethnl_request_ops ethnl_module_request_ops = { + .request_cmd = ETHTOOL_MSG_MODULE_GET, + .reply_cmd = ETHTOOL_MSG_MODULE_GET_REPLY, + .hdr_attr = ETHTOOL_A_MODULE_HEADER, + .req_info_size = sizeof(struct module_req_info), + .reply_data_size = sizeof(struct module_reply_data), + + .prepare_data = module_prepare_data, + .reply_size = module_reply_size, + .fill_reply = module_fill_reply, +}; + +/* MODULE_SET */ + +const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1] = { + [ETHTOOL_A_MODULE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy), + [ETHTOOL_A_MODULE_POWER_MODE_POLICY] = + NLA_POLICY_MAX(NLA_U8, ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP), +}; + +static int module_set_power_mode(struct net_device *dev, struct nlattr **tb, + bool *p_mod, struct netlink_ext_ack *extack) +{ + struct ethtool_module_power_mode_params power = {}; + struct ethtool_module_power_mode_params power_new; + const struct ethtool_ops *ops = dev->ethtool_ops; + int ret; + + if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]) + return 0; + + if (!ops->get_module_power_mode || !ops->set_module_power_mode) { + NL_SET_ERR_MSG_ATTR(extack, + tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], + "Setting power mode policy is not supported by this device"); + return -EOPNOTSUPP; + } + + if (netif_running(dev)) { + NL_SET_ERR_MSG_ATTR(extack, + tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], + "Cannot set power mode policy when port is administratively up"); + return -EINVAL; + } + + power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]); + ret = ops->get_module_power_mode(dev, &power, extack); + if (ret < 0) + return ret; + *p_mod = power_new.policy != power.policy; + + return ops->set_module_power_mode(dev, &power_new, extack); +} + +int ethnl_set_module(struct sk_buff *skb, struct genl_info *info) +{ + struct ethnl_req_info req_info = {}; + struct nlattr **tb = info->attrs; + struct net_device *dev; + bool mod = false; + int ret; + + ret = ethnl_parse_header_dev_get(&req_info, tb[ETHTOOL_A_MODULE_HEADER], + genl_info_net(info), info->extack, + true); + if (ret < 0) + return ret; + dev = req_info.dev; + + rtnl_lock(); + ret = ethnl_ops_begin(dev); + if (ret < 0) + goto out_rtnl; + + ret = module_set_power_mode(dev, tb, &mod, info->extack); + if (ret < 0) + goto out_ops; + + if (!mod) + goto out_ops; + + ethtool_notify(dev, ETHTOOL_MSG_MODULE_NTF, NULL); + +out_ops: + ethnl_ops_complete(dev); +out_rtnl: + rtnl_unlock(); + dev_put(dev); + return ret; +} diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 1797a0a90019..38b44c0291b1 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -282,6 +282,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = { [ETHTOOL_MSG_MODULE_EEPROM_GET] = ðnl_module_eeprom_request_ops, [ETHTOOL_MSG_STATS_GET] = ðnl_stats_request_ops, [ETHTOOL_MSG_PHC_VCLOCKS_GET] = ðnl_phc_vclocks_request_ops, + [ETHTOOL_MSG_MODULE_GET] = ðnl_module_request_ops, }; static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb) @@ -593,6 +594,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = { [ETHTOOL_MSG_PAUSE_NTF] = ðnl_pause_request_ops, [ETHTOOL_MSG_EEE_NTF] = ðnl_eee_request_ops, [ETHTOOL_MSG_FEC_NTF] = ðnl_fec_request_ops, + [ETHTOOL_MSG_MODULE_NTF] = ðnl_module_request_ops, }; /* default notification handler */ @@ -686,6 +688,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = { [ETHTOOL_MSG_PAUSE_NTF] = ethnl_default_notify, [ETHTOOL_MSG_EEE_NTF] = ethnl_default_notify, [ETHTOOL_MSG_FEC_NTF] = ethnl_default_notify, + [ETHTOOL_MSG_MODULE_NTF] = ethnl_default_notify, }; void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data) @@ -999,6 +1002,22 @@ static const struct genl_ops ethtool_genl_ops[] = { .policy = ethnl_phc_vclocks_get_policy, .maxattr = ARRAY_SIZE(ethnl_phc_vclocks_get_policy) - 1, }, + { + .cmd = ETHTOOL_MSG_MODULE_GET, + .doit = ethnl_default_doit, + .start = ethnl_default_start, + .dumpit = ethnl_default_dumpit, + .done = ethnl_default_done, + .policy = ethnl_module_get_policy, + .maxattr = ARRAY_SIZE(ethnl_module_get_policy) - 1, + }, + { + .cmd = ETHTOOL_MSG_MODULE_SET, + .flags = GENL_UNS_ADMIN_PERM, + .doit = ethnl_set_module, + .policy = ethnl_module_set_policy, + .maxattr = ARRAY_SIZE(ethnl_module_set_policy) - 1, + }, }; static const struct genl_multicast_group ethtool_nl_mcgrps[] = { diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index 077aac3929a8..1aafba5b67bb 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -337,6 +337,7 @@ extern const struct ethnl_request_ops ethnl_fec_request_ops; extern const struct ethnl_request_ops ethnl_module_eeprom_request_ops; extern const struct ethnl_request_ops ethnl_stats_request_ops; extern const struct ethnl_request_ops ethnl_phc_vclocks_request_ops; +extern const struct ethnl_request_ops ethnl_module_request_ops; extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1]; extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1]; @@ -373,6 +374,8 @@ extern const struct nla_policy ethnl_fec_set_policy[ETHTOOL_A_FEC_AUTO + 1]; extern const struct nla_policy ethnl_module_eeprom_get_policy[ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS + 1]; extern const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1]; extern const struct nla_policy ethnl_phc_vclocks_get_policy[ETHTOOL_A_PHC_VCLOCKS_HEADER + 1]; +extern const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1]; +extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1]; int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info); int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info); @@ -391,6 +394,7 @@ int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info); int ethnl_tunnel_info_start(struct netlink_callback *cb); int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb); int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info); +int ethnl_set_module(struct sk_buff *skb, struct genl_info *info); extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN]; extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];