Hi, > + [ETHTOOL_ATTR_FLAGS] = { .type = NLA_U32 },
I suppose this comes from the current API, but perhaps it'd be worthwhile to make provision for more flags? Perhaps even using NLA_BINARY and have "infinitely extensible" flags. > + [ETHTOOL_ATTR_SSET_COUNT] = { .type = NLA_U32 What do you need that for? Wouldn't it be sufficient to count the SSET values returned? I can see how this would be useful for ioctl, but not really for netlink messages? > +static struct genl_ops ethtool_ops[] = { > + { > + .cmd = ETHTOOL_CMD_GET_SETTINGS, > + .policy = ethtool_policy, > + .doit = ethtool_get_settings, > + }, [...] > + { > + .cmd = ETHTOOL_CMD_SET_MODULE_INFO, > + .policy = ethtool_policy, > + .doit = ethtool_set_module_info, > + }, > +}; Shouldn't the ops have GENL_ADMIN_PERM as flags? > +int ethtool_get_settings(struct net_device *dev, struct ethtool_cmd > *cmd) > +{ > + return 0; > +} > +EXPORT_SYMBOL_GPL(ethtool_get_settings); I don't understand what kind of placeholder this was meant to be - but why would it be exported? This part is called by the genl ops, so doesn't really make sense? It seems that instead these functions should be static, declared above the ops, and call into the existing ethtool driver ops, based on IFINDEX demultiplexing. > +void ethtool_get_ethtool_stats(struct net_device *dev, > + struct ethtool_stats *stats, > + u64 *data) > +{ > + > + /* example the driver handler would do the below > + * > + err = nla_put_u32(msg, PORT_ATTR_IFINDEX, ifindex); > + if (err < 0) > + goto err_out; > + > + err = nla_put_u32(msg, PORT_ATTR_FLAGS, flags); > + if (err < 0) > + goto err_out; > + > + err = nla_put_u32(msg, PORT_ATTR_SSET_COUNT, > + count); > + if (err < 0) > + goto err_out; > + > + nest = nla_nest_start(msg, PORT_ATTR_STATS); > + for (i = 0; i < count; i++) > + nla_put_u64(msg, PORT_ATTR_STAT, data[i]); > + nla_nest_end(msg, nest); > + > + */ > +} > +EXPORT_SYMBOL_GPL(ethtool_get_ethtool_stats); It seems possible that you could have a lot of ports, or a lot of strings, or similar, so I think this should be a dumpit instead of a doit handler. Similar, perhaps, for the EEPROM thing, unless you provide API to query the size first so the application can size it's recvmsg() buffer appropriately - however doing so also requires a big message allocation and more code in userspace, so I think having an "offset/length" type of API combined with a dumpit rather than doit would be good for all of the things that could get bigger or that might be extended in the future. johannes