On Mon, 25 Jan 2021 15:53:24 +0000 Danielle Ratson wrote:
> > > @@ -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?
Ah, yes please.
> > 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?
Yes, to a separate helper.
> 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...
Sorry maybe I quoted a little too much context form the patch.
A helper like this:
static int ethnl_check_linkmodes(...)
{
const struct nlattr *master_slave_cfg;
master_slave_cfg = tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG];
if (master_slave_cfg &&
!ethnl_validate_master_slave_cfg(nla_get_u8(master_slave_cfg))) {
NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
"master/slave value is invalid");
return -EOPNOTSUPP;
}
lanes_cfg = ...
if (!is_power_of_2(...lanes_cfg)) {
...
return -EINVAL;
}
return 0;
}
Which you can call before the device reference is taken:
int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info)
{
struct ethtool_link_ksettings ksettings = {};
struct ethnl_req_info req_info = {};
struct nlattr **tb = info->attrs;
struct net_device *dev;
bool mod = false;
int ret;
+ ret = ethnl_check_linkmodes(tb);
+ if (ret)
+ return ret;
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_LINKMODES_HEADER],
genl_info_net(info), info->extack,
true);
if (ret < 0)
return ret;
But please make sure that you move the master_slave_cfg check in a
separate patch.
