Messages in this thread | | | Date | Thu, 10 Oct 2019 17:59:55 +0200 | From | Jiri Pirko <> | Subject | Re: [PATCH net-next v7 12/17] ethtool: provide link settings with LINKINFO_GET request |
| |
Wed, Oct 09, 2019 at 10:59:37PM CEST, mkubecek@suse.cz wrote:
[...]
>+/* prepare_data() handler */
Not sure how valuable are comments like this...
>+static int linkinfo_prepare(const struct ethnl_req_info *req_base, >+ struct ethnl_reply_data *reply_base, >+ struct genl_info *info) >+{ >+ struct linkinfo_reply_data *data = >+ container_of(reply_base, struct linkinfo_reply_data, base);
A helper would be nice for this. For req_info too.
>+ struct net_device *dev = reply_base->dev; >+ int ret; >+ >+ data->lsettings = &data->ksettings.base; >+ >+ ret = ethnl_before_ops(dev);
"before_ops"/"after_ops" sounds odd. Maybe: ethnl_ops_begin ethnl_ops_complete
To me in-line with ethtool_ops names?
I guess you don't want the caller (ethnl_get_doit/ethnl_get_dumpit) to call this because it might not be needed down in prepare_data() callback, right?
>+ if (ret < 0) >+ return ret; >+ ret = __ethtool_get_link_ksettings(dev, &data->ksettings); >+ if (ret < 0 && info) >+ GENL_SET_ERR_MSG(info, "failed to retrieve link settings"); >+ ethnl_after_ops(dev); >+ >+ return ret; >+}
[...]
>+const struct get_request_ops linkinfo_request_ops = { >+ .request_cmd = ETHTOOL_MSG_LINKINFO_GET, >+ .reply_cmd = ETHTOOL_MSG_LINKINFO_GET_REPLY, >+ .hdr_attr = ETHTOOL_A_LINKINFO_HEADER, >+ .max_attr = ETHTOOL_A_LINKINFO_MAX, >+ .req_info_size = sizeof(struct linkinfo_req_info), >+ .reply_data_size = sizeof(struct linkinfo_reply_data), >+ .request_policy = linkinfo_get_policy, >+ .all_reqflags = ETHTOOL_RFLAG_LINKINFO_ALL, >+ >+ .prepare_data = linkinfo_prepare,
Please have the ops with the same name/suffix: .request_policy = linkinfo_reques_policy, .prepare_data = linkinfo_prepare_data, .reply_size = linkinfo_reply_size, .fill_reply = linkinfo_fill_reply,
Same applies of course to the other patches.
>+ .reply_size = linkinfo_size, >+ .fill_reply = linkinfo_fill, >+};
[...]
| |