Skip to content

Commit

Permalink
ethtool: netlink: handle SET intro/outro in the common code
Browse files Browse the repository at this point in the history 
Most ethtool SET callbacks follow the same general structure.

  ethnl_parse_header_dev_get()
  rtnl_lock()
  ethnl_ops_begin()

  ... do stuff ...

  ethtool_notify()
  ethnl_ops_complete()
  rtnl_unlock()
  ethnl_parse_header_dev_put()

This leads to a lot of copy / pasted code an bugs when people
mis-handle the error path.

Add a generic implementation of this pattern with a .set callback
in struct ethnl_request_ops called to "do stuff".

Also add an optional .set_validate which is called before
ethnl_ops_begin() -- a lot of implementations do basic request
capability / sanity checking at that point.

Because we want to avoid generating the notification when
no change happened - adopt a slightly hairy return values:
 - 0 means nothing to do (no notification)
 - 1 means done / continue
 - negative error codes on error

Reuse .hdr_attr from struct ethnl_request_ops, GET and SET
use the same attr spaces in all cases.

Convert pause as an example (and to avoid unused function warnings).

Signed-off-by: Jakub Kicinski <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
  • Loading branch information
@kuba-moo @davem330
kuba-moo authored and davem330 committed Jan 27, 2023
1 parent c766e07 commit 99132b6
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 50 deletions.
49 changes: 48 additions & 1 deletion net/ethtool/netlink.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
[ETHTOOL_MSG_CHANNELS_GET] = &ethnl_channels_request_ops,
[ETHTOOL_MSG_COALESCE_GET] = &ethnl_coalesce_request_ops,
[ETHTOOL_MSG_PAUSE_GET] = &ethnl_pause_request_ops,
[ETHTOOL_MSG_PAUSE_SET] = &ethnl_pause_request_ops,
[ETHTOOL_MSG_EEE_GET] = &ethnl_eee_request_ops,
[ETHTOOL_MSG_FEC_GET] = &ethnl_fec_request_ops,
[ETHTOOL_MSG_TSINFO_GET] = &ethnl_tsinfo_request_ops,
Expand Down Expand Up @@ -591,6 +592,52 @@ static int ethnl_default_done(struct netlink_callback *cb)
return 0;
}

static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
{
const struct ethnl_request_ops *ops;
struct ethnl_req_info req_info = {};
const u8 cmd = info->genlhdr->cmd;
int ret;

ops = ethnl_default_requests[cmd];
if (WARN_ONCE(!ops, "cmd %u has no ethnl_request_ops\n", cmd))
return -EOPNOTSUPP;
if (GENL_REQ_ATTR_CHECK(info, ops->hdr_attr))
return -EINVAL;

ret = ethnl_parse_header_dev_get(&req_info, info->attrs[ops->hdr_attr],
genl_info_net(info), info->extack,
true);
if (ret < 0)
return ret;

if (ops->set_validate) {
ret = ops->set_validate(&req_info, info);
/* 0 means nothing to do */
if (ret <= 0)
goto out_dev;
}

rtnl_lock();
ret = ethnl_ops_begin(req_info.dev);
if (ret < 0)
goto out_rtnl;

ret = ops->set(&req_info, info);
if (ret <= 0)
goto out_ops;
ethtool_notify(req_info.dev, ops->set_ntf_cmd, NULL);

ret = 0;
out_ops:
ethnl_ops_complete(req_info.dev);
out_rtnl:
rtnl_unlock();
out_dev:
ethnl_parse_header_dev_put(&req_info);
return ret;
}

static const struct ethnl_request_ops *
ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
[ETHTOOL_MSG_LINKINFO_NTF] = &ethnl_linkinfo_request_ops,
Expand Down Expand Up @@ -921,7 +968,7 @@ static const struct genl_ops ethtool_genl_ops[] = {
{
.cmd = ETHTOOL_MSG_PAUSE_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_pause,
.doit = ethnl_default_set_doit,
.policy = ethnl_pause_set_policy,
.maxattr = ARRAY_SIZE(ethnl_pause_set_policy) - 1,
},
Expand Down
22 changes: 20 additions & 2 deletions net/ethtool/netlink.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,13 +284,14 @@ int ethnl_ops_begin(struct net_device *dev);
void ethnl_ops_complete(struct net_device *dev);

/**
* struct ethnl_request_ops - unified handling of GET requests
* struct ethnl_request_ops - unified handling of GET and SET requests
* @request_cmd: command id for request (GET)
* @reply_cmd: command id for reply (GET_REPLY)
* @hdr_attr: attribute type for request header
* @req_info_size: size of request info
* @reply_data_size: size of reply data
* @allow_nodev_do: allow non-dump request with no device identification
* @set_ntf_cmd: notification to generate on changes (SET)
* @parse_request:
* Parse request except common header (struct ethnl_req_info). Common
* header is already filled on entry, the rest up to @repdata_offset
Expand Down Expand Up @@ -319,6 +320,18 @@ void ethnl_ops_complete(struct net_device *dev);
* used e.g. to free any additional data structures outside the main
* structure which were allocated by ->prepare_data(). When processing
* dump requests, ->cleanup() is called for each message.
* @set_validate:
* Check if set operation is supported for a given device, and perform
* extra input checks. Expected return values:
* - 0 if the operation is a noop for the device (rare)
* - 1 if operation should proceed to calling @set
* - negative errno on errors
* Called without any locks, just a reference on the netdev.
* @set:
* Execute the set operation. The implementation should return
* - 0 if no configuration has changed
* - 1 if configuration changed and notification should be generated
* - negative errno on errors
*
* Description of variable parts of GET request handling when using the
* unified infrastructure. When used, a pointer to an instance of this
Expand All @@ -335,6 +348,7 @@ struct ethnl_request_ops {
unsigned int req_info_size;
unsigned int reply_data_size;
bool allow_nodev_do;
u8 set_ntf_cmd;

int (*parse_request)(struct ethnl_req_info *req_info,
struct nlattr **tb,
Expand All @@ -348,6 +362,11 @@ struct ethnl_request_ops {
const struct ethnl_req_info *req_info,
const struct ethnl_reply_data *reply_data);
void (*cleanup_data)(struct ethnl_reply_data *reply_data);

int (*set_validate)(struct ethnl_req_info *req_info,
struct genl_info *info);
int (*set)(struct ethnl_req_info *req_info,
struct genl_info *info);
};

/* request handlers */
Expand Down Expand Up @@ -432,7 +451,6 @@ int ethnl_set_privflags(struct sk_buff *skb, struct genl_info *info);
int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info);
int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info);
int ethnl_set_coalesce(struct sk_buff *skb, struct genl_info *info);
int ethnl_set_pause(struct sk_buff *skb, struct genl_info *info);
int ethnl_set_eee(struct sk_buff *skb, struct genl_info *info);
int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info);
Expand Down
79 changes: 32 additions & 47 deletions net/ethtool/pause.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,19 +161,6 @@ static int pause_fill_reply(struct sk_buff *skb,
return 0;
}

