* [PATCH net-next 1/3] ethtool: Make module API more generic
2024-03-29 9:23 [PATCH net-next 0/3] ethtool: Max power support Wojciech Drewek
@ 2024-03-29 9:23 ` Wojciech Drewek
2024-03-29 9:23 ` [PATCH net-next 2/3] ethtool: Introduce max power support Wojciech Drewek
` (3 subsequent siblings)
4 siblings, 0 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-03-29 9:23 UTC (permalink / raw)
To: netdev
Cc: intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet, kuba,
pabeni, idosch, przemyslaw.kitszel, marcin.szycik
Prepare to introduce new attributes to ethtool module API by making
some of the names more generic.
Rename:
ethtool_module_power_mode_params -> ethtool_module_power_params
This structure will have more fields than just mode or mode_policy.
get_module_power_mode -> get_module_power_cfg
set_module_power_mode -> set_module_power_cfg
Those operations are going to be used for more cases than just
setting or getting power mode.
Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
.../net/ethernet/mellanox/mlxsw/core_env.c | 2 +-
.../net/ethernet/mellanox/mlxsw/core_env.h | 2 +-
drivers/net/ethernet/mellanox/mlxsw/minimal.c | 8 +++---
.../mellanox/mlxsw/spectrum_ethtool.c | 8 +++---
include/linux/ethtool.h | 20 ++++++-------
net/ethtool/module.c | 28 +++++++++----------
6 files changed, 33 insertions(+), 35 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_env.c b/drivers/net/ethernet/mellanox/mlxsw/core_env.c
index 53b150b7ae4e..3d7841f0ceec 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_env.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_env.c
@@ -581,7 +581,7 @@ EXPORT_SYMBOL(mlxsw_env_reset_module);
int
mlxsw_env_get_module_power_mode(struct mlxsw_core *mlxsw_core, u8 slot_index,
u8 module,
- struct ethtool_module_power_mode_params *params,
+ struct ethtool_module_power_params *params,
struct netlink_ext_ack *extack)
{
struct mlxsw_env *mlxsw_env = mlxsw_core_env(mlxsw_core);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_env.h b/drivers/net/ethernet/mellanox/mlxsw/core_env.h
index a197e3ae069c..979a35e967b3 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_env.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_env.h
@@ -35,7 +35,7 @@ int mlxsw_env_reset_module(struct net_device *netdev,
int
mlxsw_env_get_module_power_mode(struct mlxsw_core *mlxsw_core, u8 slot_index,
u8 module,
- struct ethtool_module_power_mode_params *params,
+ struct ethtool_module_power_params *params,
struct netlink_ext_ack *extack);
int
diff --git a/drivers/net/ethernet/mellanox/mlxsw/minimal.c b/drivers/net/ethernet/mellanox/mlxsw/minimal.c
index f0ceb196a6ce..df46962e92b4 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/minimal.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/minimal.c
@@ -152,7 +152,7 @@ static int mlxsw_m_reset(struct net_device *netdev, u32 *flags)
static int
mlxsw_m_get_module_power_mode(struct net_device *netdev,
- struct ethtool_module_power_mode_params *params,
+ struct ethtool_module_power_params *params,
struct netlink_ext_ack *extack)
{
struct mlxsw_m_port *mlxsw_m_port = netdev_priv(netdev);
@@ -165,7 +165,7 @@ mlxsw_m_get_module_power_mode(struct net_device *netdev,
static int
mlxsw_m_set_module_power_mode(struct net_device *netdev,
- const struct ethtool_module_power_mode_params *params,
+ const struct ethtool_module_power_params *params,
struct netlink_ext_ack *extack)
{
struct mlxsw_m_port *mlxsw_m_port = netdev_priv(netdev);
@@ -182,8 +182,8 @@ static const struct ethtool_ops mlxsw_m_port_ethtool_ops = {
.get_module_eeprom = mlxsw_m_get_module_eeprom,
.get_module_eeprom_by_page = mlxsw_m_get_module_eeprom_by_page,
.reset = mlxsw_m_reset,
- .get_module_power_mode = mlxsw_m_get_module_power_mode,
- .set_module_power_mode = mlxsw_m_set_module_power_mode,
+ .get_module_power_cfg = mlxsw_m_get_module_power_mode,
+ .set_module_power_cfg = mlxsw_m_set_module_power_mode,
};
static int
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c
index 0f29e9c19411..5ea7241bfaba 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c
@@ -1214,7 +1214,7 @@ static int mlxsw_sp_reset(struct net_device *dev, u32 *flags)
static int
mlxsw_sp_get_module_power_mode(struct net_device *dev,
- struct ethtool_module_power_mode_params *params,
+ struct ethtool_module_power_params *params,
struct netlink_ext_ack *extack)
{
struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
@@ -1228,7 +1228,7 @@ mlxsw_sp_get_module_power_mode(struct net_device *dev,
static int
mlxsw_sp_set_module_power_mode(struct net_device *dev,
- const struct ethtool_module_power_mode_params *params,
+ const struct ethtool_module_power_params *params,
struct netlink_ext_ack *extack)
{
struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
@@ -1262,8 +1262,8 @@ const struct ethtool_ops mlxsw_sp_port_ethtool_ops = {
.get_eth_ctrl_stats = mlxsw_sp_get_eth_ctrl_stats,
.get_rmon_stats = mlxsw_sp_get_rmon_stats,
.reset = mlxsw_sp_reset,
- .get_module_power_mode = mlxsw_sp_get_module_power_mode,
- .set_module_power_mode = mlxsw_sp_set_module_power_mode,
+ .get_module_power_cfg = mlxsw_sp_get_module_power_mode,
+ .set_module_power_cfg = mlxsw_sp_set_module_power_mode,
};
struct mlxsw_sp1_port_link_mode {
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9901e563f706..f3af6b31c9f1 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -506,12 +506,12 @@ struct ethtool_module_eeprom {
};
/**
- * struct ethtool_module_power_mode_params - module power mode parameters
+ * struct ethtool_module_power_params - module power 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.
*/
-struct ethtool_module_power_mode_params {
+struct ethtool_module_power_params {
enum ethtool_module_power_mode_policy policy;
enum ethtool_module_power_mode mode;
};
@@ -804,10 +804,10 @@ struct ethtool_rxfh_param {
* @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
+ * @get_module_power_cfg: 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
+ * @set_module_power_cfg: Set the power mode policy for the plug-in module
* used by the network device.
* @get_mm: Query the 802.3 MAC Merge layer state.
* @set_mm: Set the 802.3 MAC Merge layer parameters.
@@ -940,12 +940,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 (*get_module_power_cfg)(struct net_device *dev,
+ struct ethtool_module_power_params *params,
+ struct netlink_ext_ack *extack);
+ int (*set_module_power_cfg)(struct net_device *dev,
+ const struct ethtool_module_power_params *params,
+ struct netlink_ext_ack *extack);
int (*get_mm)(struct net_device *dev, struct ethtool_mm_state *state);
int (*set_mm)(struct net_device *dev, struct ethtool_mm_cfg *cfg,
struct netlink_ext_ack *extack);
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index ceb575efc290..193ca4642e04 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -12,7 +12,7 @@ struct module_req_info {
struct module_reply_data {
struct ethnl_reply_data base;
- struct ethtool_module_power_mode_params power;
+ struct ethtool_module_power_params power;
};
#define MODULE_REPDATA(__reply_base) \
@@ -24,16 +24,16 @@ 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)
+static int module_get_power_cfg(struct net_device *dev,
+ struct module_reply_data *data,
+ struct netlink_ext_ack *extack)
{
const struct ethtool_ops *ops = dev->ethtool_ops;
- if (!ops->get_module_power_mode)
+ if (!ops->get_module_power_cfg)
return 0;
- return ops->get_module_power_mode(dev, &data->power, extack);
+ return ops->get_module_power_cfg(dev, &data->power, extack);
}
static int module_prepare_data(const struct ethnl_req_info *req_base,
@@ -48,7 +48,7 @@ static int module_prepare_data(const struct ethnl_req_info *req_base,
if (ret < 0)
return ret;
- ret = module_get_power_mode(dev, data, info->extack);
+ ret = module_get_power_cfg(dev, data, info->extack);
if (ret < 0)
goto out_complete;
@@ -109,10 +109,8 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info,
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(info->extack,
- tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY],
- "Setting power mode policy is not supported by this device");
+ if (!ops->get_module_power_cfg || !ops->set_module_power_cfg) {
+ NL_SET_ERR_MSG(info->extack, "Setting power config is not supported by this device");
return -EOPNOTSUPP;
}
@@ -122,8 +120,8 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info,
static int
ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
{
- struct ethtool_module_power_mode_params power = {};
- struct ethtool_module_power_mode_params power_new;
+ struct ethtool_module_power_params power = {};
+ struct ethtool_module_power_params power_new;
const struct ethtool_ops *ops;
struct net_device *dev = req_info->dev;
struct nlattr **tb = info->attrs;
@@ -132,14 +130,14 @@ ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
ops = dev->ethtool_ops;
power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]);
- ret = ops->get_module_power_mode(dev, &power, info->extack);
+ ret = ops->get_module_power_cfg(dev, &power, info->extack);
if (ret < 0)
return ret;
if (power_new.policy == power.policy)
return 0;
- ret = ops->set_module_power_mode(dev, &power_new, info->extack);
+ ret = ops->set_module_power_cfg(dev, &power_new, info->extack);
return ret < 0 ? ret : 1;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH net-next 2/3] ethtool: Introduce max power support
2024-03-29 9:23 [PATCH net-next 0/3] ethtool: Max power support Wojciech Drewek
2024-03-29 9:23 ` [PATCH net-next 1/3] ethtool: Make module API more generic Wojciech Drewek
@ 2024-03-29 9:23 ` Wojciech Drewek
2024-03-29 22:29 ` Jakub Kicinski
2024-03-30 22:14 ` Andrew Lunn
2024-03-29 9:23 ` [PATCH net-next 3/3] ice: Implement ethtool max power configuration Wojciech Drewek
` (2 subsequent siblings)
4 siblings, 2 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-03-29 9:23 UTC (permalink / raw)
To: netdev
Cc: intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet, kuba,
pabeni, idosch, przemyslaw.kitszel, marcin.szycik
Some modules use nonstandard power levels. Adjust ethtool
module implementation to support new attributes that will allow user
to change maximum power.
Add three new get attributes:
ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
maximum power in the cage
ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
cage reported by device
ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
cage reported by device
Add two new set attributes:
ETHTOOL_A_MODULE_MAX_POWER_SET (used for get as well) - change
maximum power in the cage to the given value (milliwatts)
ETHTOOL_A_MODULE_MAX_POWER_RESET - reset maximum power setting to the
default value
Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
include/linux/ethtool.h | 17 +++++--
include/uapi/linux/ethtool_netlink.h | 4 ++
net/ethtool/module.c | 74 ++++++++++++++++++++++++++--
net/ethtool/netlink.h | 2 +-
4 files changed, 87 insertions(+), 10 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f3af6b31c9f1..74ed8997443a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -510,10 +510,18 @@ struct ethtool_module_eeprom {
* @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.
+ * @min_pwr_allowed: minimum power allowed in the cage reported by device
+ * @max_pwr_allowed: maximum power allowed in the cage reported by device
+ * @max_pwr_set: maximum power currently set in the cage
+ * @max_pwr_reset: restore default minimum power
*/
struct ethtool_module_power_params {
enum ethtool_module_power_mode_policy policy;
enum ethtool_module_power_mode mode;
+ u32 min_pwr_allowed;
+ u32 max_pwr_allowed;
+ u32 max_pwr_set;
+ u8 max_pwr_reset;
};
/**
@@ -804,11 +812,12 @@ struct ethtool_rxfh_param {
* @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_cfg: Get the power mode policy for the plug-in module
- * used by the network device and its operational power mode, if
- * plugged-in.
+ * @get_module_power_cfg: Get the power configuration for the plug-in module
+ * used by the network device which includes: its power mode policy and
+ * operational power mode, if plugged-in; maximum power settings
+ * (min and max allowed power and max power currently set)
* @set_module_power_cfg: Set the power mode policy for the plug-in module
- * used by the network device.
+ * used by the network device and its maximum power.
* @get_mm: Query the 802.3 MAC Merge layer state.
* @set_mm: Set the 802.3 MAC Merge layer parameters.
* @get_mm_stats: Query the 802.3 MAC Merge layer statistics.
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 3f89074aa06c..f7cd446b2a83 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -882,6 +882,10 @@ enum {
ETHTOOL_A_MODULE_HEADER, /* nest - _A_HEADER_* */
ETHTOOL_A_MODULE_POWER_MODE_POLICY, /* u8 */
ETHTOOL_A_MODULE_POWER_MODE, /* u8 */
+ ETHTOOL_A_MODULE_MAX_POWER_SET, /* u32 */
+ ETHTOOL_A_MODULE_MIN_POWER_ALLOWED, /* u32 */
+ ETHTOOL_A_MODULE_MAX_POWER_ALLOWED, /* u32 */
+ ETHTOOL_A_MODULE_MAX_POWER_RESET, /* u8 */
/* add new constants above here */
__ETHTOOL_A_MODULE_CNT,
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index 193ca4642e04..9f63a276357e 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -69,6 +69,15 @@ static int module_reply_size(const struct ethnl_req_info *req_base,
if (data->power.mode)
len += nla_total_size(sizeof(u8)); /* _MODULE_POWER_MODE */
+ if (data->power.min_pwr_allowed)
+ len += nla_total_size(sizeof(u32)); /* _MIN_POWER_ALLOWED */
+
+ if (data->power.max_pwr_allowed)
+ len += nla_total_size(sizeof(u32)); /* _MAX_POWER_ALLOWED */
+
+ if (data->power.max_pwr_set)
+ len += nla_total_size(sizeof(u32)); /* _MAX_POWER_SET */
+
return len;
}
@@ -77,6 +86,7 @@ static int module_fill_reply(struct sk_buff *skb,
const struct ethnl_reply_data *reply_base)
{
const struct module_reply_data *data = MODULE_REPDATA(reply_base);
+ u32 temp;
if (data->power.policy &&
nla_put_u8(skb, ETHTOOL_A_MODULE_POWER_MODE_POLICY,
@@ -87,16 +97,30 @@ static int module_fill_reply(struct sk_buff *skb,
nla_put_u8(skb, ETHTOOL_A_MODULE_POWER_MODE, data->power.mode))
return -EMSGSIZE;
+ temp = data->power.min_pwr_allowed;
+ if (temp && nla_put_u32(skb, ETHTOOL_A_MODULE_MIN_POWER_ALLOWED, temp))
+ return -EMSGSIZE;
+
+ temp = data->power.max_pwr_allowed;
+ if (temp && nla_put_u32(skb, ETHTOOL_A_MODULE_MAX_POWER_ALLOWED, temp))
+ return -EMSGSIZE;
+
+ temp = data->power.max_pwr_set;
+ if (temp && nla_put_u32(skb, ETHTOOL_A_MODULE_MAX_POWER_SET, temp))
+ return -EMSGSIZE;
+
return 0;
}
/* MODULE_SET */
-const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1] = {
+const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_MAX + 1] = {
[ETHTOOL_A_MODULE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_MODULE_POWER_MODE_POLICY] =
NLA_POLICY_RANGE(NLA_U8, ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH,
ETHTOOL_MODULE_POWER_MODE_POLICY_AUTO),
+ [ETHTOOL_A_MODULE_MAX_POWER_SET] = { .type = NLA_U32 },
+ [ETHTOOL_A_MODULE_MAX_POWER_RESET] = { .type = NLA_U8 },
};
static int
@@ -106,7 +130,9 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info,
const struct ethtool_ops *ops = req_info->dev->ethtool_ops;
struct nlattr **tb = info->attrs;
- if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY])
+ if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY] &&
+ !tb[ETHTOOL_A_MODULE_MAX_POWER_SET] &&
+ !tb[ETHTOOL_A_MODULE_MAX_POWER_RESET])
return 0;
if (!ops->get_module_power_cfg || !ops->set_module_power_cfg) {
@@ -117,26 +143,64 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info,
return 1;
}
+static void
+ethnl_update_policy(enum ethtool_module_power_mode_policy *dst,
+ const struct nlattr *attr, bool *mod)
+{
+ u8 val = *dst;
+
+ ethnl_update_u8(&val, attr, mod);
+
+ if (mod)
+ *dst = val;
+}
+
static int
ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
{
struct ethtool_module_power_params power = {};
struct ethtool_module_power_params power_new;
- const struct ethtool_ops *ops;
struct net_device *dev = req_info->dev;
struct nlattr **tb = info->attrs;
+ const struct ethtool_ops *ops;
int ret;
+ bool mod;
ops = dev->ethtool_ops;
- power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]);
ret = ops->get_module_power_cfg(dev, &power, info->extack);
if (ret < 0)
return ret;
- if (power_new.policy == power.policy)
+ power_new.max_pwr_set = power.max_pwr_set;
+ power_new.policy = power.policy;
+
+ ethnl_update_u32(&power_new.max_pwr_set,
+ tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod);
+
+ if (mod) {
+ if (power_new.max_pwr_set > power.max_pwr_allowed) {
+ NL_SET_ERR_MSG(info->extack, "Provided value is higher than maximum allowed");
+ return -EINVAL;
+ } else if (power_new.max_pwr_set < power.min_pwr_allowed) {
+ NL_SET_ERR_MSG(info->extack, "Provided value is lower than minimum allowed");
+ return -EINVAL;
+ }
+ }
+
+ ethnl_update_policy(&power_new.policy,
+ tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], &mod);
+ ethnl_update_u8(&power_new.max_pwr_reset,
+ tb[ETHTOOL_A_MODULE_MAX_POWER_RESET], &mod);
+
+ if (!mod)
return 0;
+ if (power_new.max_pwr_reset && power_new.max_pwr_set) {
+ NL_SET_ERR_MSG(info->extack, "Maximum power set and reset cannot be used at the same time");
+ return 0;
+ }
+
ret = ops->set_module_power_cfg(dev, &power_new, info->extack);
return ret < 0 ? ret : 1;
}
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 9a333a8d04c1..6282f84811ce 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -432,7 +432,7 @@ extern const struct nla_policy ethnl_module_eeprom_get_policy[ETHTOOL_A_MODULE_E
extern const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_SRC + 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];
+extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_MAX + 1];
extern const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1];
extern const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1];
extern const struct nla_policy ethnl_rss_get_policy[ETHTOOL_A_RSS_CONTEXT + 1];
--
2.40.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-03-29 9:23 ` [PATCH net-next 2/3] ethtool: Introduce max power support Wojciech Drewek
@ 2024-03-29 22:29 ` Jakub Kicinski
2024-03-29 22:29 ` Jakub Kicinski
2024-04-02 11:25 ` Wojciech Drewek
2024-03-30 22:14 ` Andrew Lunn
1 sibling, 2 replies; 34+ messages in thread
From: Jakub Kicinski @ 2024-03-29 22:29 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet,
pabeni, idosch, przemyslaw.kitszel, marcin.szycik
On Fri, 29 Mar 2024 10:23:20 +0100 Wojciech Drewek wrote:
> Some modules use nonstandard power levels. Adjust ethtool
> module implementation to support new attributes that will allow user
> to change maximum power.
>
> Add three new get attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
> maximum power in the cage
1) I'd keep the ETHTOOL_A_MODULE_POWER_ prefix, consistently.
2) The _SET makes it sound like an action. Can we go with
ETHTOOL_A_MODULE_POWER_MAX ? Or ETHTOOL_A_MODULE_POWER_LIMIT?
Yes, ETHTOOL_A_MODULE_POWER_LIMIT
ETHTOOL_A_MODULE_POWER_MAX
ETHTOOL_A_MODULE_POWER_MIN
would sound pretty good to me.
> ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
> cage reported by device
> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
> cage reported by device
>
> Add two new set attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for get as well) - change
> maximum power in the cage to the given value (milliwatts)
> ETHTOOL_A_MODULE_MAX_POWER_RESET - reset maximum power setting to the
> default value
>
> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
> include/linux/ethtool.h | 17 +++++--
> include/uapi/linux/ethtool_netlink.h | 4 ++
> net/ethtool/module.c | 74 ++++++++++++++++++++++++++--
> net/ethtool/netlink.h | 2 +-
> 4 files changed, 87 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index f3af6b31c9f1..74ed8997443a 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -510,10 +510,18 @@ struct ethtool_module_eeprom {
> * @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.
> + * @min_pwr_allowed: minimum power allowed in the cage reported by device
> + * @max_pwr_allowed: maximum power allowed in the cage reported by device
> + * @max_pwr_set: maximum power currently set in the cage
> + * @max_pwr_reset: restore default minimum power
> */
> struct ethtool_module_power_params {
> enum ethtool_module_power_mode_policy policy;
> enum ethtool_module_power_mode mode;
> + u32 min_pwr_allowed;
> + u32 max_pwr_allowed;
> + u32 max_pwr_set;
> + u8 max_pwr_reset;
bool ?
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 3f89074aa06c..f7cd446b2a83 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -882,6 +882,10 @@ enum {
> ETHTOOL_A_MODULE_HEADER, /* nest - _A_HEADER_* */
> ETHTOOL_A_MODULE_POWER_MODE_POLICY, /* u8 */
> ETHTOOL_A_MODULE_POWER_MODE, /* u8 */
> + ETHTOOL_A_MODULE_MAX_POWER_SET, /* u32 */
> + ETHTOOL_A_MODULE_MIN_POWER_ALLOWED, /* u32 */
> + ETHTOOL_A_MODULE_MAX_POWER_ALLOWED, /* u32 */
> + ETHTOOL_A_MODULE_MAX_POWER_RESET, /* u8 */
flag ?
> @@ -77,6 +86,7 @@ static int module_fill_reply(struct sk_buff *skb,
> const struct ethnl_reply_data *reply_base)
> {
> const struct module_reply_data *data = MODULE_REPDATA(reply_base);
> + u32 temp;
tmp ? temp sounds too much like temperature in context of power
> static int
> ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
> {
> struct ethtool_module_power_params power = {};
> struct ethtool_module_power_params power_new;
> - const struct ethtool_ops *ops;
> struct net_device *dev = req_info->dev;
> struct nlattr **tb = info->attrs;
> + const struct ethtool_ops *ops;
> int ret;
> + bool mod;
>
> ops = dev->ethtool_ops;
>
> - power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]);
> ret = ops->get_module_power_cfg(dev, &power, info->extack);
> if (ret < 0)
> return ret;
>
> - if (power_new.policy == power.policy)
> + power_new.max_pwr_set = power.max_pwr_set;
> + power_new.policy = power.policy;
> +
> + ethnl_update_u32(&power_new.max_pwr_set,
> + tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod);
> + if (mod) {
I think we can use if (tb[ETHTOOL_A_MODULE_MAX_POWER_SET]) here
Less error prone for future additions.
> + if (power_new.max_pwr_set > power.max_pwr_allowed) {
> + NL_SET_ERR_MSG(info->extack, "Provided value is higher than maximum allowed");
NL_SET_ERR_MSG_ATTR() to point at the bad attribute.
> + return -EINVAL;
ERANGE?
> + } else if (power_new.max_pwr_set < power.min_pwr_allowed) {
> + NL_SET_ERR_MSG(info->extack, "Provided value is lower than minimum allowed");
> + return -EINVAL;
> + }
> + }
> +
> + ethnl_update_policy(&power_new.policy,
> + tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], &mod);
> + ethnl_update_u8(&power_new.max_pwr_reset,
> + tb[ETHTOOL_A_MODULE_MAX_POWER_RESET], &mod);
I reckon reset should not be allowed if none of the max_pwr values
are set (i.e. most likely driver doesn't support the config)?
> + if (!mod)
> return 0;
>
> + if (power_new.max_pwr_reset && power_new.max_pwr_set) {
Mmm. How is that gonna work? The driver is going to set max_pwr_set
to what's currently configured. So the user is expected to send
ETHTOOL_A_MODULE_MAX_POWER_SET = 0
ETHTOOL_A_MODULE_MAX_POWER_RESET = 1
to reset?
Just:
if (tb[ETHTOOL_A_MODULE_MAX_POWER_RESET] &&
tb[ETHTOOL_A_MODULE_MAX_POWER_SET])
And you can validate this before doing any real work.
> + NL_SET_ERR_MSG(info->extack, "Maximum power set and reset cannot be used at the same time");
> + return 0;
> + }
> +
> ret = ops->set_module_power_cfg(dev, &power_new, info->extack);
> return ret < 0 ? ret : 1;
> }
--
pw-bot: cr
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-03-29 22:29 ` Jakub Kicinski
@ 2024-03-29 22:29 ` Jakub Kicinski
2024-04-02 11:25 ` Wojciech Drewek
1 sibling, 0 replies; 34+ messages in thread
From: Jakub Kicinski @ 2024-03-29 22:29 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet,
pabeni, idosch, przemyslaw.kitszel, marcin.szycik
On Fri, 29 Mar 2024 10:23:20 +0100 Wojciech Drewek wrote:
> Some modules use nonstandard power levels. Adjust ethtool
> module implementation to support new attributes that will allow user
> to change maximum power.
>
> Add three new get attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
> maximum power in the cage
1) I'd keep the ETHTOOL_A_MODULE_POWER_ prefix, consistently.
2) The _SET makes it sound like an action. Can we go with
ETHTOOL_A_MODULE_POWER_MAX ? Or ETHTOOL_A_MODULE_POWER_LIMIT?
Yes, ETHTOOL_A_MODULE_POWER_LIMIT
ETHTOOL_A_MODULE_POWER_MAX
ETHTOOL_A_MODULE_POWER_MIN
would sound pretty good to me.
> ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
> cage reported by device
> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
> cage reported by device
>
> Add two new set attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for get as well) - change
> maximum power in the cage to the given value (milliwatts)
> ETHTOOL_A_MODULE_MAX_POWER_RESET - reset maximum power setting to the
> default value
>
> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
> include/linux/ethtool.h | 17 +++++--
> include/uapi/linux/ethtool_netlink.h | 4 ++
> net/ethtool/module.c | 74 ++++++++++++++++++++++++++--
> net/ethtool/netlink.h | 2 +-
> 4 files changed, 87 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index f3af6b31c9f1..74ed8997443a 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -510,10 +510,18 @@ struct ethtool_module_eeprom {
> * @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.
> + * @min_pwr_allowed: minimum power allowed in the cage reported by device
> + * @max_pwr_allowed: maximum power allowed in the cage reported by device
> + * @max_pwr_set: maximum power currently set in the cage
> + * @max_pwr_reset: restore default minimum power
> */
> struct ethtool_module_power_params {
> enum ethtool_module_power_mode_policy policy;
> enum ethtool_module_power_mode mode;
> + u32 min_pwr_allowed;
> + u32 max_pwr_allowed;
> + u32 max_pwr_set;
> + u8 max_pwr_reset;
bool ?
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 3f89074aa06c..f7cd446b2a83 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -882,6 +882,10 @@ enum {
> ETHTOOL_A_MODULE_HEADER, /* nest - _A_HEADER_* */
> ETHTOOL_A_MODULE_POWER_MODE_POLICY, /* u8 */
> ETHTOOL_A_MODULE_POWER_MODE, /* u8 */
> + ETHTOOL_A_MODULE_MAX_POWER_SET, /* u32 */
> + ETHTOOL_A_MODULE_MIN_POWER_ALLOWED, /* u32 */
> + ETHTOOL_A_MODULE_MAX_POWER_ALLOWED, /* u32 */
> + ETHTOOL_A_MODULE_MAX_POWER_RESET, /* u8 */
flag ?
> @@ -77,6 +86,7 @@ static int module_fill_reply(struct sk_buff *skb,
> const struct ethnl_reply_data *reply_base)
> {
> const struct module_reply_data *data = MODULE_REPDATA(reply_base);
> + u32 temp;
tmp ? temp sounds too much like temperature in context of power
> static int
> ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
> {
> struct ethtool_module_power_params power = {};
> struct ethtool_module_power_params power_new;
> - const struct ethtool_ops *ops;
> struct net_device *dev = req_info->dev;
> struct nlattr **tb = info->attrs;
> + const struct ethtool_ops *ops;
> int ret;
> + bool mod;
>
> ops = dev->ethtool_ops;
>
> - power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]);
> ret = ops->get_module_power_cfg(dev, &power, info->extack);
> if (ret < 0)
> return ret;
>
> - if (power_new.policy == power.policy)
> + power_new.max_pwr_set = power.max_pwr_set;
> + power_new.policy = power.policy;
> +
> + ethnl_update_u32(&power_new.max_pwr_set,
> + tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod);
> + if (mod) {
I think we can use if (tb[ETHTOOL_A_MODULE_MAX_POWER_SET]) here
Less error prone for future additions.
> + if (power_new.max_pwr_set > power.max_pwr_allowed) {
> + NL_SET_ERR_MSG(info->extack, "Provided value is higher than maximum allowed");
NL_SET_ERR_MSG_ATTR() to point at the bad attribute.
> + return -EINVAL;
ERANGE?
> + } else if (power_new.max_pwr_set < power.min_pwr_allowed) {
> + NL_SET_ERR_MSG(info->extack, "Provided value is lower than minimum allowed");
> + return -EINVAL;
> + }
> + }
> +
> + ethnl_update_policy(&power_new.policy,
> + tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], &mod);
> + ethnl_update_u8(&power_new.max_pwr_reset,
> + tb[ETHTOOL_A_MODULE_MAX_POWER_RESET], &mod);
I reckon reset should not be allowed if none of the max_pwr values
are set (i.e. most likely driver doesn't support the config)?
> + if (!mod)
> return 0;
>
> + if (power_new.max_pwr_reset && power_new.max_pwr_set) {
Mmm. How is that gonna work? The driver is going to set max_pwr_set
to what's currently configured. So the user is expected to send
ETHTOOL_A_MODULE_MAX_POWER_SET = 0
ETHTOOL_A_MODULE_MAX_POWER_RESET = 1
to reset?
Just:
if (tb[ETHTOOL_A_MODULE_MAX_POWER_RESET] &&
tb[ETHTOOL_A_MODULE_MAX_POWER_SET])
And you can validate this before doing any real work.
> + NL_SET_ERR_MSG(info->extack, "Maximum power set and reset cannot be used at the same time");
> + return 0;
> + }
> +
> ret = ops->set_module_power_cfg(dev, &power_new, info->extack);
> return ret < 0 ? ret : 1;
> }
--
pw-bot: cr
X-sender: <netdev+bounces-83478-steffen.klassert=secunet.com@vger.kernel.org>
X-Receiver: <steffen.klassert@secunet.com> ORCPT=rfc822;steffen.klassert@secunet.com NOTIFY=NEVER; X-ExtendedProps=BQAVABYAAgAAAAUAFAARAPDFCS25BAlDktII2g02frgPADUAAABNaWNyb3NvZnQuRXhjaGFuZ2UuVHJhbnNwb3J0LkRpcmVjdG9yeURhdGEuSXNSZXNvdXJjZQIAAAUAagAJAAEAAAAAAAAABQAWAAIAAAUAQwACAAAFAEYABwADAAAABQBHAAIAAAUAEgAPAGIAAAAvbz1zZWN1bmV0L291PUV4Y2hhbmdlIEFkbWluaXN0cmF0aXZlIEdyb3VwIChGWURJQk9IRjIzU1BETFQpL2NuPVJlY2lwaWVudHMvY249U3RlZmZlbiBLbGFzc2VydDY4YwUACwAXAL4AAACheZxkHSGBRqAcAp3ukbifQ049REI2LENOPURhdGFiYXNlcyxDTj1FeGNoYW5nZSBBZG1pbmlzdHJhdGl2ZSBHcm91cCAoRllESUJPSEYyM1NQRExUKSxDTj1BZG1pbmlzdHJhdGl2ZSBHcm91cHMsQ049c2VjdW5ldCxDTj1NaWNyb3NvZnQgRXhjaGFuZ2UsQ049U2VydmljZXMsQ049Q29uZmlndXJhdGlvbixEQz1zZWN1bmV0LERDPWRlBQAOABEABiAS9uuMOkqzwmEZDvWNNQUAHQAPAAwAAABtYngtZXNzZW4tMDIFADwAAgAADwA2AAAATWljcm9zb2Z0LkV4Y2hhbmdlLlRyYW5zcG9ydC5NYWlsUmVjaXBpZW50LkRpc3BsYXlOYW1lDwARAAAAS2xhc3NlcnQsIFN0ZWZmZW4FAAwAAgAABQBsAAIAAAUAWAAXAEoAAADwxQktuQQJQ5LSCNoNNn64Q049S2xhc3NlcnQgU3RlZmZlbixPVT1Vc2VycyxPVT1NaWdyYXRpb24sREM9c2VjdW5ldCxEQz1kZQUAJgACAAEFACIADwAxAAAAQXV0b1Jlc3BvbnNlU3VwcHJlc3M6IDANClRyYW5zbWl0SGlzdG9yeTogRmFsc2UNCg8ALwAAAE1pY3Jvc29mdC5FeGNoYW5nZS5UcmFuc3BvcnQuRXhwYW5zaW9uR3JvdXBUeXBlDwAVAAAATWVtYmVyc0dyb3VwRXhwYW5zaW9uBQAjAAIAAQ==
X-CreatedBy: MSExchange15
X-HeloDomain: a.mx.secunet.com
X-ExtendedProps: BQBjAAoAkQ1rGbMv3AgFAGEACAABAAAABQA3AAIAAA8APAAAAE1pY3Jvc29mdC5FeGNoYW5nZS5UcmFuc3BvcnQuTWFpbFJlY2lwaWVudC5Pcmdhbml6YXRpb25TY29wZREAAAAAAAAAAAAAAAAAAAAAAAUASQACAAEFAAQAFCABAAAAHAAAAHN0ZWZmZW4ua2xhc3NlcnRAc2VjdW5ldC5jb20FAAYAAgABBQApAAIAAQ8ACQAAAENJQXVkaXRlZAIAAQUAAgAHAAEAAAAFAAMABwAAAAAABQAFAAIAAQUAYgAKAFIAAADMigAABQBkAA8AAwAAAEh1Yg==
X-Source: SMTP:Default MBX-ESSEN-01
X-SourceIPAddress: 62.96.220.36
X-EndOfInjectedXHeaders: 20181
Received: from cas-essen-02.secunet.de (10.53.40.202) by
mbx-essen-01.secunet.de (10.53.40.197) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.2507.35; Fri, 29 Mar 2024 23:30:12 +0100
Received: from a.mx.secunet.com (62.96.220.36) by cas-essen-02.secunet.de
(10.53.40.202) with Microsoft SMTP Server (version=TLS1_2,
cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35 via Frontend
Transport; Fri, 29 Mar 2024 23:30:12 +0100
Received: from localhost (localhost [127.0.0.1])
by a.mx.secunet.com (Postfix) with ESMTP id 870DE20883
for <steffen.klassert@secunet.com>; Fri, 29 Mar 2024 23:30:12 +0100 (CET)
X-Virus-Scanned: by secunet
X-Spam-Flag: NO
X-Spam-Score: -3.099
X-Spam-Level:
X-Spam-Status: No, score=-3.099 tagged_above=-999 required=2.1
tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.099, DKIM_SIGNED=0.1,
DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, MAILING_LIST_MULTI=-1,
RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001]
autolearn=unavailable autolearn_force=no
Authentication-Results: a.mx.secunet.com (amavisd-new);
dkim=pass (2048-bit key) header.d=kernel.org
Received: from a.mx.secunet.com ([127.0.0.1])
by localhost (a.mx.secunet.com [127.0.0.1]) (amavisd-new, port 10024)
with ESMTP id 66TQ_oaDF-3X for <steffen.klassert@secunet.com>;
Fri, 29 Mar 2024 23:30:11 +0100 (CET)
Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=147.75.80.249; helo=am.mirrors.kernel.org; envelope-from=netdev+bounces-83478-steffen.klassert=secunet.com@vger.kernel.org; receiver=steffen.klassert@secunet.com
DKIM-Filter: OpenDKIM Filter v2.11.0 a.mx.secunet.com D1BDB208AC
Received: from am.mirrors.kernel.org (am.mirrors.kernel.org [147.75.80.249])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by a.mx.secunet.com (Postfix) with ESMTPS id D1BDB208AC
for <steffen.klassert@secunet.com>; Fri, 29 Mar 2024 23:30:11 +0100 (CET)
Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by am.mirrors.kernel.org (Postfix) with ESMTPS id 5B4B71F22D96
for <steffen.klassert@secunet.com>; Fri, 29 Mar 2024 22:30:11 +0000 (UTC)
Received: from localhost.localdomain (localhost.localdomain [127.0.0.1])
by smtp.subspace.kernel.org (Postfix) with ESMTP id 5B38413DDA5;
Fri, 29 Mar 2024 22:29:56 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org;
dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="af3tEf4r"
X-Original-To: netdev@vger.kernel.org
Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by smtp.subspace.kernel.org (Postfix) with ESMTPS id BF6BB28DC1
for <netdev@vger.kernel.org>; Fri, 29 Mar 2024 22:29:55 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201
ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;
t=1711751395; cv=none; b=r8+B1IFFag2HuI6zBBZXeH+ixu4+v7LcY5wOF3/6wgJ223E0kn3xcKcwo+b9S0QAED6F64X45+Ly5CTR1T3QpysOskVw+gmCEHA7C6kqyn9w3eNJ9i4Hl/Myvb/UKIYrlUrLJA2ZIcn/zPzyZPRsgS1BxBM9vsbq2bHqgBZeDjM=
ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org;
s=arc-20240116; t=1711751395; c=relaxed/simple;
bh=YclD2gFKAd0KYU/nqrMwp6tntz6Bp0xkpGNnD7iuj3c=;
h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References:
MIME-Version:Content-Type; b=rJ7Bn5B+eJPtxb4RNqsOXTzMjoxUUJ5pI/JOpQlNhT4ZcDDv6O01CZ1g3k27TriDuD2V9f4K/PGRphgNiz/gM/TFCcH5mAojrujO3pTOIJTI+aIIUz1rLn0diYOJV7K7HUs8cBglYDPH5ri6aPJNGmrNMWJbh0ZerjwDrcQhuoc=
ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=af3tEf4r; arc=none smtp.client-ip=10.30.226.201
Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2A38DC433F1;
Fri, 29 Mar 2024 22:29:55 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;
s=k20201202; t=1711751395;
bh=YclD2gFKAd0KYU/nqrMwp6tntz6Bp0xkpGNnD7iuj3c=;
h=Date:From:To:Cc:Subject:In-Reply-To:References:From;
b=af3tEf4riCb4f2NQ149pjvDrIXXmmP43YUOkyHbXZ+M94QTqDI0JCGEF6C9SwDi2v
UbNo6lur4NhXefe9RSrYlvWkEgyygoEXlsnzAgBuwTthmMcxP2nKYOexYi7y8EYgAU
s+LYfSGZY1szJRSJJk68i3GvMqw/Vxj3slvg7t75/MisPpwS+jb6RoyLsnYv0RKoVL
12qu5ji4XYH50ruUZsUfcfxQseOwTzwtSilm9SNsMlGhgFPnOmb3sh5+EZ9kkw1axQ
GhY5mxcMFxbnq+OPRpafTOvZpCjxq7fMQ4RgncWl/e6+tXnFeaTluLTysE2h8/dy/H
aDL1nOLZRHWXA==
Date: Fri, 29 Mar 2024 15:29:54 -0700
From: Jakub Kicinski <kuba@kernel.org>
To: Wojciech Drewek <wojciech.drewek@intel.com>
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
simon.horman@corigine.com, anthony.l.nguyen@intel.com, edumazet@google.com,
pabeni@redhat.com, idosch@nvidia.com, przemyslaw.kitszel@intel.com,
marcin.szycik@linux.intel.com
Subject: Re: [PATCH net-next 2/3] ethtool: Introduce max power support
Message-ID: <20240329152954.26a7ce75@kernel.org>
In-Reply-To: <20240329092321.16843-3-wojciech.drewek@intel.com>
References: <20240329092321.16843-1-wojciech.drewek@intel.com>
<20240329092321.16843-3-wojciech.drewek@intel.com>
Precedence: bulk
X-Mailing-List: netdev@vger.kernel.org
List-Id: <netdev.vger.kernel.org>
List-Subscribe: <mailto:netdev+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:netdev+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Return-Path: netdev+bounces-83478-steffen.klassert=secunet.com@vger.kernel.org
X-MS-Exchange-Organization-OriginalArrivalTime: 29 Mar 2024 22:30:12.5869
(UTC)
X-MS-Exchange-Organization-Network-Message-Id: b874bb8e-c3ad-4cd8-d138-08dc503fcc9e
X-MS-Exchange-Organization-OriginalClientIPAddress: 62.96.220.36
X-MS-Exchange-Organization-OriginalServerIPAddress: 10.53.40.202
X-MS-Exchange-Organization-Cross-Premises-Headers-Processed: cas-essen-02.secunet.de
X-MS-Exchange-Organization-OrderedPrecisionLatencyInProgress: LSRV=mbx-essen-01.secunet.de:TOTAL-HUB=0.416|SMR=0.329(SMRDE=0.004|SMRC=0.325(SMRCL=0.104|X-SMRCR=0.325))|CAT=0.086(CATRESL=0.022
(CATRESLP2R=0.018)|CATORES=0.058(CATRS=0.058(CATRS-Transport Rule
Agent=0.001|CATRS-Index Routing Agent=0.056
))|CATORT=0.001(CATRT=0.001));2024-03-29T22:30:13.029Z
X-MS-Exchange-Forest-ArrivalHubServer: mbx-essen-01.secunet.de
X-MS-Exchange-Organization-AuthSource: cas-essen-02.secunet.de
X-MS-Exchange-Organization-AuthAs: Anonymous
X-MS-Exchange-Organization-FromEntityHeader: Internet
X-MS-Exchange-Organization-OriginalSize: 12132
X-MS-Exchange-Organization-HygienePolicy: Standard
X-MS-Exchange-Organization-MessageLatency: SRV=cas-essen-02.secunet.de:TOTAL-FE=25.002|SMR=0.025(SMRPI=0.022(SMRPI-FrontendProxyAgent=0.022))|SMS=0.002
X-MS-Exchange-Organization-Recipient-Limit-Verified: True
X-MS-Exchange-Organization-TotalRecipientCount: 1
X-MS-Exchange-Organization-Rules-Execution-History: 0b0cf904-14ac-4724-8bdf-482ee6223cf2%%%fd34672d-751c-45ae-a963-ed177fcabe23%%%d8080257-b0c3-47b4-b0db-23bc0c8ddb3c%%%95e591a2-5d7d-4afa-b1d0-7573d6c0a5d9%%%f7d0f6bc-4dcc-4876-8c5d-b3d6ddbb3d55%%%16355082-c50b-4214-9c7d-d39575f9f79b
X-MS-Exchange-Forest-RulesExecuted: mbx-essen-01
X-MS-Exchange-Organization-RulesExecuted: mbx-essen-01
X-MS-Exchange-Forest-IndexAgent-0: AQ0CZW4AAWoMAAAPAAADH4sIAAAAAAAEAK1YCXPbxhVe3odEyXKcZH
J2k6a2KJG0rlqybNnWxGqijmx5ZLVpppPhgMSSQgQCLACaUpP8tP63
vmNBAiRIyW0wGnGx+659x7dv8Z/NU0f+xbNqcuuxfGV4cmtja0dubu
xvbe9vbcj1jc2NDfmD+3PbUu0L+dJTQ3Uph54bqP1K+Zl86/aU7Lnm
wFa+HPhKOq7jB4ZjGp4p++5QedJW75TtN+Sh+fPAD6QKLgLXtZGZ+a
TV69uqp5zACCzXkYEr/UG/73qBdNRQGkHgWa1BAPKDCyOQQ8u2pWHb
7hD1eSgHONoXhtMFS4wrqzfoseYGrOHyoWkCq6cUyeuqICKTNnF0/v
356elJ87D56vTl306Omq8O/9F8c/rD0Vnz7dG5XAU9puy4nvSR15dD
ZdtVWZftgeeB2fY1LqAgGTdAWrCbCyXbRldVypXyZlUePzDlpVJ9mp
/Syzpl31Md66om2+BLyw9IRQMFbFXlOfCRVT3jElxiBdJ3B44pbetS
ScORRhud2JDfwnioZNcFhwUXlTLYNkMdbFY+l6ferPWT41fH589Jwo
/Kr80lIyp6Ziu7meb4NdEM3YFt6u2BS4LgGrbjmhjuniJ/JIXu+LUW
c3hyAr8vIU49y4kEhXIHAsrB4bBhhKSnMOlgpXUtTfXOaqsbkiOiIR
b299MwStGhSwnq/x8J2p1IUKqKGzIT/YnjrvVOOfKdYQ+UXO1BlVlD
MMOv3mDA2RGaUIe9oeFxLTATWE5Xa2A7TNUxBnbAivT2z8AXACxmvX
W9jyDUBvPe/vu6bV3Kpz16bfj0+sK2nMFVw3ICZTfabo+431pdB3jd
Tof4J8Hq6VBPNEyaeBHnrtfrZJjltO2BqR6ShocapRoXMvb8Kjd35T
o+E1wDo2/FWZuOCmDiEkT8KuUOcBEDzIYUDxn/Gm05+fwqd3dYTeKj
dUdFjZVNiZJbcp0ZdmTHQpzmtDBrcm8XNgAgipDhr65XawD8ECBb8U
S9quNjWp0OOKoLaGM8nOWo1qwVlGA5prqSnW2j86i1vdl+3NlsNHZ3
lLn3+PHuzs62AYo3Hu3s6IDM1oIE4IH5yl68kPU/b27UYDPr9LuHU3
7gDdqj86fJzm8CFntuT/7CybkmX/Rd22pDGiHQchYDJQ5xWioHyqzN
FYxFc+HCkYaVhy99e9CtQ+rquI5EogAW6PaVR6ecYUeFu50kfvn2gh
CwpTBuNikNZUqNH9L0oGw9X8LBicU/UuCT+nVSbznN/tBraljan4uH
86CQpRlXE9LmYN+tpQFSTEqKna7xo3SCmaBnHxEocD01QpjYNrXjHt
JvcioQXbNveEbPDxMCH+WAkERaDF5TZwb/PLktGwX+CW8Fn8H2lpwI
1ORq3PEzVsET0ZU9GXPSEzw1W2CQfM7nZ2JlzwWz1q3IxjW/3dl7vL
G7Yxgbj9qNRme3be7sPGptGXvbM2v+JsFxALiJGtFgb2+r9kiu4w+A
AsxQcCIxnjrgvj86fHl0VpuE04drALtQ8nUJpEzTXAvTam5Tc/oShy
fH3/5YCyVBdG7JWZuwYcS5nswZ6w9qMU7Ilfmskw1U7fask53R/8JK
/URtaquVcsc2umHaYkx3dymkj2q7jO4Ae23IuUDDZxMxE7K+b1+v6n
r3L5utAeT7mn/Zqo39nvhg7x1EgMLRspqmERhyjcctw1fUH0VTKcap
bYmy0v8DqTd+dvTm5eH54WpE4ERlB6rXp7oNen1o1PGVm2K4ELnQCg
+g1aHuH1cQ/QeAgQCXYEegrgI8XDQCauwLHUWvvDWABg1RqxNb/lfT
giMPN8yjWrizroJ1XsP/U264BcYyzh/IX3578v58UORDYqsnxouY3b
4v1+DftHhAiKY+QdfgF4wIN1h/Bu8JDDb25HJtLWgBMRPijB+J1m1t
wCT1YjhNkAxbfaIbLn6Q9QBPzPqziLSQhvc98kVDn0MHaGoTeoHmYG
81aP3zZjz6qRqxDewCEaCm/qw7Sgrt/nanuwrW1OR9eq9pN0CSGe3L
qBCrI1dR0FO5UZ2uM1gZeE7ogfFWkGt6Owf6Ms/v1bHPxqSRs0+G5M
nnYYK3otKZckzORTDoQ8mqJtTi6v1EpbUxx9STFIEYOv8E7gQvR6se
HYFTWE6V8jE0PnCY4W2+DZd6/MiCBDcLrsoL5UG3dKJ8XyrPgy4Vel
1HUb/aGRBOGKZphe3i9C7iEYm6+dmEm3VTUmUESPbF6xM0q3l0Btn3
9rvVaPLU5NdvPPedZUKTyDdQy5cXVhc2gF99nFFrqPV8XSVEjEtsHp
6fn61W8bbZd7HGjIB6xpZhji/UifucyMz60fHrvx+ekIqjs8PX3x09
T+L6TSpbByPZSU9DJ8X7ut/VSTZhKPtI97tRH916p1ObG0/9Nq8quH
CihcEz82pCJpfFNDBNl0a8IveSCpIa3Tnq5xcOtR8RxVh/nmpfuo7+
wuHzpcxxA7yYja47HfzyObrJaVs4TH6lbECpIfOq1YB7XQ8vjXhkw9
2G72/SdJXvPAhGXz7pruM6HatbjeUeJttXaNpMWN0IQXXMMMNH8v79
ZBDVwPOq12vI790hphl9eu26jmPIoetdPqfbrLYdlruu/syjvwGFkq
BpceUQeB/4kfscbwzgx4Q7Ln98wk+5KEhd9VUbr4okyzGhAOd/+zqQ
G3Np+PPUgdwkU2jf5NC/DvxgHweh+27GVE4N8NqY6eZ8IiBGPYeOKa
/dAWE4pIWFGYzI7kMadejaSj40nGuw0rDJzYlgNR8hXk1+gAOJpk5d
UK3Tlj4Yanj0jR4YYvVUImBEs2oKD0YkYcvg39AyYKbNbhvGjQF2Dt
Dr4mhfbjIJqMUPX/1hveXCbb9NDa0QaZHJiFw6JRbgD8fZbCpXTImi
EMVUKSdETuSLopQXhZwoZ0WuIIpAsyAWYT4vijBfEAvwf0ksgyheLY
sFoIQ/oMmKIswTWTEnKrAKk/CfyJYLKfEl6BFpIAiXkJEJgBHYS6Kc
ESViz6e1hcAiUjwmYpDzcSgnJwpg2GJqJaTJsWTgJXtyZE+R5ZOKEr
B/OmYHG5AYJj+PTObEAoiamlye4q1MzRSJEtxVYO1p0vh1fOMFUeY9
FshjsGUgBs+TteX3pK+wA9/Lz3mxGHMymcrE4FIWIuNCcqEBUTPodb
GI6QTGZPMUjnxKfIEDdhr6BGYqJAdoKMGKwPKJzjQguDMSm0qtpDFR
l2HMSQWWLIsiLeUoyiXMAcrGVOoeEX/DM0RzD2lSJaJME3uJaDLl1D
IPSuKDLBkDBGO9Ih1/RdWLYmkxVckLkRdL8dVS7DUFpoK4HMpPZcZj
SgOw/0OyNi9WOBU5M2GGIzhKbMp2UdBJeydLtcPVNwpQhABlcrmlxS
IXHdOUxb0MOi1LbinFQ5aNZM7y7ckWdILlmIzqNBtSVjIY3yxtrcCB
ILK7vP3PyFQqirtZKisZzrAfwDM8/0lkPgt6QebYwruJr6FhS2jM2A
yAr8VolCdzKQz31Hxy3KfISsnzYSZQEqajmZCFJI8AV158mk1BoaXo
9cNoyYduWU5TGX4Wm7+XTi7wOxmSP4UbN8xP1fgdwKtsCjwvMqks72
I8pmABJZTzCuEbA10aczJD6fpHnoH936Hah0kYl3HwIS8xLy9l6XWJ
zwtNdocyCmpc5wanIvxBIfChQ9i+SEtFVpTRyIN6l+gVjcFzDQcIxR
AwFIJmTOUk/E+aRBYQtZQLq3gBixfqdAlVo7VZKuexIl7KY9WgLnDm
NC/nKtX7SoaQOVxdCTWC2BLLHx1bSenxAbMD5T3afnjUIoDTru+msU
DyFLh75C6A1mxES5nMQ70V5k1liiKlhTDZeGYpah4xfsTOJ8m5JCF/
YMunchUGy2ibKDNEM4zw6TCDhbMlFx4ouN+S+JK3M8f4RZopiG/SmB
iZ0SlJ/ikVaSMModztFABGUuKDGGWBe4mRkSVxn7AXGqEvCJZ/Hxs4
rGTDNyR/J4eW5PlIjTj/TzPyAQB5OYdARCUZt6pMNuSEZN6pqodyK4
2k5fVJrWttdPRksMyzGZ32WcrnXAjgGvZpFTAKlj7KpwqUe8szjCkl
+a3MLRzrZZTgcMeiEDmPOHxwHpUJH0bNDB9eWQLnyXTSRZHkirDbJH
VwiKyMcnLKaR/PyNXSKMRF6tNGtUk49kliGvCpmtGHF0dw7HlmDP35
+czC10mS4/InmOKdjnAA0zvPt4D/AsaXde/OJAAAAQrOAjw/eG1sIH
ZlcnNpb249IjEuMCIgZW5jb2Rpbmc9InV0Zi0xNiI/Pg0KPEVtYWls
U2V0Pg0KICA8VmVyc2lvbj4xNS4wLjAuMDwvVmVyc2lvbj4NCiAgPE
VtYWlscz4NCiAgICA8RW1haWwgU3RhcnRJbmRleD0iMTE4MyI+DQog
ICAgICA8RW1haWxTdHJpbmc+bWFyY2luLnN6eWNpa0BsaW51eC5pbn
RlbC5jb208L0VtYWlsU3RyaW5nPg0KICAgIDwvRW1haWw+DQogICAg
PEVtYWlsIFN0YXJ0SW5kZXg9IjEyNDkiPg0KICAgICAgPEVtYWlsU3
RyaW5nPndvamNpZWNoLmRyZXdla0BpbnRlbC5jb208L0VtYWlsU3Ry
aW5nPg0KICAgIDwvRW1haWw+DQogIDwvRW1haWxzPg0KPC9FbWFpbF
NldD4BDIUHPD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0idXRm
LTE2Ij8+DQo8Q29udGFjdFNldD4NCiAgPFZlcnNpb24+MTUuMC4wLj
A8L1ZlcnNpb24+DQogIDxDb250YWN0cz4NCiAgICA8Q29udGFjdCBT
dGFydEluZGV4PSIxMTY4Ij4NCiAgICAgIDxQZXJzb24gU3RhcnRJbm
RleD0iMTE2OCI+DQogICAgICAgIDxQZXJzb25TdHJpbmc+TWFyY2lu
PC9QZXJzb25TdHJpbmc+DQogICAgICA8L1BlcnNvbj4NCiAgICAgID
xFbWFpbHM+DQogICAgICAgIDxFbWFpbCBTdGFydEluZGV4PSIxMTgz
Ij4NCiAgICAgICAgICA8RW1haWxTdHJpbmc+bWFyY2luLnN6eWNpa0
BsaW51eC5pbnRlbC5jb208L0VtYWlsU3RyaW5nPg0KICAgICAgICA8
L0VtYWlsPg0KICAgICAgPC9FbWFpbHM+DQogICAgICA8Q29udGFjdF
N0cmluZz5NYXJjaW4gU3p5Y2lrICZsdDttYXJjaW4uc3p5Y2lrQGxp
bnV4LmludGVsLmNvbTwvQ29udGFjdFN0cmluZz4NCiAgICA8L0Nvbn
RhY3Q+DQogICAgPENvbnRhY3QgU3RhcnRJbmRleD0iMTIzMiI+DQog
ICAgICA8UGVyc29uIFN0YXJ0SW5kZXg9IjEyMzIiPg0KICAgICAgIC
A8UGVyc29uU3RyaW5nPldvamNpZWNoIERyZXdlazwvUGVyc29uU3Ry
aW5nPg0KICAgICAgPC9QZXJzb24+DQogICAgICA8RW1haWxzPg0KIC
AgICAgICA8RW1haWwgU3RhcnRJbmRleD0iMTI0OSI+DQogICAgICAg
ICAgPEVtYWlsU3RyaW5nPndvamNpZWNoLmRyZXdla0BpbnRlbC5jb2
08L0VtYWlsU3RyaW5nPg0KICAgICAgICA8L0VtYWlsPg0KICAgICAg
PC9FbWFpbHM+DQogICAgICA8Q29udGFjdFN0cmluZz5Xb2pjaWVjaC
BEcmV3ZWsgJmx0O3dvamNpZWNoLmRyZXdla0BpbnRlbC5jb208L0Nv
bnRhY3RTdHJpbmc+DQogICAgPC9Db250YWN0Pg0KICA8L0NvbnRhY3
RzPg0KPC9Db250YWN0U2V0PgEOzwFSZXRyaWV2ZXJPcGVyYXRvciwx
MCwyO1JldHJpZXZlck9wZXJhdG9yLDExLDM7UG9zdERvY1BhcnNlck
9wZXJhdG9yLDEwLDE7UG9zdERvY1BhcnNlck9wZXJhdG9yLDExLDA7
UG9zdFdvcmRCcmVha2VyRGlhZ25vc3RpY09wZXJhdG9yLDEwLDc7UG
9zdFdvcmRCcmVha2VyRGlhZ25vc3RpY09wZXJhdG9yLDExLDA7VHJh
bnNwb3J0V3JpdGVyUHJvZHVjZXIsMjAsMjg=
X-MS-Exchange-Forest-IndexAgent: 1 4643
X-MS-Exchange-Forest-EmailMessageHash: 10B105DC
X-MS-Exchange-Forest-Language: en
X-MS-Exchange-Organization-Processed-By-Journaling: Journal Agent
On Fri, 29 Mar 2024 10:23:20 +0100 Wojciech Drewek wrote:
> Some modules use nonstandard power levels. Adjust ethtool
> module implementation to support new attributes that will allow user
> to change maximum power.
>
> Add three new get attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
> maximum power in the cage
1) I'd keep the ETHTOOL_A_MODULE_POWER_ prefix, consistently.
2) The _SET makes it sound like an action. Can we go with
ETHTOOL_A_MODULE_POWER_MAX ? Or ETHTOOL_A_MODULE_POWER_LIMIT?
Yes, ETHTOOL_A_MODULE_POWER_LIMIT
ETHTOOL_A_MODULE_POWER_MAX
ETHTOOL_A_MODULE_POWER_MIN
would sound pretty good to me.
> ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
> cage reported by device
> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
> cage reported by device
>
> Add two new set attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for get as well) - change
> maximum power in the cage to the given value (milliwatts)
> ETHTOOL_A_MODULE_MAX_POWER_RESET - reset maximum power setting to the
> default value
>
> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
> include/linux/ethtool.h | 17 +++++--
> include/uapi/linux/ethtool_netlink.h | 4 ++
> net/ethtool/module.c | 74 ++++++++++++++++++++++++++--
> net/ethtool/netlink.h | 2 +-
> 4 files changed, 87 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index f3af6b31c9f1..74ed8997443a 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -510,10 +510,18 @@ struct ethtool_module_eeprom {
> * @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.
> + * @min_pwr_allowed: minimum power allowed in the cage reported by device
> + * @max_pwr_allowed: maximum power allowed in the cage reported by device
> + * @max_pwr_set: maximum power currently set in the cage
> + * @max_pwr_reset: restore default minimum power
> */
> struct ethtool_module_power_params {
> enum ethtool_module_power_mode_policy policy;
> enum ethtool_module_power_mode mode;
> + u32 min_pwr_allowed;
> + u32 max_pwr_allowed;
> + u32 max_pwr_set;
> + u8 max_pwr_reset;
bool ?
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 3f89074aa06c..f7cd446b2a83 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -882,6 +882,10 @@ enum {
> ETHTOOL_A_MODULE_HEADER, /* nest - _A_HEADER_* */
> ETHTOOL_A_MODULE_POWER_MODE_POLICY, /* u8 */
> ETHTOOL_A_MODULE_POWER_MODE, /* u8 */
> + ETHTOOL_A_MODULE_MAX_POWER_SET, /* u32 */
> + ETHTOOL_A_MODULE_MIN_POWER_ALLOWED, /* u32 */
> + ETHTOOL_A_MODULE_MAX_POWER_ALLOWED, /* u32 */
> + ETHTOOL_A_MODULE_MAX_POWER_RESET, /* u8 */
flag ?
> @@ -77,6 +86,7 @@ static int module_fill_reply(struct sk_buff *skb,
> const struct ethnl_reply_data *reply_base)
> {
> const struct module_reply_data *data = MODULE_REPDATA(reply_base);
> + u32 temp;
tmp ? temp sounds too much like temperature in context of power
> static int
> ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
> {
> struct ethtool_module_power_params power = {};
> struct ethtool_module_power_params power_new;
> - const struct ethtool_ops *ops;
> struct net_device *dev = req_info->dev;
> struct nlattr **tb = info->attrs;
> + const struct ethtool_ops *ops;
> int ret;
> + bool mod;
>
> ops = dev->ethtool_ops;
>
> - power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]);
> ret = ops->get_module_power_cfg(dev, &power, info->extack);
> if (ret < 0)
> return ret;
>
> - if (power_new.policy == power.policy)
> + power_new.max_pwr_set = power.max_pwr_set;
> + power_new.policy = power.policy;
> +
> + ethnl_update_u32(&power_new.max_pwr_set,
> + tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod);
> + if (mod) {
I think we can use if (tb[ETHTOOL_A_MODULE_MAX_POWER_SET]) here
Less error prone for future additions.
> + if (power_new.max_pwr_set > power.max_pwr_allowed) {
> + NL_SET_ERR_MSG(info->extack, "Provided value is higher than maximum allowed");
NL_SET_ERR_MSG_ATTR() to point at the bad attribute.
> + return -EINVAL;
ERANGE?
> + } else if (power_new.max_pwr_set < power.min_pwr_allowed) {
> + NL_SET_ERR_MSG(info->extack, "Provided value is lower than minimum allowed");
> + return -EINVAL;
> + }
> + }
> +
> + ethnl_update_policy(&power_new.policy,
> + tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], &mod);
> + ethnl_update_u8(&power_new.max_pwr_reset,
> + tb[ETHTOOL_A_MODULE_MAX_POWER_RESET], &mod);
I reckon reset should not be allowed if none of the max_pwr values
are set (i.e. most likely driver doesn't support the config)?
> + if (!mod)
> return 0;
>
> + if (power_new.max_pwr_reset && power_new.max_pwr_set) {
Mmm. How is that gonna work? The driver is going to set max_pwr_set
to what's currently configured. So the user is expected to send
ETHTOOL_A_MODULE_MAX_POWER_SET = 0
ETHTOOL_A_MODULE_MAX_POWER_RESET = 1
to reset?
Just:
if (tb[ETHTOOL_A_MODULE_MAX_POWER_RESET] &&
tb[ETHTOOL_A_MODULE_MAX_POWER_SET])
And you can validate this before doing any real work.
> + NL_SET_ERR_MSG(info->extack, "Maximum power set and reset cannot be used at the same time");
> + return 0;
> + }
> +
> ret = ops->set_module_power_cfg(dev, &power_new, info->extack);
> return ret < 0 ? ret : 1;
> }
--
pw-bot: cr
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-03-29 22:29 ` Jakub Kicinski
2024-03-29 22:29 ` Jakub Kicinski
@ 2024-04-02 11:25 ` Wojciech Drewek
2024-04-02 14:34 ` Jakub Kicinski
1 sibling, 1 reply; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-02 11:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, intel-wired-lan, anthony.l.nguyen, edumazet, pabeni,
idosch, przemyslaw.kitszel, marcin.szycik
On 29.03.2024 23:29, Jakub Kicinski wrote:
> On Fri, 29 Mar 2024 10:23:20 +0100 Wojciech Drewek wrote:
>> Some modules use nonstandard power levels. Adjust ethtool
>> module implementation to support new attributes that will allow user
>> to change maximum power.
>>
>> Add three new get attributes:
>> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
>> maximum power in the cage
>
> 1) I'd keep the ETHTOOL_A_MODULE_POWER_ prefix, consistently.
>
> 2) The _SET makes it sound like an action. Can we go with
> ETHTOOL_A_MODULE_POWER_MAX ? Or ETHTOOL_A_MODULE_POWER_LIMIT?
> Yes, ETHTOOL_A_MODULE_POWER_LIMIT
> ETHTOOL_A_MODULE_POWER_MAX
> ETHTOOL_A_MODULE_POWER_MIN
> would sound pretty good to me.
Makes sense, although ETHTOOL_A_MODULE_POWER_LIMIT does not say if
it's max or min limit. What about:
ETHTOOL_A_MODULE_POWER_MAX_LIMIT
ETHTOOL_A_MODULE_POWER_UPPER_LIMIT
>
>> ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
>> cage reported by device
>> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
>> cage reported by device
>>
>> Add two new set attributes:
>> ETHTOOL_A_MODULE_MAX_POWER_SET (used for get as well) - change
>> maximum power in the cage to the given value (milliwatts)
>> ETHTOOL_A_MODULE_MAX_POWER_RESET - reset maximum power setting to the
>> default value
>>
>> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
>> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> ---
>> include/linux/ethtool.h | 17 +++++--
>> include/uapi/linux/ethtool_netlink.h | 4 ++
>> net/ethtool/module.c | 74 ++++++++++++++++++++++++++--
>> net/ethtool/netlink.h | 2 +-
>> 4 files changed, 87 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index f3af6b31c9f1..74ed8997443a 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -510,10 +510,18 @@ struct ethtool_module_eeprom {
>> * @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.
>> + * @min_pwr_allowed: minimum power allowed in the cage reported by device
>> + * @max_pwr_allowed: maximum power allowed in the cage reported by device
>> + * @max_pwr_set: maximum power currently set in the cage
>> + * @max_pwr_reset: restore default minimum power
>> */
>> struct ethtool_module_power_params {
>> enum ethtool_module_power_mode_policy policy;
>> enum ethtool_module_power_mode mode;
>> + u32 min_pwr_allowed;
>> + u32 max_pwr_allowed;
>> + u32 max_pwr_set;
>> + u8 max_pwr_reset;
>
> bool ?
Makes sense
>
>> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
>> index 3f89074aa06c..f7cd446b2a83 100644
>> --- a/include/uapi/linux/ethtool_netlink.h
>> +++ b/include/uapi/linux/ethtool_netlink.h
>> @@ -882,6 +882,10 @@ enum {
>> ETHTOOL_A_MODULE_HEADER, /* nest - _A_HEADER_* */
>> ETHTOOL_A_MODULE_POWER_MODE_POLICY, /* u8 */
>> ETHTOOL_A_MODULE_POWER_MODE, /* u8 */
>> + ETHTOOL_A_MODULE_MAX_POWER_SET, /* u32 */
>> + ETHTOOL_A_MODULE_MIN_POWER_ALLOWED, /* u32 */
>> + ETHTOOL_A_MODULE_MAX_POWER_ALLOWED, /* u32 */
>> + ETHTOOL_A_MODULE_MAX_POWER_RESET, /* u8 */
>
> flag ?
Agree
>
>> @@ -77,6 +86,7 @@ static int module_fill_reply(struct sk_buff *skb,
>> const struct ethnl_reply_data *reply_base)
>> {
>> const struct module_reply_data *data = MODULE_REPDATA(reply_base);
>> + u32 temp;
>
> tmp ? temp sounds too much like temperature in context of power
I'll change the name
>
>> static int
>> ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
>> {
>> struct ethtool_module_power_params power = {};
>> struct ethtool_module_power_params power_new;
>> - const struct ethtool_ops *ops;
>> struct net_device *dev = req_info->dev;
>> struct nlattr **tb = info->attrs;
>> + const struct ethtool_ops *ops;
>> int ret;
>> + bool mod;
>>
>> ops = dev->ethtool_ops;
>>
>> - power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]);
>> ret = ops->get_module_power_cfg(dev, &power, info->extack);
>> if (ret < 0)
>> return ret;
>>
>> - if (power_new.policy == power.policy)
>> + power_new.max_pwr_set = power.max_pwr_set;
>> + power_new.policy = power.policy;
>> +
>> + ethnl_update_u32(&power_new.max_pwr_set,
>> + tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod);
>> + if (mod) {
>
> I think we can use if (tb[ETHTOOL_A_MODULE_MAX_POWER_SET]) here
> Less error prone for future additions.
Yep, makes sense
>
>> + if (power_new.max_pwr_set > power.max_pwr_allowed) {
>> + NL_SET_ERR_MSG(info->extack, "Provided value is higher than maximum allowed");
>
> NL_SET_ERR_MSG_ATTR() to point at the bad attribute.
Sure
>
>> + return -EINVAL;
>
> ERANGE?
Agree
>
>> + } else if (power_new.max_pwr_set < power.min_pwr_allowed) {
>> + NL_SET_ERR_MSG(info->extack, "Provided value is lower than minimum allowed");
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + ethnl_update_policy(&power_new.policy,
>> + tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], &mod);
>> + ethnl_update_u8(&power_new.max_pwr_reset,
>> + tb[ETHTOOL_A_MODULE_MAX_POWER_RESET], &mod);
>
> I reckon reset should not be allowed if none of the max_pwr values
> are set (i.e. most likely driver doesn't support the config)?
Hmmm, I think we can allow to reset if the currently set limit is the default one.
Right now only the driver could catch such scenario because we don't have a parameter
that driver could use to inform the ethtool about the default value.
I hope that answers your question since I'm not 100% sure if that's what you asked about :)
>
>> + if (!mod)
>> return 0;
>>
>> + if (power_new.max_pwr_reset && power_new.max_pwr_set) {
>
> Mmm. How is that gonna work? The driver is going to set max_pwr_set
> to what's currently configured. So the user is expected to send
> ETHTOOL_A_MODULE_MAX_POWER_SET = 0
> ETHTOOL_A_MODULE_MAX_POWER_RESET = 1
> to reset?
Yes, that was my intention. Using both of those attributes at the same time is not allowed.
>
> Just:
>
> if (tb[ETHTOOL_A_MODULE_MAX_POWER_RESET] &&
> tb[ETHTOOL_A_MODULE_MAX_POWER_SET])
>
> And you can validate this before doing any real work.
Hmmm, makes sense
>
>> + NL_SET_ERR_MSG(info->extack, "Maximum power set and reset cannot be used at the same time");
>> + return 0;
>> + }
>> +
>> ret = ops->set_module_power_cfg(dev, &power_new, info->extack);
>> return ret < 0 ? ret : 1;
>> }
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-04-02 11:25 ` Wojciech Drewek
@ 2024-04-02 14:34 ` Jakub Kicinski
2024-04-03 10:19 ` Wojciech Drewek
0 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2024-04-02 14:34 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, intel-wired-lan, anthony.l.nguyen, edumazet, pabeni,
idosch, przemyslaw.kitszel, marcin.szycik
On Tue, 2 Apr 2024 13:25:07 +0200 Wojciech Drewek wrote:
> On 29.03.2024 23:29, Jakub Kicinski wrote:
> > On Fri, 29 Mar 2024 10:23:20 +0100 Wojciech Drewek wrote:
> >> Some modules use nonstandard power levels. Adjust ethtool
> >> module implementation to support new attributes that will allow user
> >> to change maximum power.
> >>
> >> Add three new get attributes:
> >> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
> >> maximum power in the cage
> >
> > 1) I'd keep the ETHTOOL_A_MODULE_POWER_ prefix, consistently.
> >
> > 2) The _SET makes it sound like an action. Can we go with
> > ETHTOOL_A_MODULE_POWER_MAX ? Or ETHTOOL_A_MODULE_POWER_LIMIT?
> > Yes, ETHTOOL_A_MODULE_POWER_LIMIT
> > ETHTOOL_A_MODULE_POWER_MAX
> > ETHTOOL_A_MODULE_POWER_MIN
> > would sound pretty good to me.
>
> Makes sense, although ETHTOOL_A_MODULE_POWER_LIMIT does not say if
> it's max or min limit. What about:
> ETHTOOL_A_MODULE_POWER_MAX_LIMIT
> ETHTOOL_A_MODULE_POWER_UPPER_LIMIT
Is it possible to "limit" min power? 🧐️
This is not HTB where "unused power" can go to the sibling cage...
> >> + } else if (power_new.max_pwr_set < power.min_pwr_allowed) {
> >> + NL_SET_ERR_MSG(info->extack, "Provided value is lower than minimum allowed");
> >> + return -EINVAL;
> >> + }
> >> + }
> >> +
> >> + ethnl_update_policy(&power_new.policy,
> >> + tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], &mod);
> >> + ethnl_update_u8(&power_new.max_pwr_reset,
> >> + tb[ETHTOOL_A_MODULE_MAX_POWER_RESET], &mod);
> >
> > I reckon reset should not be allowed if none of the max_pwr values
> > are set (i.e. most likely driver doesn't support the config)?
>
> Hmmm, I think we can allow to reset if the currently set limit is the default one.
> Right now only the driver could catch such scenario because we don't have a parameter
> that driver could use to inform the ethtool about the default value.
> I hope that answers your question since I'm not 100% sure if that's what you asked about :)
Let me put it differently. How do we know that the driver doesn't
support setting the power policy? AFAIU we assume driver supports
it when it reports min_pwr_allowed || max_pwr_allowed from get.
If that's not the case we should add a cap bit like
cap_link_lanes_supported.
So what I'm saying is that if driver doesn't support the feature,
we should error out if user space gave us any
tb[ETHTOOL_A_MODULE_MAX_POWER* attribute.
> >> + if (!mod)
> >> return 0;
> >>
> >> + if (power_new.max_pwr_reset && power_new.max_pwr_set) {
> >
> > Mmm. How is that gonna work? The driver is going to set max_pwr_set
> > to what's currently configured. So the user is expected to send
> > ETHTOOL_A_MODULE_MAX_POWER_SET = 0
> > ETHTOOL_A_MODULE_MAX_POWER_RESET = 1
> > to reset?
>
> Yes, that was my intention. Using both of those attributes at the same time is not allowed.
To be clear the code is:
ret = ops->get_module_power_cfg(dev, &power, info->extack);
if (ret < 0)
return ret;
power_new.max_pwr_set = power.max_pwr_set;
ethnl_update_u32(&power_new.max_pwr_set,
tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod);
// ...
if (power_new.max_pwr_reset && power_new.max_pwr_set) {
so if driver reports .max_pwr_set from get we may fall into this if
I think you got it but anyway..
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-04-02 14:34 ` Jakub Kicinski
@ 2024-04-03 10:19 ` Wojciech Drewek
2024-04-04 0:18 ` Jakub Kicinski
0 siblings, 1 reply; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-03 10:19 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, intel-wired-lan, anthony.l.nguyen, edumazet, pabeni,
idosch, przemyslaw.kitszel, marcin.szycik
On 02.04.2024 16:34, Jakub Kicinski wrote:
> On Tue, 2 Apr 2024 13:25:07 +0200 Wojciech Drewek wrote:
>> On 29.03.2024 23:29, Jakub Kicinski wrote:
>>> On Fri, 29 Mar 2024 10:23:20 +0100 Wojciech Drewek wrote:
>>>> Some modules use nonstandard power levels. Adjust ethtool
>>>> module implementation to support new attributes that will allow user
>>>> to change maximum power.
>>>>
>>>> Add three new get attributes:
>>>> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
>>>> maximum power in the cage
>>>
>>> 1) I'd keep the ETHTOOL_A_MODULE_POWER_ prefix, consistently.
>>>
>>> 2) The _SET makes it sound like an action. Can we go with
>>> ETHTOOL_A_MODULE_POWER_MAX ? Or ETHTOOL_A_MODULE_POWER_LIMIT?
>>> Yes, ETHTOOL_A_MODULE_POWER_LIMIT
>>> ETHTOOL_A_MODULE_POWER_MAX
>>> ETHTOOL_A_MODULE_POWER_MIN
>>> would sound pretty good to me.
>>
>> Makes sense, although ETHTOOL_A_MODULE_POWER_LIMIT does not say if
>> it's max or min limit. What about:
>> ETHTOOL_A_MODULE_POWER_MAX_LIMIT
>> ETHTOOL_A_MODULE_POWER_UPPER_LIMIT
>
> Is it possible to "limit" min power? 🧐️
Right, I'll stick with ETHTOOL_A_MODULE_POWER_LIMIT
> This is not HTB where "unused power" can go to the sibling cage...
>>>> + } else if (power_new.max_pwr_set < power.min_pwr_allowed) {
>>>> + NL_SET_ERR_MSG(info->extack, "Provided value is lower than minimum allowed");
>>>> + return -EINVAL;
>>>> + }
>>>> + }
>>>> +
>>>> + ethnl_update_policy(&power_new.policy,
>>>> + tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], &mod);
>>>> + ethnl_update_u8(&power_new.max_pwr_reset,
>>>> + tb[ETHTOOL_A_MODULE_MAX_POWER_RESET], &mod);
>>>
>>> I reckon reset should not be allowed if none of the max_pwr values
>>> are set (i.e. most likely driver doesn't support the config)?
>>
>> Hmmm, I think we can allow to reset if the currently set limit is the default one.
>> Right now only the driver could catch such scenario because we don't have a parameter
>> that driver could use to inform the ethtool about the default value.
>> I hope that answers your question since I'm not 100% sure if that's what you asked about :)
>
> Let me put it differently. How do we know that the driver doesn't
> support setting the power policy? AFAIU we assume driver supports
> it when it reports min_pwr_allowed || max_pwr_allowed from get.
> If that's not the case we should add a cap bit like
> cap_link_lanes_supported.
>
> So what I'm saying is that if driver doesn't support the feature,
> we should error out if user space gave us any
> tb[ETHTOOL_A_MODULE_MAX_POWER* attribute
Ok, I get now. Normally checking ops->set_module_power_cfg pointer would
be enough but here we have two features in one callback. Right now I assumed
that the driver will check which attributes were provided by the userspace
and will print error (like I did in ice_set_module_power_cfg) if the driver
does not support given attribute.
You're saying that if min_pwr_allowed or max_pwr_allowed taken from get op
are 0 than we should not allow to set max_pwr_reset and max_pwr_set?
And similarly if policy was 0 than we should not allow to set it?
I can implement whichever option you prefer.
>
>>>> + if (!mod)
>>>> return 0;
>>>>
>>>> + if (power_new.max_pwr_reset && power_new.max_pwr_set) {
>>>
>>> Mmm. How is that gonna work? The driver is going to set max_pwr_set
>>> to what's currently configured. So the user is expected to send
>>> ETHTOOL_A_MODULE_MAX_POWER_SET = 0
>>> ETHTOOL_A_MODULE_MAX_POWER_RESET = 1
>>> to reset?
>>
>> Yes, that was my intention. Using both of those attributes at the same time is not allowed.
>
> To be clear the code is:
>
> ret = ops->get_module_power_cfg(dev, &power, info->extack);
> if (ret < 0)
> return ret;
>
> power_new.max_pwr_set = power.max_pwr_set;
>
> ethnl_update_u32(&power_new.max_pwr_set,
> tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod);
> // ...
>
> if (power_new.max_pwr_reset && power_new.max_pwr_set) {
>
> so if driver reports .max_pwr_set from get we may fall into this if
> I think you got it but anyway..
Oh, I see, I'll check the attributes at the beginning before reading them
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-04-03 10:19 ` Wojciech Drewek
@ 2024-04-04 0:18 ` Jakub Kicinski
2024-04-04 12:19 ` Wojciech Drewek
0 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2024-04-04 0:18 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, intel-wired-lan, anthony.l.nguyen, edumazet, pabeni,
idosch, przemyslaw.kitszel, marcin.szycik
On Wed, 3 Apr 2024 12:19:57 +0200 Wojciech Drewek wrote:
> You're saying that if min_pwr_allowed or max_pwr_allowed taken from get op
> are 0 than we should not allow to set max_pwr_reset and max_pwr_set?
Yes, return -EOPNOTSUPP and point extack at whatever max_pwr attr user
sent. If driver doesn't return any bounds from get() it must not support
the configuration.
> And similarly if policy was 0 than we should not allow to set it?
You mean the limit? I'm not as sure about this one. We can either
treat 0 as "unset" or as unsupported. Not sure what makes more sense
for this case.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-04-04 0:18 ` Jakub Kicinski
@ 2024-04-04 12:19 ` Wojciech Drewek
0 siblings, 0 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-04 12:19 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, intel-wired-lan, anthony.l.nguyen, edumazet, pabeni,
idosch, przemyslaw.kitszel, marcin.szycik
On 04.04.2024 02:18, Jakub Kicinski wrote:
> On Wed, 3 Apr 2024 12:19:57 +0200 Wojciech Drewek wrote:
>> You're saying that if min_pwr_allowed or max_pwr_allowed taken from get op
>> are 0 than we should not allow to set max_pwr_reset and max_pwr_set?
>
> Yes, return -EOPNOTSUPP and point extack at whatever max_pwr attr user
> sent. If driver doesn't return any bounds from get() it must not support
> the configuration.
Ok
>
>> And similarly if policy was 0 than we should not allow to set it?
>
> You mean the limit? I'm not as sure about this one. We can either
> treat 0 as "unset" or as unsupported. Not sure what makes more sense
> for this case.
I was talking about ETHTOOL_A_MODULE_POWER_MODE_POLICY, attribute that
is already present in ethtool.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-03-29 9:23 ` [PATCH net-next 2/3] ethtool: Introduce max power support Wojciech Drewek
2024-03-29 22:29 ` Jakub Kicinski
@ 2024-03-30 22:14 ` Andrew Lunn
2024-03-30 22:14 ` Andrew Lunn
2024-04-03 9:50 ` Wojciech Drewek
1 sibling, 2 replies; 34+ messages in thread
From: Andrew Lunn @ 2024-03-30 22:14 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet,
kuba, pabeni, idosch, przemyslaw.kitszel, marcin.szycik
On Fri, Mar 29, 2024 at 10:23:20AM +0100, Wojciech Drewek wrote:
> Some modules use nonstandard power levels. Adjust ethtool
> module implementation to support new attributes that will allow user
> to change maximum power.
>
> Add three new get attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
> maximum power in the cage
> ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
> cage reported by device
> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
> cage reported by device
I'm confused. The cage has two power pins, if you look at the table
here:
https://www.embrionix.com/resource/how-to-design-with-video-SFP
There is VccT and VccR. I would expect there is a power regulator
supplying these pins. By default, you can draw 1W from that
regulator. The board however might be designed to support more power,
so those regulators could supply more power. And the board has also
been designed to dump the heat if more power is consumed.
So, ETHTOOL_A_MODULE_MIN_POWER_ALLOWED is about the minimum power that
regulator can supply? Does that make any sense?
ETHTOOL_A_MODULE_MAX_POWER_ALLOWED is about the maximum power the
regulator can supply and the cooling system can dump heat?
Then what does ETHTOOL_A_MODULE_MAX_POWER_SET mean? power in the cage?
The cage is passive. It does not consume power. It is the module which
does. Is this telling the module it can consume up to this amount of
power?
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-03-30 22:14 ` Andrew Lunn
@ 2024-03-30 22:14 ` Andrew Lunn
2024-04-03 9:50 ` Wojciech Drewek
1 sibling, 0 replies; 34+ messages in thread
From: Andrew Lunn @ 2024-03-30 22:14 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet,
kuba, pabeni, idosch, przemyslaw.kitszel, marcin.szycik
On Fri, Mar 29, 2024 at 10:23:20AM +0100, Wojciech Drewek wrote:
> Some modules use nonstandard power levels. Adjust ethtool
> module implementation to support new attributes that will allow user
> to change maximum power.
>
> Add three new get attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
> maximum power in the cage
> ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
> cage reported by device
> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
> cage reported by device
I'm confused. The cage has two power pins, if you look at the table
here:
https://www.embrionix.com/resource/how-to-design-with-video-SFP
There is VccT and VccR. I would expect there is a power regulator
supplying these pins. By default, you can draw 1W from that
regulator. The board however might be designed to support more power,
so those regulators could supply more power. And the board has also
been designed to dump the heat if more power is consumed.
So, ETHTOOL_A_MODULE_MIN_POWER_ALLOWED is about the minimum power that
regulator can supply? Does that make any sense?
ETHTOOL_A_MODULE_MAX_POWER_ALLOWED is about the maximum power the
regulator can supply and the cooling system can dump heat?
Then what does ETHTOOL_A_MODULE_MAX_POWER_SET mean? power in the cage?
The cage is passive. It does not consume power. It is the module which
does. Is this telling the module it can consume up to this amount of
power?
Andrew
X-sender: <netdev+bounces-83535-steffen.klassert=secunet.com@vger.kernel.org>
X-Receiver: <steffen.klassert@secunet.com> ORCPT=rfc822;steffen.klassert@secunet.com
X-CreatedBy: MSExchange15
X-HeloDomain: mbx-dresden-01.secunet.de
X-ExtendedProps: BQBjAAoAb2UFfe5Q3AgFADcAAgAADwA8AAAATWljcm9zb2Z0LkV4Y2hhbmdlLlRyYW5zcG9ydC5NYWlsUmVjaXBpZW50Lk9yZ2FuaXphdGlvblNjb3BlEQAAAAAAAAAAAAAAAAAAAAAADwA/AAAATWljcm9zb2Z0LkV4Y2hhbmdlLlRyYW5zcG9ydC5EaXJlY3RvcnlEYXRhLk1haWxEZWxpdmVyeVByaW9yaXR5DwADAAAATG93
X-Source: SMTP:Default MBX-ESSEN-02
X-SourceIPAddress: 10.53.40.199
X-EndOfInjectedXHeaders: 8772
Received: from mbx-dresden-01.secunet.de (10.53.40.199) by
mbx-essen-02.secunet.de (10.53.40.198) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.2507.37; Sat, 30 Mar 2024 23:14:25 +0100
Received: from b.mx.secunet.com (62.96.220.37) by cas-essen-01.secunet.de
(10.53.40.201) with Microsoft SMTP Server (version=TLS1_2,
cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.37 via Frontend
Transport; Sat, 30 Mar 2024 23:14:24 +0100
Received: from localhost (localhost [127.0.0.1])
by b.mx.secunet.com (Postfix) with ESMTP id C18BE2032C
for <steffen.klassert@secunet.com>; Sat, 30 Mar 2024 23:14:24 +0100 (CET)
X-Virus-Scanned: by secunet
X-Spam-Flag: NO
X-Spam-Score: -5.051
X-Spam-Level:
X-Spam-Status: No, score=-5.051 tagged_above=-999 required=2.1
tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1,
DKIM_VALID_AU=-0.1, HEADER_FROM_DIFFERENT_DOMAINS=0.249,
MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001,
SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: a.mx.secunet.com (amavisd-new);
dkim=pass (1024-bit key) header.d=lunn.ch
Received: from b.mx.secunet.com ([127.0.0.1])
by localhost (a.mx.secunet.com [127.0.0.1]) (amavisd-new, port 10024)
with ESMTP id GqE8n768s_on for <steffen.klassert@secunet.com>;
Sat, 30 Mar 2024 23:14:23 +0100 (CET)
Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=139.178.88.99; helo=sv.mirrors.kernel.org; envelope-from=netdev+bounces-83535-steffen.klassert=secunet.com@vger.kernel.org; receiver=steffen.klassert@secunet.com
DKIM-Filter: OpenDKIM Filter v2.11.0 b.mx.secunet.com 9123E2025D
Authentication-Results: b.mx.secunet.com;
dkim=pass (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b="WAQ3uGBN"
Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org [139.178.88.99])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by b.mx.secunet.com (Postfix) with ESMTPS id 9123E2025D
for <steffen.klassert@secunet.com>; Sat, 30 Mar 2024 23:14:23 +0100 (CET)
Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by sv.mirrors.kernel.org (Postfix) with ESMTPS id 9EBEC282BC5
for <steffen.klassert@secunet.com>; Sat, 30 Mar 2024 22:14:20 +0000 (UTC)
Received: from localhost.localdomain (localhost.localdomain [127.0.0.1])
by smtp.subspace.kernel.org (Postfix) with ESMTP id 8A8324CDE5;
Sat, 30 Mar 2024 22:14:16 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org;
dkim=pass (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b="WAQ3uGBN"
X-Original-To: netdev@vger.kernel.org
Received: from vps0.lunn.ch (vps0.lunn.ch [156.67.10.101])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by smtp.subspace.kernel.org (Postfix) with ESMTPS id 43C2641C93
for <netdev@vger.kernel.org>; Sat, 30 Mar 2024 22:14:14 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=156.67.10.101
ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;
t=1711836856; cv=none; b=ET1Z3thUny7dCCkpC8lbUebGMy5Plrtw6hvPlf/UJkksxBeLbmw1ghzEaCGB4mURAd074f3qtBzUYMarYvRF614ab6MmcXulApsm2/AWHxcSS4meNk4xemu+sLA9LGeMAe5fjb5xaN1coMfmUABfnU7im3jdIJCj5hwISMynSTc=
ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org;
s=arc-20240116; t=1711836856; c=relaxed/simple;
bh=htnwxaRYLoNkryWLJaBVSFOfS9tn6Oh5EBSghNzeRHE=;
h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:
Content-Type:Content-Disposition:In-Reply-To; b=q2reOn0VT70EDOh7dna04qpxZe84BLB7BiD9y/VeQ6ly9aFwPhVqy1eWaKESospD71beZBL20aEdIGypfjBPmMd5m04+WfJtonrC+U0P6N7eG8FeuxbCnVBE+GEUIlHWjCKZxDJEUKQTsJzVHLIsEGpD4f9dPMvsSvg7l/ZEbCs=
ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lunn.ch; spf=pass smtp.mailfrom=lunn.ch; dkim=pass (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b=WAQ3uGBN; arc=none smtp.client-ip=156.67.10.101
Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lunn.ch
Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=lunn.ch
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch;
s=20171124; h=In-Reply-To:Content-Disposition:Content-Type:MIME-Version:
References:Message-ID:Subject:Cc:To:From:Date:From:Sender:Reply-To:Subject:
Date:Message-ID:To:Cc:MIME-Version:Content-Type:Content-Transfer-Encoding:
Content-ID:Content-Description:Content-Disposition:In-Reply-To:References;
bh=E3M9C94KlYgWkxVnJ7peUnv4RFU6IKeHU/i44aG+ovM=; b=WAQ3uGBNwM5q1A2jNEp8fqa4y4
JAPFdkTelMrUfMAR1POls1sGXQYtQhs18rv/qsFXdzweD1Q+mSVouU8wxfgek9U/hyINTk4SSk4Kx
yGhGJ2J93d1KR7+sE78wwu9AikmLOwitEiZB3TRceJ7vybO9GqK7qDxwvkkMuc+q6+TM=;
Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2)
(envelope-from <andrew@lunn.ch>)
id 1rqgxm-00BkRL-2u; Sat, 30 Mar 2024 23:14:10 +0100
Date: Sat, 30 Mar 2024 23:14:10 +0100
From: Andrew Lunn <andrew@lunn.ch>
To: Wojciech Drewek <wojciech.drewek@intel.com>
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
simon.horman@corigine.com, anthony.l.nguyen@intel.com,
edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
idosch@nvidia.com, przemyslaw.kitszel@intel.com,
marcin.szycik@linux.intel.com
Subject: Re: [PATCH net-next 2/3] ethtool: Introduce max power support
Message-ID: <07572365-1c9f-4948-ad2f-4d56c6d4e4ab@lunn.ch>
References: <20240329092321.16843-1-wojciech.drewek@intel.com>
<20240329092321.16843-3-wojciech.drewek@intel.com>
Precedence: bulk
X-Mailing-List: netdev@vger.kernel.org
List-Id: <netdev.vger.kernel.org>
List-Subscribe: <mailto:netdev+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:netdev+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240329092321.16843-3-wojciech.drewek@intel.com>
Return-Path: netdev+bounces-83535-steffen.klassert=secunet.com@vger.kernel.org
X-MS-Exchange-Organization-OriginalArrivalTime: 30 Mar 2024 22:14:24.8293
(UTC)
X-MS-Exchange-Organization-Network-Message-Id: acc1b26e-2c68-4e88-39da-08dc5106c220
X-MS-Exchange-Organization-OriginalClientIPAddress: 62.96.220.37
X-MS-Exchange-Organization-OriginalServerIPAddress: 10.53.40.201
X-MS-Exchange-Organization-Cross-Premises-Headers-Processed: cas-essen-01.secunet.de
X-MS-Exchange-Organization-OrderedPrecisionLatencyInProgress: LSRV=cas-essen-01.secunet.de:TOTAL-FE=0.030|SMR=0.031(SMRPI=0.028(SMRPI-FrontendProxyAgent=0.028));2024-03-30T22:14:24.860Z
X-MS-Exchange-Forest-ArrivalHubServer: mbx-essen-02.secunet.de
X-MS-Exchange-Organization-AuthSource: cas-essen-01.secunet.de
X-MS-Exchange-Organization-AuthAs: Anonymous
X-MS-Exchange-Organization-OriginalSize: 8224
X-MS-Exchange-Organization-Transport-Properties: DeliveryPriority=Low
X-MS-Exchange-Organization-Prioritization: 2:ShadowRedundancy
X-MS-Exchange-Organization-IncludeInSla: False:ShadowRedundancy
On Fri, Mar 29, 2024 at 10:23:20AM +0100, Wojciech Drewek wrote:
> Some modules use nonstandard power levels. Adjust ethtool
> module implementation to support new attributes that will allow user
> to change maximum power.
>
> Add three new get attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
> maximum power in the cage
> ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
> cage reported by device
> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
> cage reported by device
I'm confused. The cage has two power pins, if you look at the table
here:
https://www.embrionix.com/resource/how-to-design-with-video-SFP
There is VccT and VccR. I would expect there is a power regulator
supplying these pins. By default, you can draw 1W from that
regulator. The board however might be designed to support more power,
so those regulators could supply more power. And the board has also
been designed to dump the heat if more power is consumed.
So, ETHTOOL_A_MODULE_MIN_POWER_ALLOWED is about the minimum power that
regulator can supply? Does that make any sense?
ETHTOOL_A_MODULE_MAX_POWER_ALLOWED is about the maximum power the
regulator can supply and the cooling system can dump heat?
Then what does ETHTOOL_A_MODULE_MAX_POWER_SET mean? power in the cage?
The cage is passive. It does not consume power. It is the module which
does. Is this telling the module it can consume up to this amount of
power?
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 2/3] ethtool: Introduce max power support
2024-03-30 22:14 ` Andrew Lunn
2024-03-30 22:14 ` Andrew Lunn
@ 2024-04-03 9:50 ` Wojciech Drewek
1 sibling, 0 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-03 9:50 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, intel-wired-lan, anthony.l.nguyen, edumazet, kuba, pabeni,
idosch, przemyslaw.kitszel, marcin.szycik
On 30.03.2024 23:14, Andrew Lunn wrote:
> On Fri, Mar 29, 2024 at 10:23:20AM +0100, Wojciech Drewek wrote:
>> Some modules use nonstandard power levels. Adjust ethtool
>> module implementation to support new attributes that will allow user
>> to change maximum power.
>>
>> Add three new get attributes:
>> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
>> maximum power in the cage
>> ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
>> cage reported by device
>> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
>> cage reported by device
>
> I'm confused. The cage has two power pins, if you look at the table
> here:
>
> https://www.embrionix.com/resource/how-to-design-with-video-SFP
>
> There is VccT and VccR. I would expect there is a power regulator
> supplying these pins. By default, you can draw 1W from that
> regulator. The board however might be designed to support more power,
> so those regulators could supply more power. And the board has also
> been designed to dump the heat if more power is consumed.
>
> So, ETHTOOL_A_MODULE_MIN_POWER_ALLOWED is about the minimum power that
> regulator can supply? Does that make any sense?
>
> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED is about the maximum power the
> regulator can supply and the cooling system can dump heat?
>
> Then what does ETHTOOL_A_MODULE_MAX_POWER_SET mean? power in the cage?
> The cage is passive. It does not consume power. It is the module which
> does. Is this telling the module it can consume up to this amount of
> power?
Right, all of those attributes describe restrictions for modules that
can be plugged into the given cage. ETHTOOL_A_MODULE_MAX_POWER_SET is
currently set maximum. The other two define the range of values that
ETHTOOL_A_MODULE_MAX_POWER_SET can take.
I hope that answers your question.
>
> Andrew
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 3/3] ice: Implement ethtool max power configuration
2024-03-29 9:23 [PATCH net-next 0/3] ethtool: Max power support Wojciech Drewek
2024-03-29 9:23 ` [PATCH net-next 1/3] ethtool: Make module API more generic Wojciech Drewek
2024-03-29 9:23 ` [PATCH net-next 2/3] ethtool: Introduce max power support Wojciech Drewek
@ 2024-03-29 9:23 ` Wojciech Drewek
2024-03-29 22:16 ` [PATCH net-next 0/3] ethtool: Max power support Jakub Kicinski
2024-03-30 21:57 ` Andrew Lunn
4 siblings, 0 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-03-29 9:23 UTC (permalink / raw)
To: netdev
Cc: intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet, kuba,
pabeni, idosch, przemyslaw.kitszel, marcin.szycik
Implement get_module_power_cfg and set_module_power_cfg ethtool ops.
Only a part of the parameters provided by those callbacks are going
to be supported, power policy is not on one of them.
Introduce new NVM module, Cage Max Power Override which allows
to change default max power values in the cages. This module
have an array of 8 words, each one of them is associated with
different cage. If ICE_AQC_NVM_CMPO_ENABLE bit is set then
firmware will use maximum power stored in the given word.
The overall sum of the powers in the board cannot exceed
board maximum, which is stored in EMP settings NVM module.
Before changing the maximum power check if the new value will not
broke this limit.
Minimum limit per cage is based on the type of the cage, SFP or
QSFP.
Maximum limit per cage is calculated with below formula:
max_power_per_board - (number_of_cages - 1) * min_power_per_cage
Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
drivers/net/ethernet/intel/ice/ice.h | 2 +
.../net/ethernet/intel/ice/ice_adminq_cmd.h | 21 +
drivers/net/ethernet/intel/ice/ice_common.c | 46 ++
drivers/net/ethernet/intel/ice/ice_common.h | 3 +
drivers/net/ethernet/intel/ice/ice_devlink.c | 14 +-
drivers/net/ethernet/intel/ice/ice_ethtool.c | 461 ++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_nvm.c | 2 +-
drivers/net/ethernet/intel/ice/ice_nvm.h | 3 +
drivers/net/ethernet/intel/ice/ice_type.h | 4 +
9 files changed, 550 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index c4127d5f2be3..ca145ce2b1eb 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -655,6 +655,8 @@ struct ice_pf {
struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
struct ice_dplls dplls;
struct device *hwmon_dev;
+
+ int split_cnt;
};
extern struct workqueue_struct *ice_lag_wq;
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 540c0bdca936..daf53e00a497 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1497,6 +1497,10 @@ struct ice_aqc_link_topo_addr {
struct ice_aqc_get_link_topo {
struct ice_aqc_link_topo_addr addr;
u8 node_part_num;
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_SFP_PLUS 0x11
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_SFP28 0x12
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_QSFP_PLUS 0x13
+#define ICE_AQC_GET_LINK_TOPO_NODE_NR_QSFP28 0x14
#define ICE_AQC_GET_LINK_TOPO_NODE_NR_PCA9575 0x21
#define ICE_AQC_GET_LINK_TOPO_NODE_NR_ZL30632_80032 0x24
#define ICE_AQC_GET_LINK_TOPO_NODE_NR_SI5383_5384 0x25
@@ -1664,6 +1668,23 @@ struct ice_aqc_nvm {
__le32 addr_low;
};
+#define ICE_AQC_NVM_CMPO_MOD_ID 0x153
+#define ICE_AQC_NVM_EMP_SETTINGS_MOD_ID 0x0F
+#define ICE_AQC_NVM_MAX_PWR_LIMIT_OFFSET 0x1A
+#define ICE_AQC_NVM_DFLT_MAX_PWR_MASK GENMASK(7, 0)
+#define ICE_AQC_NVM_BOARD_MAX_PWR_MASK GENMASK(15, 8)
+
+#define ICE_NUM_OF_CAGES 8
+
+#define ICE_AQC_NVM_CMPO_ENABLE BIT(8)
+#define ICE_AQC_NVM_CMPO_POWER_MASK GENMASK(7, 0)
+
+/* Cage Max Power override NVM module */
+struct ice_aqc_nvm_cmpo {
+ __le16 length;
+ __le16 cages_cfg[ICE_NUM_OF_CAGES];
+};
+
#define ICE_AQC_NVM_START_POINT 0
/* NVM Checksum Command (direct, 0x0706) */
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index f4ac3c30b124..081f6b6dbc30 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -571,6 +571,23 @@ static bool ice_is_media_cage_present(struct ice_port_info *pi)
NULL);
}
+int ice_get_port_cage_node(struct ice_hw *hw, u8 index,
+ u16 *node_handle, u8 *node_part_number)
+{
+ struct ice_aqc_get_link_topo cmd = {};
+
+ cmd.addr.topo_params.node_type_ctx =
+ FIELD_PREP(ICE_AQC_LINK_TOPO_NODE_TYPE_M,
+ ICE_AQC_LINK_TOPO_NODE_TYPE_CAGE);
+ cmd.addr.topo_params.node_type_ctx |=
+ FIELD_PREP(ICE_AQC_LINK_TOPO_NODE_CTX_M,
+ ICE_AQC_LINK_TOPO_NODE_CTX_GLOBAL);
+ cmd.addr.topo_params.index = index;
+
+ return ice_aq_get_netlist_node(hw, &cmd, node_part_number,
+ node_handle);
+}
+
/**
* ice_get_media_type - Gets media type
* @pi: port information structure
@@ -5723,6 +5740,29 @@ static bool ice_is_fw_api_min_ver(struct ice_hw *hw, u8 maj, u8 min, u8 patch)
return false;
}
+/**
+ * ice_is_fw_min_ver
+ * @hw: pointer to the hardware structure
+ * @maj: major version
+ * @min: minor version
+ * @patch: patch version
+ *
+ * Checks if the firmware is minimum version
+ */
+static bool ice_is_fw_min_ver(struct ice_hw *hw, u8 maj, u8 min, u8 patch)
+{
+ if (hw->fw_maj_ver > maj)
+ return true;
+ if (hw->fw_maj_ver == maj) {
+ if (hw->fw_min_ver > min)
+ return true;
+ if (hw->fw_min_ver == min && hw->fw_patch >= patch)
+ return true;
+ }
+
+ return false;
+}
+
/**
* ice_fw_supports_link_override
* @hw: pointer to the hardware structure
@@ -5736,6 +5776,12 @@ bool ice_fw_supports_link_override(struct ice_hw *hw)
ICE_FW_API_LINK_OVERRIDE_PATCH);
}
+bool ice_fw_supports_cmpo(struct ice_hw *hw)
+{
+ return ice_is_fw_min_ver(hw, ICE_FW_CMPO_MAJ, ICE_FW_CMPO_MIN,
+ ICE_FW_CMPO_PATCH);
+}
+
/**
* ice_get_link_default_override
* @ldo: pointer to the link default override struct
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index ffb22c7ce28b..c476fbf60400 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -96,6 +96,8 @@ bool ice_is_phy_rclk_in_netlist(struct ice_hw *hw);
bool ice_is_clock_mux_in_netlist(struct ice_hw *hw);
bool ice_is_cgu_in_netlist(struct ice_hw *hw);
bool ice_is_gps_in_netlist(struct ice_hw *hw);
+int ice_get_port_cage_node(struct ice_hw *hw, u8 index,
+ u16 *node_handle, u8 *node_part_number);
int
ice_aq_get_netlist_node(struct ice_hw *hw, struct ice_aqc_get_link_topo *cmd,
u8 *node_part_number, u16 *node_handle);
@@ -117,6 +119,7 @@ int
ice_aq_set_phy_cfg(struct ice_hw *hw, struct ice_port_info *pi,
struct ice_aqc_set_phy_cfg_data *cfg, struct ice_sq_cd *cd);
bool ice_fw_supports_link_override(struct ice_hw *hw);
+bool ice_fw_supports_cmpo(struct ice_hw *hw);
int
ice_get_link_default_override(struct ice_link_default_override_tlv *ldo,
struct ice_port_info *pi);
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 3c3616f0f811..8d5ce9c2ca91 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -1604,6 +1604,14 @@ ice_devlink_set_port_split_options(struct ice_pf *pf,
return;
}
+ pf->split_cnt = options[active_idx].pmd;
+
+ /* As FW supports only port split options for whole device,
+ * set port split options only for first PF.
+ */
+ if (pf->hw.pf_id != 0)
+ return;
+
/* find the biggest available port split count */
for (i = 0; i < option_count; i++)
attrs->lanes = max_t(int, attrs->lanes, options[i].pmd);
@@ -1648,11 +1656,7 @@ int ice_devlink_create_pf_port(struct ice_pf *pf)
attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
attrs.phys.port_number = pf->hw.bus.func;
- /* As FW supports only port split options for whole device,
- * set port split options only for first PF.
- */
- if (pf->hw.pf_id == 0)
- ice_devlink_set_port_split_options(pf, &attrs);
+ ice_devlink_set_port_split_options(pf, &attrs);
ice_devlink_set_switch_id(pf, &attrs.switch_id);
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 255a9c8151b4..b38a984b44a2 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -4297,6 +4297,465 @@ ice_get_module_eeprom(struct net_device *netdev,
return 0;
}
+/**
+ * ice_get_min_pwr_allowed - Get min power allowed
+ * @hw: pointer to the hardware structure
+ * @extack: extended ACK from the Netlink message
+ *
+ * Values are constant based on the cage type.
+ * Return value in mW.
+ */
+static int ice_get_min_pwr_allowed(struct ice_hw *hw,
+ struct netlink_ext_ack *extack)
+{
+ u8 node_part_number;
+ u16 node_handle;
+ int err;
+
+ err = ice_get_port_cage_node(hw, 0, &node_handle, &node_part_number);
+ if (err) {
+ NL_SET_ERR_MSG_MOD(extack, "Failed to get cage node handle");
+ return err;
+ }
+
+ switch (node_part_number) {
+ case ICE_AQC_GET_LINK_TOPO_NODE_NR_SFP_PLUS:
+ case ICE_AQC_GET_LINK_TOPO_NODE_NR_SFP28:
+ return 1000;
+ case ICE_AQC_GET_LINK_TOPO_NODE_NR_QSFP_PLUS:
+ case ICE_AQC_GET_LINK_TOPO_NODE_NR_QSFP28:
+ return 1500;
+ default:
+ return -EINVAL;
+ }
+}
+
+/**
+ * ice_pwr_nvm_to_ethtool - Convert NVM values to ethtool
+ * @pwr: power from NVM
+ */
+static u32 ice_pwr_nvm_to_ethtool(u32 pwr)
+{
+ /* ethtool takes power values in mW */
+ pwr *= 1000;
+ /* 0.5 W resolution */
+ pwr /= 2;
+
+ return pwr;
+}
+
+/**
+ * ice_pwr_ethtool_to_nvm - Convert ethtool values to NVM
+ * @pwr: power from ethtool, in mW
+ */
+static u32 ice_pwr_ethtool_to_nvm(u32 pwr)
+{
+ /* 0.5 W resolution */
+ pwr *= 2;
+ /* ethtool takes power values in mW */
+ pwr /= 1000;
+
+ return pwr;
+}
+
+/**
+ * ice_get_num_of_cages - Get number of cages in the board
+ * @hw: pointer to the hardware structure
+ *
+ * We have as many cages as netlist nodes of cage type.
+ */
+static int ice_get_num_of_cages(struct ice_hw *hw)
+{
+ int i, err, cage_count = 0;
+ u8 node_part_number;
+ u16 node_handle;
+
+ for (i = 0; i < ICE_NUM_OF_CAGES; i++) {
+ err = ice_get_port_cage_node(hw, i, &node_handle,
+ &node_part_number);
+ if (!err && node_handle)
+ cage_count++;
+ }
+
+ return cage_count;
+}
+
+/**
+ * ice_get_board_max_pwr - Get max power allowed per board
+ * @hw: pointer to the hardware structure
+ *
+ * Board maximum power is stored in EMP settings NVM module
+ */
+static int ice_get_board_max_pwr(struct ice_hw *hw)
+{
+ u16 board_max_pwr;
+ __le16 data;
+ int err;
+
+ err = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (err)
+ return err;
+
+ err = ice_aq_read_nvm(hw, ICE_AQC_NVM_EMP_SETTINGS_MOD_ID,
+ ICE_AQC_NVM_MAX_PWR_LIMIT_OFFSET,
+ sizeof(data), &data,
+ true, false, NULL);
+ if (err) {
+ ice_release_nvm(hw);
+ return err;
+ }
+
+ ice_release_nvm(hw);
+
+ board_max_pwr = __le16_to_cpu(data);
+ board_max_pwr = FIELD_GET(ICE_AQC_NVM_BOARD_MAX_PWR_MASK,
+ board_max_pwr);
+
+ return ice_pwr_nvm_to_ethtool(board_max_pwr);
+}
+
+/**
+ * ice_get_max_pwr_allowed - Get max power allowed per cage
+ * @hw: pointer to the hardware structure
+ * @min_pwr_allowed: min allowed power
+ * @cage_count: number of cages in the board
+ */
+static int ice_get_max_pwr_allowed(struct ice_hw *hw, u32 min_pwr_allowed,
+ int cage_count)
+{
+ int board_max_pwr;
+
+ board_max_pwr = ice_get_board_max_pwr(hw);
+ if (board_max_pwr < 0)
+ return board_max_pwr;
+
+ return board_max_pwr - (cage_count - 1) * min_pwr_allowed;
+}
+
+/**
+ * ice_get_cage_idx - Get index to the cage
+ * @pf: pointer to the PF structure
+ * @cage_count: number of cages in the board
+ * @extack: extended ACK from the Netlink message
+ *
+ * Get index to the cage and validate if the given PF
+ * is associated with the cage.
+ */
+static int ice_get_cage_idx(struct ice_pf *pf, int cage_count,
+ struct netlink_ext_ack *extack)
+{
+ /* if there is only on cage, PF 0 is responsoble for it */
+ if (cage_count == 1) {
+ if (pf->hw.pf_id == 0)
+ return 0;
+ goto err;
+ } else if (cage_count == 4) {
+ /* if there are 4 cages, than port split is not supported
+ * so each PF is responsoble for its cage
+ */
+ return pf->hw.pf_id;
+ } else if (cage_count == 2) {
+ /* We have 2 cages, PF 0 always takes care of the first one.
+ * If the split_cnt is 2 than PF 1 takes care of the second cage.
+ * If the split_cnt is 4 than PF 2 takes care of the second cage.
+ * If the split_cnt is 8 than PF 4 takes care of the second cage.
+ * So, the formula for the second cage is pf_id * 2 == split_cnt
+ */
+ if (pf->hw.pf_id == 0 || pf->hw.pf_id * 2 == pf->split_cnt)
+ return pf->hw.pf_id;
+ }
+
+err:
+ NL_SET_ERR_MSG_MOD(extack, "Cage maximum power cannot be requested for selected port");
+
+ return -EPERM;
+}
+
+/**
+ * ice_get_dflt_max_pwr - Get dflt max power
+ * @hw: pointer to the hardware structure
+ *
+ * Default max power is stored in EMP settings NVM module
+ */
+static int ice_get_dflt_max_pwr(struct ice_hw *hw)
+{
+ __le16 data;
+ u16 pwr;
+ int ret;
+
+ ret = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (ret)
+ return ret;
+
+ ret = ice_aq_read_nvm(hw, ICE_AQC_NVM_EMP_SETTINGS_MOD_ID,
+ ICE_AQC_NVM_MAX_PWR_LIMIT_OFFSET, sizeof(data),
+ &data, true, false, NULL);
+ if (ret) {
+ ice_release_nvm(hw);
+ return ret;
+ }
+
+ ice_release_nvm(hw);
+
+ pwr = __le16_to_cpu(data);
+ pwr = FIELD_GET(ICE_AQC_NVM_DFLT_MAX_PWR_MASK, pwr);
+
+ return ice_pwr_nvm_to_ethtool(pwr);
+}
+
+/**
+ * ice_get_max_pwr_set - Get currently set max power
+ * @hw: pointer to the hardware structure
+ * @idx: index of the cage
+ *
+ * If cmpo enable bit is set, use the value from
+ * CMPO module otherwise use default value.
+ */
+static int ice_get_max_pwr_set(struct ice_hw *hw, int idx)
+{
+ struct ice_aqc_nvm_cmpo data;
+ int max_pwr_set;
+ u16 temp;
+ int ret;
+
+ ret = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (ret)
+ return ret;
+
+ ret = ice_aq_read_nvm(hw, ICE_AQC_NVM_CMPO_MOD_ID, 0, sizeof(data),
+ &data, true, false, NULL);
+ if (ret) {
+ ice_release_nvm(hw);
+ return ret;
+ }
+
+ ice_release_nvm(hw);
+
+ temp = le16_to_cpu(data.cages_cfg[idx]);
+
+ if (FIELD_GET(ICE_AQC_NVM_CMPO_ENABLE, temp)) {
+ max_pwr_set = FIELD_GET(ICE_AQC_NVM_CMPO_POWER_MASK, temp);
+ return ice_pwr_nvm_to_ethtool(max_pwr_set);
+ } else {
+ return ice_get_dflt_max_pwr(hw);
+ }
+}
+
+/**
+ * ice_get_module_power_cfg - Get device's power setting
+ * @dev: network device
+ * @params: output parameters
+ * @extack: extended ACK from the Netlink message
+ *
+ * We care only about min_pwr_allowed, max_pwr_allowed and max_pwr_set params.
+ */
+static int
+ice_get_module_power_cfg(struct net_device *dev,
+ struct ethtool_module_power_params *params,
+ struct netlink_ext_ack *extack)
+{
+ int min_pwr_allowed, max_pwr_allowed, max_pwr_set, cage_count;
+ struct ice_netdev_priv *np = netdev_priv(dev);
+ struct ice_vsi *vsi = np->vsi;
+ struct ice_pf *pf = vsi->back;
+ struct ice_hw *hw = &pf->hw;
+ int idx;
+
+ if (!ice_fw_supports_cmpo(hw)) {
+ NL_SET_ERR_MSG_MOD(extack, "Cage maximum power request is unsupported by the current firmware");
+ return -EOPNOTSUPP;
+ }
+
+ cage_count = ice_get_num_of_cages(hw);
+
+ idx = ice_get_cage_idx(pf, cage_count, extack);
+ if (idx < 0)
+ return idx;
+
+ min_pwr_allowed = ice_get_min_pwr_allowed(hw, extack);
+ if (min_pwr_allowed < 0) {
+ NL_SET_ERR_MSG_MOD(extack, "Unable to get min power limit");
+ return min_pwr_allowed;
+ }
+
+ max_pwr_allowed = ice_get_max_pwr_allowed(hw, min_pwr_allowed,
+ cage_count);
+ if (max_pwr_allowed < 0) {
+ NL_SET_ERR_MSG_MOD(extack, "Unable to get max power limit");
+ return max_pwr_allowed;
+ }
+
+ max_pwr_set = ice_get_max_pwr_set(hw, idx);
+ if (max_pwr_set < 0) {
+ NL_SET_ERR_MSG_MOD(extack, "Unable to get max power currently set");
+ return max_pwr_set;
+ }
+
+ params->min_pwr_allowed = min_pwr_allowed;
+ params->max_pwr_allowed = max_pwr_allowed;
+ params->max_pwr_set = max_pwr_set;
+
+ return 0;
+}
+
+/**
+ * ice_check_board_pwr_sum - Check if the new sum exceeds the board maximum.
+ * @hw: pointer to the hardware structure
+ * @data: current power config from NVM
+ * @idx: index of the cage we want to update
+ * @power: new power value from ethtool
+ * @cage_count: number of cages in the board
+ * @extack: extended ACK from the Netlink message
+ *
+ * Get number of cages, board maximum and default power value.
+ * Add up all values. If cmpo enable bit is set, use the value from
+ * CMPO module otherwise use default value.
+ */
+static int
+ice_check_board_pwr_sum(struct ice_hw *hw, struct ice_aqc_nvm_cmpo *data,
+ int idx, u32 power, int cage_count,
+ struct netlink_ext_ack *extack)
+{
+ int board_max_pwr, dflt_pwr, max_pwr_set, sum = 0;
+ u16 temp;
+ int i;
+
+ board_max_pwr = ice_get_board_max_pwr(hw);
+ if (board_max_pwr < 0)
+ return board_max_pwr;
+
+ dflt_pwr = ice_get_dflt_max_pwr(hw);
+ if (dflt_pwr < 0)
+ return dflt_pwr;
+
+ for (i = 0; i < cage_count; i++) {
+ temp = le16_to_cpu(data->cages_cfg[i]);
+
+ /* skipping the cage we want to update with the new value, we
+ * want to add the new power, not the value from NVM
+ */
+ if (i == idx)
+ continue;
+
+ if (FIELD_GET(ICE_AQC_NVM_CMPO_ENABLE, temp)) {
+ max_pwr_set = FIELD_GET(ICE_AQC_NVM_CMPO_POWER_MASK,
+ temp);
+ sum += ice_pwr_nvm_to_ethtool(max_pwr_set);
+ } else {
+ sum += dflt_pwr;
+ }
+ }
+
+ sum += power;
+
+ if (sum > board_max_pwr) {
+ NL_SET_ERR_MSG_MOD(extack, "Sum of power values is out of range: overbudgeting board level.");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/**
+ * ice_set_module_power_cfg - Update device's power setting
+ * @dev: network device
+ * @params: new power config
+ * @extack: extended ACK from the Netlink message
+ *
+ * We care only about max_pwr_set and max_pwr_reset params.
+ */
+static int
+ice_set_module_power_cfg(struct net_device *dev,
+ const struct ethtool_module_power_params *params,
+ struct netlink_ext_ack *extack)
+{
+ struct ice_netdev_priv *np = netdev_priv(dev);
+ struct ice_vsi *vsi = np->vsi;
+ struct ice_pf *pf = vsi->back;
+ struct ice_aqc_nvm_cmpo data;
+ struct ice_hw *hw = &pf->hw;
+ int idx, ret, cage_count;
+ u16 power;
+
+ if (params->policy) {
+ NL_SET_ERR_MSG_MOD(extack, "Unsupported power parameter.");
+ return -EOPNOTSUPP;
+ }
+
+ if (!ice_fw_supports_cmpo(hw)) {
+ NL_SET_ERR_MSG_MOD(extack, "Cage maximum power request is unsupported by the current firmware.");
+ return -EOPNOTSUPP;
+ }
+
+ if (params->max_pwr_set % 500) {
+ NL_SET_ERR_MSG_MOD(extack, "Unsupported power resolution, use 500 mW resolution.");
+ return -EOPNOTSUPP;
+ }
+
+ cage_count = ice_get_num_of_cages(hw);
+
+ idx = ice_get_cage_idx(pf, cage_count, extack);
+ if (idx < 0)
+ return idx;
+
+ ret = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (ret)
+ return ret;
+
+ ret = ice_aq_read_nvm(hw, ICE_AQC_NVM_CMPO_MOD_ID, 0, sizeof(data),
+ &data, true, false, NULL);
+ if (ret) {
+ ice_release_nvm(hw);
+ NL_SET_ERR_MSG_MOD(extack, "Failed to read NVM power config.");
+ return ret;
+ }
+
+ ice_release_nvm(hw);
+
+ power = ice_pwr_ethtool_to_nvm(params->max_pwr_set);
+
+ if (power) {
+ ret = ice_check_board_pwr_sum(hw, &data, idx,
+ params->max_pwr_set, cage_count,
+ extack);
+ if (ret)
+ return ret;
+
+ data.cages_cfg[idx] =
+ cpu_to_le16(power & ICE_AQC_NVM_CMPO_POWER_MASK);
+ data.cages_cfg[idx] |= cpu_to_le16(ICE_AQC_NVM_CMPO_ENABLE);
+ } else {
+ data.cages_cfg[idx] &= ~cpu_to_le16(ICE_AQC_NVM_CMPO_ENABLE);
+ }
+
+ ret = ice_acquire_nvm(hw, ICE_RES_WRITE);
+ if (ret)
+ return ret;
+
+ ret = ice_aq_update_nvm(hw, ICE_AQC_NVM_CMPO_MOD_ID, 2,
+ sizeof(data.cages_cfg), data.cages_cfg,
+ false, 0, NULL);
+ if (ret) {
+ ice_release_nvm(hw);
+ NL_SET_ERR_MSG_MOD(extack, "Failed to update NVM power config.");
+ return ret;
+ }
+
+ ret = ice_nvm_write_activate(&pf->hw, ICE_AQC_NVM_ACTIV_REQ_EMPR,
+ NULL);
+ if (ret) {
+ ice_release_nvm(hw);
+ NL_SET_ERR_MSG_MOD(extack, "Failed to save NVM power config.");
+ return ret;
+ }
+
+ ice_release_nvm(hw);
+
+ dev_info(ice_pf_to_dev(pf), "Reboot is required to complete power change.");
+
+ return 0;
+}
+
static const struct ethtool_ops ice_ethtool_ops = {
.cap_rss_ctx_supported = true,
.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
@@ -4344,6 +4803,8 @@ static const struct ethtool_ops ice_ethtool_ops = {
.set_fecparam = ice_set_fecparam,
.get_module_info = ice_get_module_info,
.get_module_eeprom = ice_get_module_eeprom,
+ .get_module_power_cfg = ice_get_module_power_cfg,
+ .set_module_power_cfg = ice_set_module_power_cfg,
};
static const struct ethtool_ops ice_ethtool_safe_mode_ops = {
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index d4e05d2cb30c..f3ef1211acd7 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -18,7 +18,7 @@
*
* Read the NVM using the admin queue commands (0x0701)
*/
-static int
+int
ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset, u16 length,
void *data, bool last_command, bool read_shadow_ram,
struct ice_sq_cd *cd)
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
index 774c2317967d..63cdc6bdac58 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.h
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
@@ -14,6 +14,9 @@ struct ice_orom_civd_info {
int ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access);
void ice_release_nvm(struct ice_hw *hw);
+int ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
+ u16 length, void *data, bool last_command,
+ bool read_shadow_ram, struct ice_sq_cd *cd);
int
ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
bool read_shadow_ram);
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 08ec5efdafe6..91506ddc5419 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -1152,6 +1152,10 @@ struct ice_aq_get_set_rss_lut_params {
#define ICE_FW_API_LINK_OVERRIDE_MIN 5
#define ICE_FW_API_LINK_OVERRIDE_PATCH 2
+#define ICE_FW_CMPO_MAJ 7
+#define ICE_FW_CMPO_MIN 4
+#define ICE_FW_CMPO_PATCH 1
+
#define ICE_SR_WORDS_IN_1KB 512
/* AQ API version for LLDP_FILTER_CONTROL */
--
2.40.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH net-next 0/3] ethtool: Max power support
2024-03-29 9:23 [PATCH net-next 0/3] ethtool: Max power support Wojciech Drewek
` (2 preceding siblings ...)
2024-03-29 9:23 ` [PATCH net-next 3/3] ice: Implement ethtool max power configuration Wojciech Drewek
@ 2024-03-29 22:16 ` Jakub Kicinski
2024-04-02 9:58 ` Wojciech Drewek
2024-03-30 21:57 ` Andrew Lunn
4 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2024-03-29 22:16 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet,
pabeni, idosch, przemyslaw.kitszel, marcin.szycik
On Fri, 29 Mar 2024 10:23:18 +0100 Wojciech Drewek wrote:
> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
> module implementation to support new attributes that will allow user
> to change maximum power. Rename structures and functions to be more
> generic. Introduce an example of the new API in ice driver.
I'm no SFP expert but seems reasonable.
Would be good to insert more references to the SFP / CMIS specs
which describe the standard registers.
Also the series is suffering from lack of docs and spec, please
update both:
Documentation/networking/ethtool-netlink.rst
Documentation/netlink/specs/ethtool.yaml
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH net-next 0/3] ethtool: Max power support
2024-03-29 22:16 ` [PATCH net-next 0/3] ethtool: Max power support Jakub Kicinski
@ 2024-04-02 9:58 ` Wojciech Drewek
0 siblings, 0 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-02 9:58 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet,
pabeni, idosch, przemyslaw.kitszel, marcin.szycik
On 29.03.2024 23:16, Jakub Kicinski wrote:
> On Fri, 29 Mar 2024 10:23:18 +0100 Wojciech Drewek wrote:
>> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
>> module implementation to support new attributes that will allow user
>> to change maximum power. Rename structures and functions to be more
>> generic. Introduce an example of the new API in ice driver.
>
> I'm no SFP expert but seems reasonable.
>
> Would be good to insert more references to the SFP / CMIS specs
> which describe the standard registers.
Sure, I'll search for additional references.
>
> Also the series is suffering from lack of docs and spec, please
> update both:
>
> Documentation/networking/ethtool-netlink.rst
> Documentation/netlink/specs/ethtool.yaml
Sure thing, I'll write the documentation.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 0/3] ethtool: Max power support
2024-03-29 9:23 [PATCH net-next 0/3] ethtool: Max power support Wojciech Drewek
` (3 preceding siblings ...)
2024-03-29 22:16 ` [PATCH net-next 0/3] ethtool: Max power support Jakub Kicinski
@ 2024-03-30 21:57 ` Andrew Lunn
2024-03-30 21:57 ` Andrew Lunn
2024-04-02 11:38 ` [Intel-wired-lan] " Wojciech Drewek
4 siblings, 2 replies; 34+ messages in thread
From: Andrew Lunn @ 2024-03-30 21:57 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet,
kuba, pabeni, idosch, przemyslaw.kitszel, marcin.szycik
On Fri, Mar 29, 2024 at 10:23:18AM +0100, Wojciech Drewek wrote:
> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
> module implementation to support new attributes that will allow user
> to change maximum power. Rename structures and functions to be more
> generic. Introduce an example of the new API in ice driver.
>
> Ethtool examples:
> $ ethtool --show-module enp1s0f0np0
> Module parameters for enp1s0f0np0:
> power-min-allowed: 1000 mW
> power-max-allowed: 3000 mW
> power-max-set: 1500 mW
>
> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
We have had a device tree property for a long time:
maximum-power-milliwatt:
minimum: 1000
default: 1000
description:
Maximum module power consumption Specifies the maximum power consumption
allowable by a module in the slot, in milli-Watts. Presently, modules can
be up to 1W, 1.5W or 2W.
Could you flip the name around to be consistent with DT?
> minimum-power-allowed: 1000 mW
> maximum-power-allowed: 3000 mW
> maximum-power-set: 1500 mW
Also, what does minimum-power-allowed actually tell us? Do you imagine
it will ever be below 1W because of bad board design? Do you have a
bad board design which does not allow 1W?
Also, this is about the board, the SFP cage, not the actual SFP
module? Maybe the word cage needs to be in these names?
Do we want to be able to enumerate what the module itself supports?
If so, we need to include module in the name, to identify the numbers
are about the module, not the cage.
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH net-next 0/3] ethtool: Max power support
2024-03-30 21:57 ` Andrew Lunn
@ 2024-03-30 21:57 ` Andrew Lunn
2024-04-02 11:38 ` [Intel-wired-lan] " Wojciech Drewek
1 sibling, 0 replies; 34+ messages in thread
From: Andrew Lunn @ 2024-03-30 21:57 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, intel-wired-lan, simon.horman, anthony.l.nguyen, edumazet,
kuba, pabeni, idosch, przemyslaw.kitszel, marcin.szycik
On Fri, Mar 29, 2024 at 10:23:18AM +0100, Wojciech Drewek wrote:
> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
> module implementation to support new attributes that will allow user
> to change maximum power. Rename structures and functions to be more
> generic. Introduce an example of the new API in ice driver.
>
> Ethtool examples:
> $ ethtool --show-module enp1s0f0np0
> Module parameters for enp1s0f0np0:
> power-min-allowed: 1000 mW
> power-max-allowed: 3000 mW
> power-max-set: 1500 mW
>
> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
We have had a device tree property for a long time:
maximum-power-milliwatt:
minimum: 1000
default: 1000
description:
Maximum module power consumption Specifies the maximum power consumption
allowable by a module in the slot, in milli-Watts. Presently, modules can
be up to 1W, 1.5W or 2W.
Could you flip the name around to be consistent with DT?
> minimum-power-allowed: 1000 mW
> maximum-power-allowed: 3000 mW
> maximum-power-set: 1500 mW
Also, what does minimum-power-allowed actually tell us? Do you imagine
it will ever be below 1W because of bad board design? Do you have a
bad board design which does not allow 1W?
Also, this is about the board, the SFP cage, not the actual SFP
module? Maybe the word cage needs to be in these names?
Do we want to be able to enumerate what the module itself supports?
If so, we need to include module in the name, to identify the numbers
are about the module, not the cage.
Andrew
X-sender: <netdev+bounces-83534-steffen.klassert=secunet.com@vger.kernel.org>
X-Receiver: <steffen.klassert@secunet.com> ORCPT=rfc822;steffen.klassert@secunet.com NOTIFY=NEVER; X-ExtendedProps=BQAVABYAAgAAAAUAFAARAPDFCS25BAlDktII2g02frgPADUAAABNaWNyb3NvZnQuRXhjaGFuZ2UuVHJhbnNwb3J0LkRpcmVjdG9yeURhdGEuSXNSZXNvdXJjZQIAAAUAagAJAAEAAAAAAAAABQAWAAIAAAUAQwACAAAFAEYABwADAAAABQBHAAIAAAUAEgAPAGIAAAAvbz1zZWN1bmV0L291PUV4Y2hhbmdlIEFkbWluaXN0cmF0aXZlIEdyb3VwIChGWURJQk9IRjIzU1BETFQpL2NuPVJlY2lwaWVudHMvY249U3RlZmZlbiBLbGFzc2VydDY4YwUACwAXAL4AAACheZxkHSGBRqAcAp3ukbifQ049REI2LENOPURhdGFiYXNlcyxDTj1FeGNoYW5nZSBBZG1pbmlzdHJhdGl2ZSBHcm91cCAoRllESUJPSEYyM1NQRExUKSxDTj1BZG1pbmlzdHJhdGl2ZSBHcm91cHMsQ049c2VjdW5ldCxDTj1NaWNyb3NvZnQgRXhjaGFuZ2UsQ049U2VydmljZXMsQ049Q29uZmlndXJhdGlvbixEQz1zZWN1bmV0LERDPWRlBQAOABEABiAS9uuMOkqzwmEZDvWNNQUAHQAPAAwAAABtYngtZXNzZW4tMDIFADwAAgAADwA2AAAATWljcm9zb2Z0LkV4Y2hhbmdlLlRyYW5zcG9ydC5NYWlsUmVjaXBpZW50LkRpc3BsYXlOYW1lDwARAAAAS2xhc3NlcnQsIFN0ZWZmZW4FAAwAAgAABQBsAAIAAAUAWAAXAEoAAADwxQktuQQJQ5LSCNoNNn64Q049S2xhc3NlcnQgU3RlZmZlbixPVT1Vc2VycyxPVT1NaWdyYXRpb24sREM9c2VjdW5ldCxEQz1kZQUAJgACAAEFACIADwAxAAAAQXV0b1Jlc3BvbnNlU3VwcHJlc3M6IDANClRyYW5zbWl0SGlzdG9yeTogRmFsc2UNCg8ALwAAAE1pY3Jvc29mdC5FeGNoYW5nZS5UcmFuc3BvcnQuRXhwYW5zaW9uR3JvdXBUeXBlDwAVAAAATWVtYmVyc0dyb3VwRXhwYW5zaW9uBQAjAAIAAQ==
X-CreatedBy: MSExchange15
X-HeloDomain: a.mx.secunet.com
X-ExtendedProps: BQBjAAoAUaRAQuxQ3AgFAGEACAABAAAABQA3AAIAAA8APAAAAE1pY3Jvc29mdC5FeGNoYW5nZS5UcmFuc3BvcnQuTWFpbFJlY2lwaWVudC5Pcmdhbml6YXRpb25TY29wZREAAAAAAAAAAAAAAAAAAAAAAAUASQACAAEFAGIACgCOAAAAo4oAAAUABAAUIAEAAAAcAAAAc3RlZmZlbi5rbGFzc2VydEBzZWN1bmV0LmNvbQUABgACAAEFACkAAgABDwAJAAAAQ0lBdWRpdGVkAgABBQACAAcAAQAAAAUAAwAHAAAAAAAFAAUAAgABBQBkAA8AAwAAAEh1Yg==
X-Source: SMTP:Default MBX-DRESDEN-01
X-SourceIPAddress: 62.96.220.36
X-EndOfInjectedXHeaders: 11670
Received: from cas-essen-02.secunet.de (10.53.40.202) by
mbx-dresden-01.secunet.de (10.53.40.199) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.2507.37; Sat, 30 Mar 2024 22:57:27 +0100
Received: from a.mx.secunet.com (62.96.220.36) by cas-essen-02.secunet.de
(10.53.40.202) with Microsoft SMTP Server (version=TLS1_2,
cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.37 via Frontend
Transport; Sat, 30 Mar 2024 22:57:27 +0100
Received: from localhost (localhost [127.0.0.1])
by a.mx.secunet.com (Postfix) with ESMTP id 903C720883
for <steffen.klassert@secunet.com>; Sat, 30 Mar 2024 22:57:27 +0100 (CET)
X-Virus-Scanned: by secunet
X-Spam-Flag: NO
X-Spam-Score: -2.751
X-Spam-Level:
X-Spam-Status: No, score=-2.751 tagged_above=-999 required=2.1
tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1,
DKIM_VALID_AU=-0.1, HEADER_FROM_DIFFERENT_DOMAINS=0.249,
MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_NONE=-0.0001,
SPF_HELO_NONE=0.001, SPF_PASS=-0.001]
autolearn=unavailable autolearn_force=no
Authentication-Results: a.mx.secunet.com (amavisd-new);
dkim=pass (1024-bit key) header.d=lunn.ch
Received: from a.mx.secunet.com ([127.0.0.1])
by localhost (a.mx.secunet.com [127.0.0.1]) (amavisd-new, port 10024)
with ESMTP id dzDR-mKVAYro for <steffen.klassert@secunet.com>;
Sat, 30 Mar 2024 22:57:23 +0100 (CET)
Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=147.75.48.161; helo=sy.mirrors.kernel.org; envelope-from=netdev+bounces-83534-steffen.klassert=secunet.com@vger.kernel.org; receiver=steffen.klassert@secunet.com
DKIM-Filter: OpenDKIM Filter v2.11.0 a.mx.secunet.com B65A020820
Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org [147.75.48.161])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by a.mx.secunet.com (Postfix) with ESMTPS id B65A020820
for <steffen.klassert@secunet.com>; Sat, 30 Mar 2024 22:57:22 +0100 (CET)
Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by sy.mirrors.kernel.org (Postfix) with ESMTPS id 263B5B21BA2
for <steffen.klassert@secunet.com>; Sat, 30 Mar 2024 21:57:18 +0000 (UTC)
Received: from localhost.localdomain (localhost.localdomain [127.0.0.1])
by smtp.subspace.kernel.org (Postfix) with ESMTP id C2FD04AEF3;
Sat, 30 Mar 2024 21:57:11 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org;
dkim=pass (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b="PBZbgQcq"
X-Original-To: netdev@vger.kernel.org
Received: from vps0.lunn.ch (vps0.lunn.ch [156.67.10.101])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9788217D2
for <netdev@vger.kernel.org>; Sat, 30 Mar 2024 21:57:08 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=156.67.10.101
ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;
t=1711835831; cv=none; b=o3bmGg6pkxuKNLm23H/EqQl645mc3BI/3oNOnCmXXZeDTufy4S/7XoTQckaCTsQgQPfJ1oCkaO6nIW3MXaFEVFyHktLJfRbH2yk609gThZggBQh+G6ND84LMgOQYRvJsHTSzLdM/7WchqsReFhlSICSnm9tENjPKVSVZFcZUWmI=
ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org;
s=arc-20240116; t=1711835831; c=relaxed/simple;
bh=p8JMrR3aXxDyceVxKGrjVePzewVWjN37i/MfiACxrRY=;
h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:
Content-Type:Content-Disposition:In-Reply-To; b=ccWt3HBLvS2OtGm3Ycn21Leg/8+JZXm2Z9Ar33psizw6Ywv3BgORO6Ckh90tqq6sWTFhv2V/gQpr631zW6reHM8kdXGh044T2C270gIj2TzxdFBYQ+TjGsrSta6WOIIbwiRgt+z9HVlYHX4ep4OLtsd+CphA2ThRAj3Ga1w3Cdc=
ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lunn.ch; spf=pass smtp.mailfrom=lunn.ch; dkim=pass (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b=PBZbgQcq; arc=none smtp.client-ip=156.67.10.101
Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lunn.ch
Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=lunn.ch
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch;
s=20171124; h=In-Reply-To:Content-Disposition:Content-Type:MIME-Version:
References:Message-ID:Subject:Cc:To:From:Date:From:Sender:Reply-To:Subject:
Date:Message-ID:To:Cc:MIME-Version:Content-Type:Content-Transfer-Encoding:
Content-ID:Content-Description:Content-Disposition:In-Reply-To:References;
bh=IZ0VCe3iNf6wEVXOTCw6lmBDLZ7NjriGD/YDnR3wMN4=; b=PBZbgQcqz+Z/9+/z5NNgXvLHao
y+Z95swHz8k9SInML7Cx6typz+jPac5PQ8x2QOVjZiHrhxDhb6tkgljD806DlPmYqcV54edGZmbCl
y8BmaRaIJM2yB2pDVuq/vtwhzgL16ybpZWuM+0dzv0/X9Nml7k+SAMK9FfVrBl/aRKLQ=;
Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2)
(envelope-from <andrew@lunn.ch>)
id 1rqghD-00BkPE-0K; Sat, 30 Mar 2024 22:57:03 +0100
Date: Sat, 30 Mar 2024 22:57:02 +0100
From: Andrew Lunn <andrew@lunn.ch>
To: Wojciech Drewek <wojciech.drewek@intel.com>
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
simon.horman@corigine.com, anthony.l.nguyen@intel.com,
edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
idosch@nvidia.com, przemyslaw.kitszel@intel.com,
marcin.szycik@linux.intel.com
Subject: Re: [PATCH net-next 0/3] ethtool: Max power support
Message-ID: <38d874e3-f25b-4af2-8c1c-946ab74c1925@lunn.ch>
References: <20240329092321.16843-1-wojciech.drewek@intel.com>
Precedence: bulk
X-Mailing-List: netdev@vger.kernel.org
List-Id: <netdev.vger.kernel.org>
List-Subscribe: <mailto:netdev+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:netdev+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240329092321.16843-1-wojciech.drewek@intel.com>
Return-Path: netdev+bounces-83534-steffen.klassert=secunet.com@vger.kernel.org
X-MS-Exchange-Organization-OriginalArrivalTime: 30 Mar 2024 21:57:27.6324
(UTC)
X-MS-Exchange-Organization-Network-Message-Id: ba3aa9db-22e3-4c07-4c0e-08dc510463d4
X-MS-Exchange-Organization-OriginalClientIPAddress: 62.96.220.36
X-MS-Exchange-Organization-OriginalServerIPAddress: 10.53.40.202
X-MS-Exchange-Organization-Cross-Premises-Headers-Processed: cas-essen-02.secunet.de
X-MS-Exchange-Organization-OrderedPrecisionLatencyInProgress: LSRV=mbx-dresden-01.secunet.de:TOTAL-HUB=0.389|SMR=0.342(SMRDE=0.034|SMRC=0.307(SMRCL=0.102|X-SMRCR=0.307))|CAT=0.046(CATOS=0.011
(CATSM=0.011(CATSM-Malware
Agent=0.011))|CATRESL=0.020(CATRESLP2R=0.017)|CATORES=0.013
(CATRS=0.013(CATRS-Index Routing Agent=0.012)));2024-03-30T21:57:28.034Z
X-MS-Exchange-Forest-ArrivalHubServer: mbx-dresden-01.secunet.de
X-MS-Exchange-Organization-AuthSource: cas-essen-02.secunet.de
X-MS-Exchange-Organization-AuthAs: Anonymous
X-MS-Exchange-Organization-FromEntityHeader: Internet
X-MS-Exchange-Organization-OriginalSize: 8117
X-MS-Exchange-Organization-HygienePolicy: Standard
X-MS-Exchange-Organization-MessageLatency: SRV=cas-essen-02.secunet.de:TOTAL-FE=0.012|SMR=0.011(SMRPI=0.008(SMRPI-FrontendProxyAgent=0.008))
X-MS-Exchange-Organization-AVStamp-Enterprise: 1.0
X-MS-Exchange-Organization-Recipient-Limit-Verified: True
X-MS-Exchange-Organization-TotalRecipientCount: 1
X-MS-Exchange-Organization-Rules-Execution-History: 0b0cf904-14ac-4724-8bdf-482ee6223cf2%%%fd34672d-751c-45ae-a963-ed177fcabe23%%%d8080257-b0c3-47b4-b0db-23bc0c8ddb3c%%%95e591a2-5d7d-4afa-b1d0-7573d6c0a5d9%%%f7d0f6bc-4dcc-4876-8c5d-b3d6ddbb3d55%%%16355082-c50b-4214-9c7d-d39575f9f79b
X-MS-Exchange-Forest-RulesExecuted: mbx-dresden-01
X-MS-Exchange-Organization-RulesExecuted: mbx-dresden-01
X-MS-Exchange-Forest-IndexAgent-0: AQ0CZW4AAa0EAAAPAAADH4sIAAAAAAAEAHVUTW8bNxDlWtqVtIrygb
bodQ69VRIkJwFaH2IYTQLkYNRoCuhQ9EBpKYvpLilwuZb10/rv+obU
ynLqAMSCO3zz8WYe+e+z3w19dHpM19LR+a9jOp+dvyHpaT67OH99Mf
/l6pp+ns1nszEt7JeVVqsNvXdqp/6hnbNeXYzyd/TZVoqU3yhnlKfK
Fk2pampqRcaa2ktTSFfQ1u6Uo1LdqbKmv+Z/T+nDvVemYE9vbcmRoi
/paluqShkvvbaGvKW62W6t82TUDtV5p5eNRw6/Qak7XZYky9LuOKfj
OPBYbaS5VVTJe101Vcw+pT+UkSi29q5Z+cYhBKqjdWNWnKlmxyWcrF
Mc5lYZ5fRqSp+MdyhtpQAndS+5PrJr5FehpKubT6QNaQAKp++QCe4c
4UPk1vrUoV8/tZRpMqk3djc50FZmO69n65nZzhh2Ha1b6VCyV66mtX
WnoBAsEJtU2kxCC1RxgdnNZlQtTk7l/cPp66dOa+Xh97Y9+V+Vyj9R
5GN/eoPAo3yULxRt5B1/CpJUqDtui3cKVJzdKuf3gYik0ppb8rpiFY
1yamc1aSmVpd5h2Bd8hlNt+DTSi6ZCrWVT+semeuX0lqd58CNoO2rg
wCAKcYVxN1UA0uetWum1DoL6SjKnuDZeaKVcItRyDxqtaE3wrkvrx/
wT6p8sQKCe0g2kBj2X+/HxfqzkMSAk12xZe/PFmObTtwtCf84XU27L
b7YpC9rbhtal3kbFsYSlsw2kGwXLReoat4lvg8cV/fOSfd+1TTu09C
mFPG76Uyp5jHislFF+VdZ2TDu+iIUFrSczksRtw35PXuGyNvUlvbeB
lK7krTa4bfpwkfE+OKa0VHyh5wtsVpLfEly3JRS1tPyYYMz61hyjBL
3JUf41AHVpvFihMGP94ZWYLy4fKvcbXROWXNrGh/aGAOOw/fzxBnO6
VePgzZZIhA9GeZzkJQtsj4r5eGeRmz3wLKiifU+iNOo4uTokR+E7wC
UmFjFBT9gq01TKSa9iT4MgDwLztSrX7VPIYT7hj5sfs7G3NquyKdRX
muS043BcQCJ6vY/WplriVRnl0qkT+tH1gTGzmcYLSnRlCrz+/CfEme
h0RNpJxDMs0T0T3a5Iz5JOPxF9gdXHPhUiFb2+GGSil4ocAOx7ot8R
3aF4loosSwAXiegC0BNDfJ+LF4gcMbkYwgULyC4Cik6A9RETG5wCMx
A57FgA9MQg/vYCGKdxxX0mnsPeF6NEnPVQTCKGIkWcoXhxjDYUL9NE
vBIZ9r1k0BWCSYnsW8ZRMHaStDUOYnC0JQ+kmOYDrBtg/VBtCgvAwM
S2xN/Ad9hLxI9MMxuIl23G/LgZiu9BJLQ0Q3xQ5sICfii+ix2IkSPx
rsiwSZlvhtYNEJ/bO4x945V0wxTOWgtHDo5p/MZQWHEioIkKfwgZTx
vyLeOrYDzpEqaZxck+uPBoBuH0MC9MMzYqGKGl7HSyLJ52D0DyYOwF
taTx94zbmx0jBGMvqqIbhxLG1GGOgyPrKLljf1Lu21Fm6elmEFQdg2
Qs9djV7hEQJ3U6iDjrXvjGjFlQewCMsv8AWnKrhZUJAAABDs4BUmV0
cmlldmVyT3BlcmF0b3IsMTAsMDtSZXRyaWV2ZXJPcGVyYXRvciwxMS
wwO1Bvc3REb2NQYXJzZXJPcGVyYXRvciwxMCwwO1Bvc3REb2NQYXJz
ZXJPcGVyYXRvciwxMSwwO1Bvc3RXb3JkQnJlYWtlckRpYWdub3N0aW
NPcGVyYXRvciwxMCwwO1Bvc3RXb3JkQnJlYWtlckRpYWdub3N0aWNP
cGVyYXRvciwxMSwwO1RyYW5zcG9ydFdyaXRlclByb2R1Y2VyLDIwLD c=
X-MS-Exchange-Forest-IndexAgent: 1 1418
X-MS-Exchange-Forest-EmailMessageHash: ECED4171
X-MS-Exchange-Forest-Language: en
X-MS-Exchange-Organization-Processed-By-Journaling: Journal Agent
On Fri, Mar 29, 2024 at 10:23:18AM +0100, Wojciech Drewek wrote:
> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
> module implementation to support new attributes that will allow user
> to change maximum power. Rename structures and functions to be more
> generic. Introduce an example of the new API in ice driver.
>
> Ethtool examples:
> $ ethtool --show-module enp1s0f0np0
> Module parameters for enp1s0f0np0:
> power-min-allowed: 1000 mW
> power-max-allowed: 3000 mW
> power-max-set: 1500 mW
>
> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
We have had a device tree property for a long time:
maximum-power-milliwatt:
minimum: 1000
default: 1000
description:
Maximum module power consumption Specifies the maximum power consumption
allowable by a module in the slot, in milli-Watts. Presently, modules can
be up to 1W, 1.5W or 2W.
Could you flip the name around to be consistent with DT?
> minimum-power-allowed: 1000 mW
> maximum-power-allowed: 3000 mW
> maximum-power-set: 1500 mW
Also, what does minimum-power-allowed actually tell us? Do you imagine
it will ever be below 1W because of bad board design? Do you have a
bad board design which does not allow 1W?
Also, this is about the board, the SFP cage, not the actual SFP
module? Maybe the word cage needs to be in these names?
Do we want to be able to enumerate what the module itself supports?
If so, we need to include module in the name, to identify the numbers
are about the module, not the cage.
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-03-30 21:57 ` Andrew Lunn
2024-03-30 21:57 ` Andrew Lunn
@ 2024-04-02 11:38 ` Wojciech Drewek
2024-04-02 14:25 ` Jakub Kicinski
2024-04-02 14:46 ` Andrew Lunn
1 sibling, 2 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-02 11:38 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel
On 30.03.2024 22:57, Andrew Lunn wrote:
> On Fri, Mar 29, 2024 at 10:23:18AM +0100, Wojciech Drewek wrote:
>> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
>> module implementation to support new attributes that will allow user
>> to change maximum power. Rename structures and functions to be more
>> generic. Introduce an example of the new API in ice driver.
>>
>> Ethtool examples:
>> $ ethtool --show-module enp1s0f0np0
>> Module parameters for enp1s0f0np0:
>> power-min-allowed: 1000 mW
>> power-max-allowed: 3000 mW
>> power-max-set: 1500 mW
>>
>> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
>
> We have had a device tree property for a long time:
>
> maximum-power-milliwatt:
> minimum: 1000
> default: 1000
> description:
> Maximum module power consumption Specifies the maximum power consumption
> allowable by a module in the slot, in milli-Watts. Presently, modules can
> be up to 1W, 1.5W or 2W.
>
> Could you flip the name around to be consistent with DT?
Yea, I'm open to any name suggestion although I don't like the unit in the parameter name :)
>
>> minimum-power-allowed: 1000 mW
>> maximum-power-allowed: 3000 mW
>> maximum-power-set: 1500 mW
>
> Also, what does minimum-power-allowed actually tell us? Do you imagine
> it will ever be below 1W because of bad board design? Do you have a
> bad board design which does not allow 1W?
Yes. in case of QSFP we don't support 1W, 1.5W is the minimum.
This parameter tells the user what is the lowest limit he can set.
>
> Also, this is about the board, the SFP cage, not the actual SFP
> module? Maybe the word cage needs to be in these names?
It's about cage. Thanks for bringing it to my attention because now I
see it might be misleading. I'm extending {set|show}-module command
but the changes are about max power in the cage. With that in mind
I agree that adding 'cage' to the names makes sense.
>
> Do we want to be able to enumerate what the module itself supports?
> If so, we need to include module in the name, to identify the numbers
> are about the module, not the cage.
I hope that my previous paragraph answers this as well.
>
> Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-02 11:38 ` [Intel-wired-lan] " Wojciech Drewek
@ 2024-04-02 14:25 ` Jakub Kicinski
2024-04-02 14:53 ` Andrew Lunn
2024-04-02 14:46 ` Andrew Lunn
1 sibling, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2024-04-02 14:25 UTC (permalink / raw)
To: Wojciech Drewek
Cc: Andrew Lunn, netdev, idosch, edumazet, marcin.szycik,
anthony.l.nguyen, intel-wired-lan, pabeni, przemyslaw.kitszel
On Tue, 2 Apr 2024 13:38:59 +0200 Wojciech Drewek wrote:
> > Also, this is about the board, the SFP cage, not the actual SFP
> > module? Maybe the word cage needs to be in these names?
>
> It's about cage. Thanks for bringing it to my attention because now I
> see it might be misleading. I'm extending {set|show}-module command
> but the changes are about max power in the cage. With that in mind
> I agree that adding 'cage' to the names makes sense.
Noob question, what happens if you plug a module with higher power
needs into the cage?
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-02 14:25 ` Jakub Kicinski
@ 2024-04-02 14:53 ` Andrew Lunn
0 siblings, 0 replies; 34+ messages in thread
From: Andrew Lunn @ 2024-04-02 14:53 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Wojciech Drewek, netdev, idosch, edumazet, marcin.szycik,
anthony.l.nguyen, intel-wired-lan, pabeni, przemyslaw.kitszel
On Tue, Apr 02, 2024 at 07:25:47AM -0700, Jakub Kicinski wrote:
> On Tue, 2 Apr 2024 13:38:59 +0200 Wojciech Drewek wrote:
> > > Also, this is about the board, the SFP cage, not the actual SFP
> > > module? Maybe the word cage needs to be in these names?
> >
> > It's about cage. Thanks for bringing it to my attention because now I
> > see it might be misleading. I'm extending {set|show}-module command
> > but the changes are about max power in the cage. With that in mind
> > I agree that adding 'cage' to the names makes sense.
>
> Noob question, what happens if you plug a module with higher power
> needs into the cage?
https://www.optcore.net/wp-content/uploads/2017/04/QSFP-MSA.pdf
Section 3.2:
It is recommended that the host, through the management interface,
identify the power consumption class of the module before allowing the
module to go into high power mode.
So it should start in lower power mode. Table 7 suggests the module
can assume 1.5W, since that is the lowest power level.
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-02 11:38 ` [Intel-wired-lan] " Wojciech Drewek
2024-04-02 14:25 ` Jakub Kicinski
@ 2024-04-02 14:46 ` Andrew Lunn
2024-04-02 14:57 ` Jakub Kicinski
2024-04-03 13:18 ` Wojciech Drewek
1 sibling, 2 replies; 34+ messages in thread
From: Andrew Lunn @ 2024-04-02 14:46 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel
On Tue, Apr 02, 2024 at 01:38:59PM +0200, Wojciech Drewek wrote:
>
>
> On 30.03.2024 22:57, Andrew Lunn wrote:
> > On Fri, Mar 29, 2024 at 10:23:18AM +0100, Wojciech Drewek wrote:
> >> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
> >> module implementation to support new attributes that will allow user
> >> to change maximum power. Rename structures and functions to be more
> >> generic. Introduce an example of the new API in ice driver.
> >>
> >> Ethtool examples:
> >> $ ethtool --show-module enp1s0f0np0
> >> Module parameters for enp1s0f0np0:
> >> power-min-allowed: 1000 mW
> >> power-max-allowed: 3000 mW
> >> power-max-set: 1500 mW
> >>
> >> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
> >
> > We have had a device tree property for a long time:
> >
> > maximum-power-milliwatt:
> > minimum: 1000
> > default: 1000
> > description:
> > Maximum module power consumption Specifies the maximum power consumption
> > allowable by a module in the slot, in milli-Watts. Presently, modules can
> > be up to 1W, 1.5W or 2W.
> >
> > Could you flip the name around to be consistent with DT?
>
> Yea, I'm open to any name suggestion although I don't like the unit in the parameter name :)
That is a DT thing. Helps make the units of an ABI obvious. However,
milliwatts is pretty standard with the kernel of user APIs, e.g. all
hwmon calls use milliwatts.
> >> minimum-power-allowed: 1000 mW
> >> maximum-power-allowed: 3000 mW
> >> maximum-power-set: 1500 mW
> >
> > Also, what does minimum-power-allowed actually tell us? Do you imagine
> > it will ever be below 1W because of bad board design? Do you have a
> > bad board design which does not allow 1W?
>
> Yes. in case of QSFP we don't support 1W, 1.5W is the minimum.
So if i plug in a 1W QSFP device, it will let the magic smoke out
because it is force fed 1.5W?
Looking at
https://www.optcore.net/wp-content/uploads/2017/04/QSFP-MSA.pdf table
7 it indicates different power budget classifications. Power level 1
is a Maximum power of 1.5W. So does your parameter represent this? It
is the minimum maximum power? And your other parameter is the maximum
maximum power?
I agree with Jakub here, there needs to be documentation added
explaining in detail what these parameters mean, and ideally,
references to the specification.
Does
$ ethtool --set-module enp1s0f0np0 power-max-set 4000
actually talk to the SFP module and tell it the maximum power it can
consume. So in this case, it is not the cage, but the module?
Or is it talking to some entity which is managing the overall power
consumption of a number of cages, and asking it to allocate a maximum
of 4W to this cage. It might return an error message saying there is
no power budget left?
Or is it doing both?
Sorry to be picky, but at some point, somebody is going to want to
implement this in the Linux SFP driver, and we want a consistent
implementation cross different implementations.
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-02 14:46 ` Andrew Lunn
@ 2024-04-02 14:57 ` Jakub Kicinski
2024-04-03 13:18 ` Wojciech Drewek
1 sibling, 0 replies; 34+ messages in thread
From: Jakub Kicinski @ 2024-04-02 14:57 UTC (permalink / raw)
To: Wojciech Drewek
Cc: Andrew Lunn, netdev, idosch, edumazet, marcin.szycik,
anthony.l.nguyen, intel-wired-lan, pabeni, przemyslaw.kitszel
On Tue, 2 Apr 2024 16:46:54 +0200 Andrew Lunn wrote:
> Looking at
> https://www.optcore.net/wp-content/uploads/2017/04/QSFP-MSA.pdf table
> 7 it indicates different power budget classifications. Power level 1
> is a Maximum power of 1.5W. So does your parameter represent this? It
> is the minimum maximum power? And your other parameter is the maximum
> maximum power?
>
> I agree with Jakub here, there needs to be documentation added
> explaining in detail what these parameters mean, and ideally,
> references to the specification.
>
> Does
>
> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
>
> actually talk to the SFP module and tell it the maximum power it can
> consume. So in this case, it is not the cage, but the module?
>
> Or is it talking to some entity which is managing the overall power
> consumption of a number of cages, and asking it to allocate a maximum
> of 4W to this cage. It might return an error message saying there is
> no power budget left?
>
> Or is it doing both?
>
> Sorry to be picky, but at some point, somebody is going to want to
> implement this in the Linux SFP driver, and we want a consistent
> implementation cross different implementations.
Or "guessing how things work" another way of putting this would be -
please go investigate what exactly the FW will do with these values.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-02 14:46 ` Andrew Lunn
2024-04-02 14:57 ` Jakub Kicinski
@ 2024-04-03 13:18 ` Wojciech Drewek
2024-04-03 13:40 ` Andrew Lunn
2024-04-03 13:49 ` Andrew Lunn
1 sibling, 2 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-03 13:18 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel
On 02.04.2024 16:46, Andrew Lunn wrote:
> On Tue, Apr 02, 2024 at 01:38:59PM +0200, Wojciech Drewek wrote:
>>
>>
>> On 30.03.2024 22:57, Andrew Lunn wrote:
>>> On Fri, Mar 29, 2024 at 10:23:18AM +0100, Wojciech Drewek wrote:
>>>> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
>>>> module implementation to support new attributes that will allow user
>>>> to change maximum power. Rename structures and functions to be more
>>>> generic. Introduce an example of the new API in ice driver.
>>>>
>>>> Ethtool examples:
>>>> $ ethtool --show-module enp1s0f0np0
>>>> Module parameters for enp1s0f0np0:
>>>> power-min-allowed: 1000 mW
>>>> power-max-allowed: 3000 mW
>>>> power-max-set: 1500 mW
>>>>
>>>> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
>>>
>>> We have had a device tree property for a long time:
>>>
>>> maximum-power-milliwatt:
>>> minimum: 1000
>>> default: 1000
>>> description:
>>> Maximum module power consumption Specifies the maximum power consumption
>>> allowable by a module in the slot, in milli-Watts. Presently, modules can
>>> be up to 1W, 1.5W or 2W.
>>>
>>> Could you flip the name around to be consistent with DT?
>>
>> Yea, I'm open to any name suggestion although I don't like the unit in the parameter name :)
>
> That is a DT thing. Helps make the units of an ABI obvious. However,
> milliwatts is pretty standard with the kernel of user APIs, e.g. all
> hwmon calls use milliwatts.
>
>>>> minimum-power-allowed: 1000 mW
>>>> maximum-power-allowed: 3000 mW
>>>> maximum-power-set: 1500 mW
>>>
>>> Also, what does minimum-power-allowed actually tell us? Do you imagine
>>> it will ever be below 1W because of bad board design? Do you have a
>>> bad board design which does not allow 1W?
>>
>> Yes. in case of QSFP we don't support 1W, 1.5W is the minimum.
>
> So if i plug in a 1W QSFP device, it will let the magic smoke out
> because it is force fed 1.5W?
>
> Looking at
> https://www.optcore.net/wp-content/uploads/2017/04/QSFP-MSA.pdf table
> 7 it indicates different power budget classifications. Power level 1
> is a Maximum power of 1.5W. So does your parameter represent this? It
> is the minimum maximum power? And your other parameter is the maximum
> maximum power?
Exactly as you described, minimum-power-allowed is in fact minimum value
which maximum-power-set can be set to (so minimum maximum). the other
parameter is maximim maximum.
>
> I agree with Jakub here, there needs to be documentation added
> explaining in detail what these parameters mean, and ideally,
> references to the specification.
I completely agree, I'll include documentation in the next version.
I see now that those parameters might look confusing, minimum-power-allowed
is not true minimum in fact. We can try to came up with better names
but it might get silly (minimum-maximum-power) XD.
>
> Does
>
> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
>
> actually talk to the SFP module and tell it the maximum power it can
> consume. So in this case, it is not the cage, but the module?
It does not work that way in ice example.
>
> Or is it talking to some entity which is managing the overall power
> consumption of a number of cages, and asking it to allocate a maximum
> of 4W to this cage. It might return an error message saying there is
> no power budget left?
That's right, we talk to firmware to set those restrictions.
In the ice implementation, the driver is responsible for checking if the
overall board budget is not exceeded.
>
> Or is it doing both?
>
> Sorry to be picky, but at some point, somebody is going to want to
> implement this in the Linux SFP driver, and we want a consistent
> implementation cross different implementations.
No problem, I see that your points are totally valid.
>
> Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-03 13:18 ` Wojciech Drewek
@ 2024-04-03 13:40 ` Andrew Lunn
2024-04-04 12:21 ` Wojciech Drewek
2024-04-03 13:49 ` Andrew Lunn
1 sibling, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2024-04-03 13:40 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel
On Wed, Apr 03, 2024 at 03:18:44PM +0200, Wojciech Drewek wrote:
>
>
> On 02.04.2024 16:46, Andrew Lunn wrote:
> > On Tue, Apr 02, 2024 at 01:38:59PM +0200, Wojciech Drewek wrote:
> >>
> >>
> >> On 30.03.2024 22:57, Andrew Lunn wrote:
> >>> On Fri, Mar 29, 2024 at 10:23:18AM +0100, Wojciech Drewek wrote:
> >>>> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
> >>>> module implementation to support new attributes that will allow user
> >>>> to change maximum power. Rename structures and functions to be more
> >>>> generic. Introduce an example of the new API in ice driver.
> >>>>
> >>>> Ethtool examples:
> >>>> $ ethtool --show-module enp1s0f0np0
> >>>> Module parameters for enp1s0f0np0:
> >>>> power-min-allowed: 1000 mW
> >>>> power-max-allowed: 3000 mW
> >>>> power-max-set: 1500 mW
> >>>>
> >>>> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
> >>>
> >>> We have had a device tree property for a long time:
> >>>
> >>> maximum-power-milliwatt:
> >>> minimum: 1000
> >>> default: 1000
> >>> description:
> >>> Maximum module power consumption Specifies the maximum power consumption
> >>> allowable by a module in the slot, in milli-Watts. Presently, modules can
> >>> be up to 1W, 1.5W or 2W.
> >>>
> >>> Could you flip the name around to be consistent with DT?
> >>
> >> Yea, I'm open to any name suggestion although I don't like the unit in the parameter name :)
> >
> > That is a DT thing. Helps make the units of an ABI obvious. However,
> > milliwatts is pretty standard with the kernel of user APIs, e.g. all
> > hwmon calls use milliwatts.
> >
> >>>> minimum-power-allowed: 1000 mW
> >>>> maximum-power-allowed: 3000 mW
> >>>> maximum-power-set: 1500 mW
> >>>
> >>> Also, what does minimum-power-allowed actually tell us? Do you imagine
> >>> it will ever be below 1W because of bad board design? Do you have a
> >>> bad board design which does not allow 1W?
> >>
> >> Yes. in case of QSFP we don't support 1W, 1.5W is the minimum.
> >
> > So if i plug in a 1W QSFP device, it will let the magic smoke out
> > because it is force fed 1.5W?
> >
> > Looking at
> > https://www.optcore.net/wp-content/uploads/2017/04/QSFP-MSA.pdf table
> > 7 it indicates different power budget classifications. Power level 1
> > is a Maximum power of 1.5W. So does your parameter represent this? It
> > is the minimum maximum power? And your other parameter is the maximum
> > maximum power?
>
> Exactly as you described, minimum-power-allowed is in fact minimum value
> which maximum-power-set can be set to (so minimum maximum). the other
> parameter is maximim maximum.
Table 7 in that document is titled "Power Budget Classification". So
how about
minimum-power-class-allowed: 1000 mW
maximum-power-class-allowed: 3000 mW
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-03 13:40 ` Andrew Lunn
@ 2024-04-04 12:21 ` Wojciech Drewek
0 siblings, 0 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-04 12:21 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel
On 03.04.2024 15:40, Andrew Lunn wrote:
> On Wed, Apr 03, 2024 at 03:18:44PM +0200, Wojciech Drewek wrote:
>>
>>
>> On 02.04.2024 16:46, Andrew Lunn wrote:
>>> On Tue, Apr 02, 2024 at 01:38:59PM +0200, Wojciech Drewek wrote:
>>>>
>>>>
>>>> On 30.03.2024 22:57, Andrew Lunn wrote:
>>>>> On Fri, Mar 29, 2024 at 10:23:18AM +0100, Wojciech Drewek wrote:
>>>>>> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
>>>>>> module implementation to support new attributes that will allow user
>>>>>> to change maximum power. Rename structures and functions to be more
>>>>>> generic. Introduce an example of the new API in ice driver.
>>>>>>
>>>>>> Ethtool examples:
>>>>>> $ ethtool --show-module enp1s0f0np0
>>>>>> Module parameters for enp1s0f0np0:
>>>>>> power-min-allowed: 1000 mW
>>>>>> power-max-allowed: 3000 mW
>>>>>> power-max-set: 1500 mW
>>>>>>
>>>>>> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
>>>>>
>>>>> We have had a device tree property for a long time:
>>>>>
>>>>> maximum-power-milliwatt:
>>>>> minimum: 1000
>>>>> default: 1000
>>>>> description:
>>>>> Maximum module power consumption Specifies the maximum power consumption
>>>>> allowable by a module in the slot, in milli-Watts. Presently, modules can
>>>>> be up to 1W, 1.5W or 2W.
>>>>>
>>>>> Could you flip the name around to be consistent with DT?
>>>>
>>>> Yea, I'm open to any name suggestion although I don't like the unit in the parameter name :)
>>>
>>> That is a DT thing. Helps make the units of an ABI obvious. However,
>>> milliwatts is pretty standard with the kernel of user APIs, e.g. all
>>> hwmon calls use milliwatts.
>>>
>>>>>> minimum-power-allowed: 1000 mW
>>>>>> maximum-power-allowed: 3000 mW
>>>>>> maximum-power-set: 1500 mW
>>>>>
>>>>> Also, what does minimum-power-allowed actually tell us? Do you imagine
>>>>> it will ever be below 1W because of bad board design? Do you have a
>>>>> bad board design which does not allow 1W?
>>>>
>>>> Yes. in case of QSFP we don't support 1W, 1.5W is the minimum.
>>>
>>> So if i plug in a 1W QSFP device, it will let the magic smoke out
>>> because it is force fed 1.5W?
>>>
>>> Looking at
>>> https://www.optcore.net/wp-content/uploads/2017/04/QSFP-MSA.pdf table
>>> 7 it indicates different power budget classifications. Power level 1
>>> is a Maximum power of 1.5W. So does your parameter represent this? It
>>> is the minimum maximum power? And your other parameter is the maximum
>>> maximum power?
>>
>> Exactly as you described, minimum-power-allowed is in fact minimum value
>> which maximum-power-set can be set to (so minimum maximum). the other
>> parameter is maximim maximum.
>
> Table 7 in that document is titled "Power Budget Classification". So
> how about
>
> minimum-power-class-allowed: 1000 mW
> maximum-power-class-allowed: 3000 mW
Sounds good
>
> Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-03 13:18 ` Wojciech Drewek
2024-04-03 13:40 ` Andrew Lunn
@ 2024-04-03 13:49 ` Andrew Lunn
2024-04-04 12:45 ` Wojciech Drewek
1 sibling, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2024-04-03 13:49 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel
> > $ ethtool --set-module enp1s0f0np0 power-max-set 4000
> >
> > actually talk to the SFP module and tell it the maximum power it can
> > consume. So in this case, it is not the cage, but the module?
>
> It does not work that way in ice example.
> >
> > Or is it talking to some entity which is managing the overall power
> > consumption of a number of cages, and asking it to allocate a maximum
> > of 4W to this cage. It might return an error message saying there is
> > no power budget left?
>
> That's right, we talk to firmware to set those restrictions.
> In the ice implementation, the driver is responsible for checking if the
> overall board budget is not exceeded.
So i can get the board to agree that the cage can supply 3W to the
module, but how do i then tell the module this?
I would also suggest you don't focus too much on ICE. I find it better
to think about an abstract system. A board with a power supply to a
number of SFP cages, and some cages have modules in them. What does
the kAPI look like, the use cases for this abstract system.
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-03 13:49 ` Andrew Lunn
@ 2024-04-04 12:45 ` Wojciech Drewek
2024-04-04 13:53 ` Andrew Lunn
0 siblings, 1 reply; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-04 12:45 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel
On 03.04.2024 15:49, Andrew Lunn wrote:
>>> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
>>>
>>> actually talk to the SFP module and tell it the maximum power it can
>>> consume. So in this case, it is not the cage, but the module?
>>
>> It does not work that way in ice example.
>>>
>>> Or is it talking to some entity which is managing the overall power
>>> consumption of a number of cages, and asking it to allocate a maximum
>>> of 4W to this cage. It might return an error message saying there is
>>> no power budget left?
>>
>> That's right, we talk to firmware to set those restrictions.
>> In the ice implementation, the driver is responsible for checking if the
>> overall board budget is not exceeded.
>
> So i can get the board to agree that the cage can supply 3W to the
> module, but how do i then tell the module this?
I'd assume it is not possible, if the module consumes more power
than maximum than the link will not come up and error will be printed.
>
> I would also suggest you don't focus too much on ICE. I find it better
> to think about an abstract system. A board with a power supply to a
> number of SFP cages, and some cages have modules in them. What does
> the kAPI look like, the use cases for this abstract system.
My design for this API is to have an option to get and set maximum
power that the module in the cage can consume. It's not about modifying
module's power consumption, it's about setting restrictions for it.
The use case is to let the user change maximum power in the given cage
(so he can plug in the module with higher power consumption). Before that
he will lower maximum power in different cage. Thanks to that the overall
budget for the board won't be exceeded. Does it make sense for the abstract
system you described?
>
> Andrew
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-04 12:45 ` Wojciech Drewek
@ 2024-04-04 13:53 ` Andrew Lunn
2024-04-09 12:20 ` Wojciech Drewek
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2024-04-04 13:53 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel
On Thu, Apr 04, 2024 at 02:45:43PM +0200, Wojciech Drewek wrote:
>
>
> On 03.04.2024 15:49, Andrew Lunn wrote:
> >>> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
> >>>
> >>> actually talk to the SFP module and tell it the maximum power it can
> >>> consume. So in this case, it is not the cage, but the module?
> >>
> >> It does not work that way in ice example.
> >>>
> >>> Or is it talking to some entity which is managing the overall power
> >>> consumption of a number of cages, and asking it to allocate a maximum
> >>> of 4W to this cage. It might return an error message saying there is
> >>> no power budget left?
> >>
> >> That's right, we talk to firmware to set those restrictions.
> >> In the ice implementation, the driver is responsible for checking if the
> >> overall board budget is not exceeded.
> >
> > So i can get the board to agree that the cage can supply 3W to the
> > module, but how do i then tell the module this?
>
> I'd assume it is not possible, if the module consumes more power
> than maximum than the link will not come up and error will be printed.
Take a look at the Linux SFP driver. In sfp_probe() is reads the DT
property maximum-power-milliwatt:
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/sfp.c#L3030
When the module is inserted and probed, the modules power capabilities
are read from the EEPROM:
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/sfp.c#L2320
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/sfp.c#L1929
The code looks to see what conformance level the module has, so to
know if it even supports different power levels, and the registers
needed have been implemented.
Later, the SFP state machine will transition the module to higher
power:
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/sfp.c#L1995
by writing a register in the SFP.
> > I would also suggest you don't focus too much on ICE. I find it better
> > to think about an abstract system. A board with a power supply to a
> > number of SFP cages, and some cages have modules in them. What does
> > the kAPI look like, the use cases for this abstract system.
>
> My design for this API is to have an option to get and set maximum
> power that the module in the cage can consume. It's not about modifying
> module's power consumption, it's about setting restrictions for it.
>
> The use case is to let the user change maximum power in the given cage
> (so he can plug in the module with higher power consumption). Before that
> he will lower maximum power in different cage. Thanks to that the overall
> budget for the board won't be exceeded. Does it make sense for the abstract
> system you described?
So there are a few different phases here. The standard says the module
start up in low power mode.
Something needs to enumerate what the module actually supports in
terms of different power modes.
Something then needs to determine if the board/cage can support higher
power operation, that there is sufficient power budged. Budget then
needs to be allocated to the cage.
Lastly, the module needs to be told it can go to a higher power level.
I would say the current Linux SFP code is overly simple, by design. It
supports the concept of cage supplied by a dedicated regulator. There
is no sharing of power across a number of cages. Hence it just ups the
power if the board indicates the power is available and the module
support a higher power level.
However, you have a more complex setup, shared power supplies,
etc. The policy of what modules gets how much power should come from
user space. So we need user space APIs for this, and a clear
understanding of how they work. Please could you describe how i would
use ethtool to go through these phases. And how do i down grade a
modules power consumption to make more power available to another
module.
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-04 13:53 ` Andrew Lunn
@ 2024-04-09 12:20 ` Wojciech Drewek
2024-04-09 13:39 ` Andrew Lunn
0 siblings, 1 reply; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-09 12:20 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel
On 04.04.2024 15:53, Andrew Lunn wrote:
> On Thu, Apr 04, 2024 at 02:45:43PM +0200, Wojciech Drewek wrote:
>>
>>
>> On 03.04.2024 15:49, Andrew Lunn wrote:
>>>>> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
>>>>>
>>>>> actually talk to the SFP module and tell it the maximum power it can
>>>>> consume. So in this case, it is not the cage, but the module?
>>>>
>>>> It does not work that way in ice example.
>>>>>
>>>>> Or is it talking to some entity which is managing the overall power
>>>>> consumption of a number of cages, and asking it to allocate a maximum
>>>>> of 4W to this cage. It might return an error message saying there is
>>>>> no power budget left?
>>>>
>>>> That's right, we talk to firmware to set those restrictions.
>>>> In the ice implementation, the driver is responsible for checking if the
>>>> overall board budget is not exceeded.
>>>
>>> So i can get the board to agree that the cage can supply 3W to the
>>> module, but how do i then tell the module this?
>>
>> I'd assume it is not possible, if the module consumes more power
>> than maximum than the link will not come up and error will be printed.
>
> Take a look at the Linux SFP driver. In sfp_probe() is reads the DT
> property maximum-power-milliwatt:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/sfp.c#L3030
>
> When the module is inserted and probed, the modules power capabilities
> are read from the EEPROM:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/sfp.c#L2320
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/sfp.c#L1929
>
> The code looks to see what conformance level the module has, so to
> know if it even supports different power levels, and the registers
> needed have been implemented.
>
> Later, the SFP state machine will transition the module to higher
> power:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/sfp.c#L1995
>
> by writing a register in the SFP.
>
>>> I would also suggest you don't focus too much on ICE. I find it better
>>> to think about an abstract system. A board with a power supply to a
>>> number of SFP cages, and some cages have modules in them. What does
>>> the kAPI look like, the use cases for this abstract system.
>>
>> My design for this API is to have an option to get and set maximum
>> power that the module in the cage can consume. It's not about modifying
>> module's power consumption, it's about setting restrictions for it.
>>
>> The use case is to let the user change maximum power in the given cage
>> (so he can plug in the module with higher power consumption). Before that
>> he will lower maximum power in different cage. Thanks to that the overall
>> budget for the board won't be exceeded. Does it make sense for the abstract
>> system you described?
>
> So there are a few different phases here. The standard says the module
> start up in low power mode.
>
> Something needs to enumerate what the module actually supports in
> terms of different power modes.
So, this is something that would require new netlink attribute.
From what you described, SFP driver is capable of reading module's
power capabilities so it could be implemented there.
>
> Something then needs to determine if the board/cage can support higher
> power operation, that there is sufficient power budged. Budget then
> needs to be allocated to the cage.
This is something my current design supports I think. Using
ETHTOOL_A_MODULE_MAX_POWER_SET user can get what cage supports
and change it.
>
> Lastly, the module needs to be told it can go to a higher power level.
This could be done using ethtool_module_power_mode_policy I think.
>
> I would say the current Linux SFP code is overly simple, by design. It
> supports the concept of cage supplied by a dedicated regulator. There
> is no sharing of power across a number of cages. Hence it just ups the
> power if the board indicates the power is available and the module
> support a higher power level.
>
> However, you have a more complex setup, shared power supplies,
> etc. The policy of what modules gets how much power should come from
> user space. So we need user space APIs for this, and a clear
> understanding of how they work. Please could you describe how i would
> use ethtool to go through these phases. And how do i down grade a
> modules power consumption to make more power available to another
> module.
I described how I see it for each phase above.
Downgrading module's power consumption is not possible with my initial
design. You can downgrade max power for the cage and if the module
plugged in to it consumes more power than error will be printed and
link will not be established.
In the end it looks like we would need new netlink attribute to support
modification of module's power consumption.
>
> Andrew
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-09 12:20 ` Wojciech Drewek
@ 2024-04-09 13:39 ` Andrew Lunn
2024-04-12 13:21 ` Wojciech Drewek
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2024-04-09 13:39 UTC (permalink / raw)
To: Wojciech Drewek
Cc: netdev, idosch, edumazet, marcin.szycik, anthony.l.nguyen, kuba,
intel-wired-lan, pabeni, przemyslaw.kitszel, Jesse Brandeburg
> This is something my current design supports I think. Using
> ETHTOOL_A_MODULE_MAX_POWER_SET user can get what cage supports
> and change it.
> This could be done using ethtool_module_power_mode_policy I think.
All these 'I think' don't give me a warm fuzzy feeling this is a well
thought out and designed uAPI.
I assume you have ethtool patches for your new netlink attributes. So
show us the real usage. Start with an SFP in its default lower power
mode. Show us the commands to display the current status. Allocate it
more power, tell the module it can use more power, and then show us
the status after the change has been made.
Then lower the power to that cage and assign the power to a different
cage.
This is something you can later reuse in the 0/X patch describing the
big picture of what the patchset does, and it will guide others who
want to implement the same API in the Linux SFP driver, or other MAC
drivers.
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-09 13:39 ` Andrew Lunn
@ 2024-04-12 13:21 ` Wojciech Drewek
2024-04-15 22:03 ` Andrew Lunn
0 siblings, 1 reply; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-12 13:21 UTC (permalink / raw)
To: Andrew Lunn
Cc: pabeni, netdev, edumazet, marcin.szycik, anthony.l.nguyen, idosch,
kuba, intel-wired-lan, przemyslaw.kitszel
On 09.04.2024 15:39, Andrew Lunn wrote:
>> This is something my current design supports I think. Using
>> ETHTOOL_A_MODULE_MAX_POWER_SET user can get what cage supports
>> and change it.
>
>> This could be done using ethtool_module_power_mode_policy I think.
>
> All these 'I think' don't give me a warm fuzzy feeling this is a well
> thought out and designed uAPI.
>
> I assume you have ethtool patches for your new netlink attributes. So
> show us the real usage. Start with an SFP in its default lower power
> mode. Show us the commands to display the current status. Allocate it
> more power, tell the module it can use more power, and then show us
> the status after the change has been made.
Ok, but do we really need an API to switch the module between high/low power mode?
I'd assume this is done automatically e.g. when we lower max power in the cage below
module's high power mode than it should imply that the module should go to low power mode.
Same with the high power mode, if enough power is assigned to the cage than module
should go to the high power mode.
Regarding the current status and what module supports, there is -m option:
$ ethtool -m ens801f0np0
Identifier : 0x0d (QSFP+)
Extended identifier : 0x00
Extended identifier description : 1.5W max. Power consumption
Extended identifier description : No CDR in TX, No CDR in RX
Extended identifier description : High Power Class (> 3.5 W) not enabled
>
> Then lower the power to that cage and assign the power to a different
> cage.
>
> This is something you can later reuse in the 0/X patch describing the
> big picture of what the patchset does, and it will guide others who
> want to implement the same API in the Linux SFP driver, or other MAC
> drivers.
>
> Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-12 13:21 ` Wojciech Drewek
@ 2024-04-15 22:03 ` Andrew Lunn
2024-04-18 11:48 ` Wojciech Drewek
0 siblings, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2024-04-15 22:03 UTC (permalink / raw)
To: Wojciech Drewek
Cc: pabeni, netdev, edumazet, marcin.szycik, anthony.l.nguyen, idosch,
kuba, intel-wired-lan, przemyslaw.kitszel
On Fri, Apr 12, 2024 at 03:21:24PM +0200, Wojciech Drewek wrote:
>
>
> On 09.04.2024 15:39, Andrew Lunn wrote:
> >> This is something my current design supports I think. Using
> >> ETHTOOL_A_MODULE_MAX_POWER_SET user can get what cage supports
> >> and change it.
> >
> >> This could be done using ethtool_module_power_mode_policy I think.
> >
> > All these 'I think' don't give me a warm fuzzy feeling this is a well
> > thought out and designed uAPI.
> >
> > I assume you have ethtool patches for your new netlink attributes. So
> > show us the real usage. Start with an SFP in its default lower power
> > mode. Show us the commands to display the current status. Allocate it
> > more power, tell the module it can use more power, and then show us
> > the status after the change has been made.
>
> Ok, but do we really need an API to switch the module between high/low power mode?
Probably not. But you need to document that the API you are adding is
also expected to talk to the module and tell it to use more/less
power.
> Regarding the current status and what module supports, there is -m option:
> $ ethtool -m ens801f0np0
> Identifier : 0x0d (QSFP+)
> Extended identifier : 0x00
> Extended identifier description : 1.5W max. Power consumption
> Extended identifier description : No CDR in TX, No CDR in RX
> Extended identifier description : High Power Class (> 3.5 W) not enabled
So you can make this part of your commit message. Show this. Invoke
your new ethtool option, then show this again with the module
reporting a higher power consumption. The reduce the power using
ethtool and show the power consumption has reduced.
Also, in the ethtool-netlink.rst file, clearly document what the API
is doing, so that somebody else can implement it for another device.
Please also document hotplug behaviour. Say I use your new API to
increase the power to 3.5W. I then eject the module. Does the
available power automatically get put back into the pool? When i
reinsert the module, it will be in low power class, and i need to
issue the ethtool command again to increase its power?
Andrew
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support
2024-04-15 22:03 ` Andrew Lunn
@ 2024-04-18 11:48 ` Wojciech Drewek
0 siblings, 0 replies; 34+ messages in thread
From: Wojciech Drewek @ 2024-04-18 11:48 UTC (permalink / raw)
To: Andrew Lunn
Cc: pabeni, netdev, edumazet, marcin.szycik, anthony.l.nguyen, idosch,
kuba, intel-wired-lan, przemyslaw.kitszel
On 16.04.2024 00:03, Andrew Lunn wrote:
> On Fri, Apr 12, 2024 at 03:21:24PM +0200, Wojciech Drewek wrote:
>>
>>
>> On 09.04.2024 15:39, Andrew Lunn wrote:
>>>> This is something my current design supports I think. Using
>>>> ETHTOOL_A_MODULE_MAX_POWER_SET user can get what cage supports
>>>> and change it.
>>>
>>>> This could be done using ethtool_module_power_mode_policy I think.
>>>
>>> All these 'I think' don't give me a warm fuzzy feeling this is a well
>>> thought out and designed uAPI.
>>>
>>> I assume you have ethtool patches for your new netlink attributes. So
>>> show us the real usage. Start with an SFP in its default lower power
>>> mode. Show us the commands to display the current status. Allocate it
>>> more power, tell the module it can use more power, and then show us
>>> the status after the change has been made.
>>
>> Ok, but do we really need an API to switch the module between high/low power mode?
>
> Probably not. But you need to document that the API you are adding is
> also expected to talk to the module and tell it to use more/less
> power.
>
>> Regarding the current status and what module supports, there is -m option:
>> $ ethtool -m ens801f0np0
>> Identifier : 0x0d (QSFP+)
>> Extended identifier : 0x00
>> Extended identifier description : 1.5W max. Power consumption
>> Extended identifier description : No CDR in TX, No CDR in RX
>> Extended identifier description : High Power Class (> 3.5 W) not enabled
>
> So you can make this part of your commit message. Show this. Invoke
> your new ethtool option, then show this again with the module
> reporting a higher power consumption. The reduce the power using
> ethtool and show the power consumption has reduced.
>
> Also, in the ethtool-netlink.rst file, clearly document what the API
> is doing, so that somebody else can implement it for another device.
>
> Please also document hotplug behaviour. Say I use your new API to
> increase the power to 3.5W. I then eject the module. Does the
> available power automatically get put back into the pool? When i
> reinsert the module, it will be in low power class, and i need to
> issue the ethtool command again to increase its power?
Ok, I'll answer all of those questions in the documentation included
in the 2nd version of the patchset.
>
> Andrew
>
^ permalink raw reply [flat|nested] 34+ messages in thread