Merge branch 'ethtool-netlink-next'
Jakub Kicinski says: ==================== ethtool: netlink: handle SET intro/outro in the common code Factor out the boilerplate code from SET handlers to common code. I volunteered to refactor the extack in GET in a conversation with Vladimir but I gave up. The handling of failures during dump in GET handlers is a bit unclear to me. Some code uses presence of info as indication of dump and tries to avoid reporting errors altogether (including extack messages). There's also the question of whether we should have a validation callback (similar to .set_validate here) for GET. It looks like .parse_request was expected to perform the validation. It takes the extack and tb directly, not via info: int (*parse_request)(struct ethnl_req_info *req_info, struct nlattr **tb, struct netlink_ext_ack *extack); int (*prepare_data)(const struct ethnl_req_info *req_info, struct ethnl_reply_data *reply_data, struct genl_info *info); so no crashes dereferencing info possible. But .parse_request doesn't run under rtnl nor ethnl_ops_begin(). As a result some implementations defer validation until .prepare_data where all the locks are held and they can call out to the driver. All this makes me think that maybe we should refactor GET in the same direction I'm refactoring SET. Split .prepare_data, take more locks in the core, and add a validation helper which would take extack directly: - ret = ops->prepare_data(req_info, reply_data, info); + ret = ops->prepare_data_validate(req_info, reply_data, attrs, extack); + if (ret < 1) // if 0 -> skip for dump; -EOPNOTSUPP in do + goto err1; + + ret = ethnl_ops_begin(dev); + if (ret) + goto err1; + + ret = ops->prepare_data(req_info, reply_data); // no extack + ethnl_ops_complete(dev); I'll file that away as a TODO for posterity / older me. v2: - invert checks for coalescing to avoid error code changes - rebase and convert MM as well v1: https://lore.kernel.org/all/20230121054430.642280-1-kuba@kernel.org/ ==================== Signed-off-by: David S. Miller <davem@davemloft.net>
Showing
Please register or sign in to comment