| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM6PR12MB4516C3011B5D158930444203D8BD9@DM6PR12MB4516.namprd12.prod.outlook.com>
Date: Mon, 25 Jan 2021 15:53:24 +0000
From: Danielle Ratson <danieller@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
Jiri Pirko <jiri@...dia.com>,
"andrew@...n.ch" <andrew@...n.ch>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"mkubecek@...e.cz" <mkubecek@...e.cz>, mlxsw <mlxsw@...dia.com>,
Ido Schimmel <idosch@...dia.com>
Subject: RE: [PATCH net-next v3 1/7] ethtool: Extend link modes settings uAPI
with lanes
> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Friday, January 22, 2021 5:45 AM
> To: Danielle Ratson <danieller@...dia.com>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; Jiri Pirko <jiri@...dia.com>; andrew@...n.ch; f.fainelli@...il.com;
> mkubecek@...e.cz; mlxsw <mlxsw@...dia.com>; Ido Schimmel <idosch@...dia.com>
> Subject: Re: [PATCH net-next v3 1/7] ethtool: Extend link modes settings uAPI with lanes
>
> On Wed, 20 Jan 2021 11:37:07 +0200 Danielle Ratson wrote:
> > Currently, when auto negotiation is on, the user can advertise all the
> > linkmodes which correspond to a specific speed, but does not have a
> > similar selector for the number of lanes. This is significant when a
> > specific speed can be achieved using different number of lanes. For
> > example, 2x50 or 4x25.
> >
> > Add 'ETHTOOL_A_LINKMODES_LANES' attribute and expand 'struct
> > ethtool_link_settings' with lanes field in order to implement a new
> > lanes-selector that will enable the user to advertise a specific
> > number of lanes as well.
> >
> > When auto negotiation is off, lanes parameter can be forced only if
> > the driver supports it. Add a capability bit in 'struct ethtool_ops'
> > that allows ethtool know if the driver can handle the lanes parameter
> > when auto negotiation is off, so if it does not, an error message will
> > be returned when trying to set lanes.
>
> > Signed-off-by: Danielle Ratson <danieller@...dia.com>
>
> > diff --git a/include/uapi/linux/ethtool.h
> > b/include/uapi/linux/ethtool.h index cde753bb2093..80edae2c24f7 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -1738,6 +1738,8 @@ static inline int ethtool_validate_speed(__u32 speed)
> > return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN; }
> >
> > +#define ETHTOOL_LANES_UNKNOWN 0
>
> I already complained about these unnecessary uAPI constants, did you reply to that and I missed it?
I guess I missed it, sorry, will fix.
>
> Don't report the nlattr if it's unknown, we have netlink now, those constants are from times when we returned structures and all
> fields had to have a value.
>
> > /* Duplex, half or full. */
> > #define DUPLEX_HALF 0x00
> > #define DUPLEX_FULL 0x01
> > diff --git a/include/uapi/linux/ethtool_netlink.h
> > b/include/uapi/linux/ethtool_netlink.h
> > index e2bf36e6964b..a286635ac9b8 100644
> > --- a/include/uapi/linux/ethtool_netlink.h
> > +++ b/include/uapi/linux/ethtool_netlink.h
> > @@ -227,6 +227,7 @@ enum {
> > ETHTOOL_A_LINKMODES_DUPLEX, /* u8 */
> > ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG, /* u8 */
> > ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE, /* u8 */
> > + ETHTOOL_A_LINKMODES_LANES, /* u32 */
> >
> > /* add new constants above here */
> > __ETHTOOL_A_LINKMODES_CNT,
> > diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c index
> > c5bcb9abc8b9..fb7d73250864 100644
> > --- a/net/ethtool/linkmodes.c
> > +++ b/net/ethtool/linkmodes.c
> > @@ -152,12 +152,14 @@ const struct ethnl_request_ops
> > ethnl_linkmodes_request_ops = {
> >
> > struct link_mode_info {
> > int speed;
> > + u32 lanes;
>
> This is not uapi, we can make it u8 now, save a few (hundred?) bytes of memory and bump it to u16 later.
>
> > u8 duplex;
> > };
>
> > @@ -353,10 +358,39 @@ static int ethnl_update_linkmodes(struct
> > genl_info *info, struct nlattr **tb,
> >
> > *mod = false;
> > req_speed = tb[ETHTOOL_A_LINKMODES_SPEED];
> > + req_lanes = tb[ETHTOOL_A_LINKMODES_LANES];
> > req_duplex = tb[ETHTOOL_A_LINKMODES_DUPLEX];
> >
> > ethnl_update_u8(&lsettings->autoneg, tb[ETHTOOL_A_LINKMODES_AUTONEG],
> > mod);
> > +
> > + if (req_lanes) {
> > + u32 lanes_cfg = nla_get_u32(tb[ETHTOOL_A_LINKMODES_LANES]);
>
> req_lanes == tb[ETHTOOL_A_LINKMODES_LANES], right?
Yes, but req_lanes is a bool and doesn't fit to nla_get_u32. Do you want me to change the req_lanes type and name?
>
> Please use req_lanes variable where possible.
>
> > +
> > + if (!is_power_of_2(lanes_cfg)) {
> > + NL_SET_ERR_MSG_ATTR(info->extack,
> > + tb[ETHTOOL_A_LINKMODES_LANES],
> > + "lanes value is invalid");
> > + return -EINVAL;
> > + }
> > +
> > + /* If autoneg is off and lanes parameter is not supported by the
> > + * driver, return an error.
> > + */
> > + if (!lsettings->autoneg &&
> > + !dev->ethtool_ops->cap_link_lanes_supported) {
> > + NL_SET_ERR_MSG_ATTR(info->extack,
> > + tb[ETHTOOL_A_LINKMODES_LANES],
> > + "lanes configuration not supported by device");
> > + return -EOPNOTSUPP;
> > + }
>
> This validation does not depend on the current settings at all, it's just input validation, it can be done before rtnl_lock is taken (in a
> new function).
>
> You can move ethnl_validate_master_slave_cfg() to that function as well (as a cleanup before this patch).
Do you mean to move the ethnl_validate_master_slave_cfg() if from that function? Doesn't it depend on the current settings, as opposed to the supported lanes param that you wanted me to move as well?
Not sure I understand the second part of the request...
>
> > + } else if (!lsettings->autoneg) {
> > + /* If autoneg is off and lanes parameter is not passed from user,
> > + * set the lanes parameter to UNKNOWN.
> > + */
> > + ksettings->lanes = ETHTOOL_LANES_UNKNOWN;
> > + }
> > +
> > ret = ethnl_update_bitset(ksettings->link_modes.advertising,
> > __ETHTOOL_LINK_MODE_MASK_NBITS,
> > tb[ETHTOOL_A_LINKMODES_OURS], link_mode_names,
Powered by blists - more mailing lists