lkml.org 
[lkml]   [2019]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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,
    >+};

    [...]

    \
     
     \ /
      Last update: 2019-10-10 18:01    [W:4.382 / U:0.332 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site