const struct ethnl_request_ops ethnl_pause_request_ops = {
.request_cmd = ETHTOOL_MSG_PAUSE_GET,
.reply_cmd = ETHTOOL_MSG_PAUSE_GET_REPLY,
.hdr_attr = ETHTOOL_A_PAUSE_HEADER,
.req_info_size = sizeof(struct pause_req_info),
.reply_data_size = sizeof(struct pause_reply_data),

.parse_request = pause_parse_request,
.prepare_data = pause_prepare_data,
.reply_size = pause_reply_size,
.fill_reply = pause_fill_reply,
};

/* PAUSE_SET */

const struct nla_policy ethnl_pause_set_policy[] = {
Expand All @@ -184,51 +171,49 @@ const struct nla_policy ethnl_pause_set_policy[] = {
[ETHTOOL_A_PAUSE_TX] = { .type = NLA_U8 },
};

int ethnl_set_pause(struct sk_buff *skb, struct genl_info *info)
static int
ethnl_set_pause_validate(struct ethnl_req_info *req_info,
struct genl_info *info)
{
const struct ethtool_ops *ops = req_info->dev->ethtool_ops;

return ops->get_pauseparam && ops->set_pauseparam ? 1 : -EOPNOTSUPP;
}

static int
ethnl_set_pause(struct ethnl_req_info *req_info, struct genl_info *info)
{
struct net_device *dev = req_info->dev;
struct ethtool_pauseparam params = {};
struct ethnl_req_info req_info = {};
struct nlattr **tb = info->attrs;
const struct ethtool_ops *ops;
struct net_device *dev;
bool mod = false;
int ret;

ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_PAUSE_HEADER],
genl_info_net(info), info->extack,
true);
if (ret < 0)
return ret;
dev = req_info.dev;
ops = dev->ethtool_ops;
ret = -EOPNOTSUPP;
if (!ops->get_pauseparam || !ops->set_pauseparam)
goto out_dev;

rtnl_lock();
ret = ethnl_ops_begin(dev);
if (ret < 0)
goto out_rtnl;
ops->get_pauseparam(dev, &params);
dev->ethtool_ops->get_pauseparam(dev, &params);

ethnl_update_bool32(&params.autoneg, tb[ETHTOOL_A_PAUSE_AUTONEG], &mod);
ethnl_update_bool32(&params.rx_pause, tb[ETHTOOL_A_PAUSE_RX], &mod);
ethnl_update_bool32(&params.tx_pause, tb[ETHTOOL_A_PAUSE_TX], &mod);
ret = 0;
if (!mod)
goto out_ops;
return 0;

ret = dev->ethtool_ops->set_pauseparam(dev, &params);
if (ret < 0)
goto out_ops;
ethtool_notify(dev, ETHTOOL_MSG_PAUSE_NTF, NULL);

out_ops:
ethnl_ops_complete(dev);
out_rtnl:
rtnl_unlock();
out_dev:
ethnl_parse_header_dev_put(&req_info);
return ret;
return ret < 0 ? ret : 1;
}

const struct ethnl_request_ops ethnl_pause_request_ops = {
.request_cmd = ETHTOOL_MSG_PAUSE_GET,
.reply_cmd = ETHTOOL_MSG_PAUSE_GET_REPLY,
.hdr_attr = ETHTOOL_A_PAUSE_HEADER,
.req_info_size = sizeof(struct pause_req_info),
.reply_data_size = sizeof(struct pause_reply_data),

.parse_request = pause_parse_request,
.prepare_data = pause_prepare_data,
.reply_size = pause_reply_size,
.fill_reply = pause_fill_reply,

.set_validate = ethnl_set_pause_validate,
.set = ethnl_set_pause,
.set_ntf_cmd = ETHTOOL_MSG_PAUSE_NTF,
};

0 comments on commit 99132b6

Please sign in to comment.