On Thu, Oct 10, 2019 at 05:59:55PM +0200, Jiri Pirko wrote: > Wed, Oct 09, 2019 at 10:59:37PM CEST, mkube...@suse.cz wrote: > > [...] > > >+/* prepare_data() handler */ > > Not sure how valuable are comments like this...
I'll drop them. > >+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. Good point. > >+ 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? OK > 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? Yes, there are some which do not call any ethtool_ops callbacks, e.g. netdev features (ethtool -k / -K). > >+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. OK Michal