Wed, Oct 09, 2019 at 10:59:37PM CEST, mkube...@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, >+}; [...]