netdev - RE: [PATCH net-next repost v2 1/7] ethtool: Extend link modes settings uAPI with lanes

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR12MB451691B3A3E6EE51B87AD75FD8AB0@DM6PR12MB4516.namprd12.prod.outlook.com>
Date:   Mon, 11 Jan 2021 14:00:20 +0000
From:   Danielle Ratson <danieller@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Danielle Ratson <danieller@...lanox.com>
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 repost v2 1/7] ethtool: Extend link modes
 settings uAPI with lanes



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Friday, January 8, 2021 2:35 AM
> To: Danielle Ratson <danieller@...lanox.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>; Danielle Ratson <danieller@...dia.com>
> Subject: Re: [PATCH net-next repost v2 1/7] ethtool: Extend link modes settings uAPI with lanes
> 
> On Wed,  6 Jan 2021 15:06:16 +0200 Danielle Ratson wrote:
> > From: Danielle Ratson <danieller@...dia.com>
> >
> > 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.
> 
> > @@ -420,6 +423,7 @@ struct ethtool_pause_stats {
> >   * of the generic netdev features interface.
> >   */
> >  struct ethtool_ops {
> > +	u32     capabilities;
> 
> An appropriately named bitfield seems better. Alternatively maybe let the driver specify which lane counts it can accept?

Not sure what did you mean, can you please explain?
Thanks!

> 
> And please remember to add the kdoc.
> 
> >  	u32	supported_coalesce_params;
> >  	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
> >  	int	(*get_regs_len)(struct net_device *);
> 
> > @@ -274,16 +277,17 @@ const struct nla_policy ethnl_linkmodes_set_policy[] = {
> >  	[ETHTOOL_A_LINKMODES_SPEED]		= { .type = NLA_U32 },
> >  	[ETHTOOL_A_LINKMODES_DUPLEX]		= { .type = NLA_U8 },
> >  	[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG]	= { .type = NLA_U8 },
> > +	[ETHTOOL_A_LINKMODES_LANES]		= { .type = NLA_U32 },
> 
> Please set the min and max for the policy, so userspace can at least see that part.
> 
> > +static bool ethnl_validate_lanes_cfg(u32 cfg) {
> > +	switch (cfg) {
> > +	case 1:
> > +	case 2:
> > +	case 4:
> > +	case 8:
> > +		return true;
> 
> And with the policy checking min and max this can be turned into a simple is_power_of_2() call.
> 
> > +	}
> > +
> > +	return false;
> > +}