Re: [PATCH net-next v1 1/7] net: ethtool: extend link mode support to 48 bits
From: Ben Hutchings <hidden>
Date: 2015-01-04 22:01:22
Also in:
lkml, netdev
On Sun, 2015-01-04 at 12:56 -0800, David Decotigny wrote:
From: David Decotigny <decot-Ypc/8FJVVoBWk0Htik3J/w@public.gmane.org>
This is mostly fine, with just a few minor issues.
quoted hunk ↗ jump to hunk
Signed-off-by: David Decotigny <decot-Ypc/8FJVVoBWk0Htik3J/w@public.gmane.org> --- include/uapi/linux/ethtool.h | 130 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 110 insertions(+), 20 deletions(-)diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 5f66d9c..61e7734 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h
[...]
quoted hunk ↗ jump to hunk
@@ -123,6 +140,46 @@ static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep) return (ep->speed_hi << 16) | ep->speed; } +/** + * MAKE_ETHTOOL_LINK_MODE_ACCESSORS - create the link_mode accessors + * Macro to generate the %ethtool_cmd_supported_*, + * %ethtool_cmd_advertising_*, %ethtool_cmd_lp_advertising_*, + * %ethtool_eee_supported_*, %ethtool_eee_advertised_*, + * %ethtool_eee_lp_advertised_* families of functions. + * + * Macro args:
Delete the 'Macro args:' heading; that's implied by the @ prefixes.
+ * @struct_name: either %ethtool_cmd or %ethtool_eee + * @field_name: name of the fields in %struct_name to + * access. supported/advertising/lp_advertising for ethtool_cmd, + * supported/advertised/lp_advertised for ethtool_eee + * + * Generates the following static functions: + * @ethtool_cmd_supported(const struct ethtool_cmd*): returns + * the 64b value of %supported fields (the upper bits 63..48 are 0) + * @ethtool_cmd_supported_set(struct ethtool_cmd*, + * ethtool_link_mode_mask_t value): set the %supported fields to + * given %value (only the lower 48 bits used, upper bits 63..48 + * ignored)
Delete the @ prefixes from these headings.
+ * Same doc for all the other functions. + */ +#define MAKE_ETHTOOL_LINK_MODE_ACCESSORS(struct_name, field_name) \
I think the name should begin with ETHTOOL but it's not a big deal. [...]
quoted hunk ↗ jump to hunk
@@ -1192,6 +1273,9 @@ enum ethtool_sfeatures_retval_bits { #define SPARC_ETH_GSET ETHTOOL_GSET #define SPARC_ETH_SSET ETHTOOL_SSET +/* Do not use the following macros directly to update + * ethtool_cmd::supported, ethtool_eee::supported. Please use + * ethtool_(cmd|eee)_supported(|_set) instead */
The ending */ belongs on a new line.
quoted hunk ↗ jump to hunk
#define SUPPORTED_10baseT_Half (1 << 0) #define SUPPORTED_10baseT_Full (1 << 1) #define SUPPORTED_100baseT_Half (1 << 2)@@ -1223,7 +1307,12 @@ enum ethtool_sfeatures_retval_bits { #define SUPPORTED_56000baseCR4_Full (1 << 28) #define SUPPORTED_56000baseSR4_Full (1 << 29) #define SUPPORTED_56000baseLR4_Full (1 << 30) +/* TODO: for bit indices >= 31, make sure to shift 1ULL instead of 1 */
[...] I don't think that comment is necessary; the compiler will surely warn if someone forgets. Ben. -- Ben Hutchings This sentence contradicts itself - no actually it doesn't.
Attachments
- signature.asc [application/pgp-signature] 811 bytes