Re: [PATCH net-next v1 3/9] net: ethtool: netlink: Add support for triggering a cable test

[Andrew Lunn <andrew@lunn.ch>]

> > +const struct ethnl_request_ops ethnl_cable_test_act_ops = {
> > +	.request_cmd		= ETHTOOL_MSG_CABLE_TEST_ACT,
> > +	.reply_cmd		= ETHTOOL_MSG_CABLE_TEST_ACT_REPLY,
> > +	.hdr_attr		= ETHTOOL_A_CABLE_TEST_HEADER,
> > +	.max_attr		= ETHTOOL_A_CABLE_TEST_MAX,
> > +	.req_info_size		= sizeof(struct cable_test_req_info),
> > +	.reply_data_size	= sizeof(struct cable_test_reply_data),
> > +	.request_policy		= cable_test_get_policy,
> > +};
> > +
> 
> As you register ethnl_act_cable_test() as doit handler and don't use any
> of ethnl_default_*() handlers, you don't need to define
> ethnl_cable_test_act_ops (and also struct cable_test_req_info and struct
> cable_test_reply_data). These would be only used by default doit/dumpit
> handlers and default notification handler.

O.K, than

> 
> > +/* CABLE_TEST_ACT */
> > +
> > +static const struct nla_policy
> > +cable_test_set_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
> > +	[ETHTOOL_A_CABLE_TEST_UNSPEC]		= { .type = NLA_REJECT },
> > +	[ETHTOOL_A_CABLE_TEST_HEADER]		= { .type = NLA_NESTED },
> > +};
> 
> This should be probably rather named cable_test_act_policy - or maybe
> cable_test_policy would suffice as you have only one request message
> type (I've been using *_get_policy for *_GET request and *_set_policy
> for *_SET).

I probably just cut/paste and changes the name, but not set to act. I
will fix this.

> > +int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +	struct nlattr *tb[ETHTOOL_A_CABLE_TEST_MAX + 1];
> > +	struct ethnl_req_info req_info = {};
> > +	struct net_device *dev;
> > +	int ret;
> > +
> > +	ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
> > +			  ETHTOOL_A_CABLE_TEST_MAX,
> > +			  cable_test_set_policy, info->extack);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ethnl_parse_header_dev_get(&req_info,
> > +					 tb[ETHTOOL_A_CABLE_TEST_HEADER],
> > +					 genl_info_net(info), info->extack,
> > +					 true);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	dev = req_info.dev;
> > +	if (!dev->phydev) {
> > +		ret = -EOPNOTSUPP;
> > +		goto out_dev_put;
> > +	}
> > +
> > +	rtnl_lock();
> > +	ret = ethnl_ops_begin(dev);
> > +	if (ret < 0)
> > +		goto out_rtnl;
> > +
> > +	ret = phy_start_cable_test(dev->phydev, info->extack);
> > +
> > +	ethnl_ops_complete(dev);
> > +out_rtnl:
> > +	rtnl_unlock();
> > +out_dev_put:
> > +	dev_put(dev);
> > +	return ret;
> > +}
> 
> As you don't send a reply message, you don't need
> ETHTOOL_MSG_CABLE_TEST_ACT_REPLY either (we may introduce it later if
> there is a reply message).

One thing i was thinking about is sending user space a cookie at this
point, to help pair the request to the multicasted results. Then the
reply would be useful.

      Andrew