| Message ID | 20231201-feature_poe-v2-4-56d8cac607fa@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Add support for Power over Ethernet (PoE) | expand |
> @@ -143,6 +150,43 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info) > return -EOPNOTSUPP; > } > > + if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > + !tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]) > + return 0; -EINVAL? Is there a real use case for not passing either of them? > + > + if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > + !(pse_get_types(phydev->psec) & PSE_PODL)) { > + NL_SET_ERR_MSG_ATTR(info->extack, > + tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL], > + "setting PSE PoDL admin control not supported"); > + return -EOPNOTSUPP; > + } > + if (tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL] && > + !(pse_get_types(phydev->psec) & PSE_C33)) { > + NL_SET_ERR_MSG_ATTR(info->extack, > + tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL], > + "setting PSE PoE admin control not supported"); This probably should be C33, not PoE? I guess it depends on what the user space tools are using. Andrew
On Sun, 3 Dec 2023 19:45:18 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > @@ -143,6 +150,43 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct > > genl_info *info) return -EOPNOTSUPP; > > } > > > > + if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > > + !tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]) > > + return 0; > > -EINVAL? Is there a real use case for not passing either of them? No indeed. > > + > > + if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > > + !(pse_get_types(phydev->psec) & PSE_PODL)) { > > + NL_SET_ERR_MSG_ATTR(info->extack, > > + tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL], > > + "setting PSE PoDL admin control not > > supported"); > > + return -EOPNOTSUPP; > > + } > > + if (tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL] && > > + !(pse_get_types(phydev->psec) & PSE_C33)) { > > + NL_SET_ERR_MSG_ATTR(info->extack, > > + tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL], > > + "setting PSE PoE admin control not > > supported"); > > This probably should be C33, not PoE? > > I guess it depends on what the user space tools are using. Yes, I have hesitated on replacing that one. If you prefer c33 in the log, I will change it in next version Regards,
On Sun, Dec 03, 2023 at 07:45:18PM +0100, Andrew Lunn wrote: > > @@ -143,6 +150,43 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info) > > return -EOPNOTSUPP; > > } > > > > + if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > > + !tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]) > > + return 0; > > -EINVAL? Is there a real use case for not passing either of them? > > > + > > + if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > > + !(pse_get_types(phydev->psec) & PSE_PODL)) { > > + NL_SET_ERR_MSG_ATTR(info->extack, > > + tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL], > > + "setting PSE PoDL admin control not supported"); > > + return -EOPNOTSUPP; > > + } > > + if (tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL] && > > + !(pse_get_types(phydev->psec) & PSE_C33)) { > > + NL_SET_ERR_MSG_ATTR(info->extack, > > + tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL], > > + "setting PSE PoE admin control not supported"); > > This probably should be C33, not PoE? > > I guess it depends on what the user space tools are using. The same problem is in the documentation. Mixing different naming schemes is problematic. Even unmixed this "PoE" is not really suitable for most use cases. Expanding this abbreviations make it probably more clear: - PSE PoE - Power Source Equipment Power over Ethernet - C33 PSE - Clause 33 Power Source Equipment
On Fri, Dec 01, 2023 at 06:10:26PM +0100, Kory Maincent wrote: > @@ -1741,6 +1757,7 @@ Request contents: > ====================================== ====== ============================= > ``ETHTOOL_A_PSE_HEADER`` nested request header > ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` u32 Control PoDL PSE Admin state > + ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` u32 Control PSE Admin state > ====================================== ====== ============================= I get htmldocs warning: ``` Documentation/networking/ethtool-netlink.rst:1760: WARNING: Malformed table. Text in column margin in table line 4. ====================================== ====== ============================= ``ETHTOOL_A_PSE_HEADER`` nested request header ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` u32 Control PoDL PSE Admin state ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` u32 Control PSE Admin state ====================================== ====== ============================= ``` I have to fix it up: ---- >8 ---- diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst index e02a7dabc673e2..8da5068105e3e9 100644 --- a/Documentation/networking/ethtool-netlink.rst +++ b/Documentation/networking/ethtool-netlink.rst @@ -1757,7 +1757,7 @@ Request contents: ====================================== ====== ============================= ``ETHTOOL_A_PSE_HEADER`` nested request header ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` u32 Control PoDL PSE Admin state - ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` u32 Control PSE Admin state + ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` u32 Control PSE Admin state ====================================== ====== ============================= When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is used Thanks.
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst index 2540c70952ff..e02a7dabc673 100644 --- a/Documentation/networking/ethtool-netlink.rst +++ b/Documentation/networking/ethtool-netlink.rst @@ -1711,6 +1711,10 @@ Kernel response contents: PSE functions ``ETHTOOL_A_PODL_PSE_PW_D_STATUS`` u32 power detection status of the PoDL PSE. + ``ETHTOOL_A_C33_PSE_ADMIN_STATE`` u32 Operational state of the PoE + PSE functions. + ``ETHTOOL_A_C33_PSE_PW_D_STATUS`` u32 power detection status of the + PoE PSE. ====================================== ====== ============================= When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_STATE`` attribute identifies @@ -1722,6 +1726,12 @@ aPoDLPSEAdminState. Possible values are: .. kernel-doc:: include/uapi/linux/ethtool.h :identifiers: ethtool_podl_pse_admin_state +The same goes for ``ETHTOOL_A_C33_PSE_ADMIN_STATE`` implementing +``IEEE 802.3-2022`` 30.9.1.1.2 aPSEAdminState. + +.. kernel-doc:: include/uapi/linux/ethtool.h + :identifiers: ethtool_c33_pse_admin_state + When set, the optional ``ETHTOOL_A_PODL_PSE_PW_D_STATUS`` attribute identifies the power detection status of the PoDL PSE. The status depend on internal PSE state machine and automatic PD classification support. This option is @@ -1731,6 +1741,12 @@ Possible values are: .. kernel-doc:: include/uapi/linux/ethtool.h :identifiers: ethtool_podl_pse_pw_d_status +The same goes for ``ETHTOOL_A_C33_PSE_ADMIN_PW_D_STATUS`` implementing +``IEEE 802.3-2022`` 30.9.1.1.5 aPSEPowerDetectionStatus. + +.. kernel-doc:: include/uapi/linux/ethtool.h + :identifiers: ethtool_c33_pse_pw_d_status + PSE_SET ======= @@ -1741,6 +1757,7 @@ Request contents: ====================================== ====== ============================= ``ETHTOOL_A_PSE_HEADER`` nested request header ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` u32 Control PoDL PSE Admin state + ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` u32 Control PSE Admin state ====================================== ====== ============================= When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is used @@ -1748,6 +1765,9 @@ to control PoDL PSE Admin functions. This option is implementing ``IEEE 802.3-2018`` 30.15.1.2.1 acPoDLPSEAdminControl. See ``ETHTOOL_A_PODL_PSE_ADMIN_STATE`` for supported values. +The same goes for ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` implementing +``IEEE 802.3-2022`` 30.9.1.2.1 acPSEAdminControl. + RSS_GET ======= diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c index aef57a058f0d..7a98dc029c4f 100644 --- a/net/ethtool/pse-pd.c +++ b/net/ethtool/pse-pd.c @@ -82,6 +82,10 @@ static int pse_reply_size(const struct ethnl_req_info *req_base, len += nla_total_size(sizeof(u32)); /* _PODL_PSE_ADMIN_STATE */ if (st->podl_pw_status > 0) len += nla_total_size(sizeof(u32)); /* _PODL_PSE_PW_D_STATUS */ + if (st->c33_admin_state > 0) + len += nla_total_size(sizeof(u32)); /* _C33_PSE_ADMIN_STATE */ + if (st->c33_pw_status > 0) + len += nla_total_size(sizeof(u32)); /* _C33_PSE_PW_D_STATUS */ return len; } @@ -103,6 +107,16 @@ static int pse_fill_reply(struct sk_buff *skb, st->podl_pw_status)) return -EMSGSIZE; + if (st->c33_admin_state > 0 && + nla_put_u32(skb, ETHTOOL_A_C33_PSE_ADMIN_STATE, + st->c33_admin_state)) + return -EMSGSIZE; + + if (st->c33_pw_status > 0 && + nla_put_u32(skb, ETHTOOL_A_C33_PSE_PW_D_STATUS, + st->c33_pw_status)) + return -EMSGSIZE; + return 0; } @@ -113,25 +127,18 @@ const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1] = { [ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] = NLA_POLICY_RANGE(NLA_U32, ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED, ETHTOOL_PODL_PSE_ADMIN_STATE_ENABLED), + [ETHTOOL_A_C33_PSE_ADMIN_CONTROL] = + NLA_POLICY_RANGE(NLA_U32, ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED, + ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED), }; static int ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info) -{ - return !!info->attrs[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]; -} - -static int -ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info) { struct net_device *dev = req_info->dev; - struct pse_control_config config = {}; struct nlattr **tb = info->attrs; struct phy_device *phydev; - /* this values are already validated by the ethnl_pse_set_policy */ - config.podl_admin_control = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]); - phydev = dev->phydev; if (!phydev) { NL_SET_ERR_MSG(info->extack, "No PHY is attached"); @@ -143,6 +150,43 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info) return -EOPNOTSUPP; } + if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && + !tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]) + return 0; + + if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && + !(pse_get_types(phydev->psec) & PSE_PODL)) { + NL_SET_ERR_MSG_ATTR(info->extack, + tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL], + "setting PSE PoDL admin control not supported"); + return -EOPNOTSUPP; + } + if (tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL] && + !(pse_get_types(phydev->psec) & PSE_C33)) { + NL_SET_ERR_MSG_ATTR(info->extack, + tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL], + "setting PSE PoE admin control not supported"); + return -EOPNOTSUPP; + } + + return 1; +} + +static int +ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info) +{ + struct net_device *dev = req_info->dev; + struct pse_control_config config = {}; + struct nlattr **tb = info->attrs; + struct phy_device *phydev; + + phydev = dev->phydev; + /* These values are already validated by the ethnl_pse_set_policy */ + if (pse_get_types(phydev->psec) & PSE_PODL) + config.podl_admin_control = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]); + if (pse_get_types(phydev->psec) & PSE_C33) + config.c33_admin_control = nla_get_u32(tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]); + /* Return errno directly - PSE has no notification */ return pse_ethtool_set_config(phydev->psec, info->extack, &config); }
Add PSE PoE interface support in the ethtool pse command. Sponsored-by: Dent Project <dentproject@linuxfoundation.org> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> --- Changes in v2: - Follow the "c33" PoE prefix naming change. --- Documentation/networking/ethtool-netlink.rst | 20 +++++++++ net/ethtool/pse-pd.c | 64 +++++++++++++++++++++++----- 2 files changed, 74 insertions(+), 10 deletions(-)