Re: [ethtool PATCH v4 10/11] ethtool.c: add support for ETHTOOL_xLINKSETTINGS ioctls
flat view
From: Ben Hutchings <hidden>
Date: 2016-03-14 01:32:46
On Fri, 2016-03-11 at 09:58 -0800, David Decotigny wrote: [...]
quoted hunk ↗ jump to hunk
+static int parse_hex_u32_bitmap(const char *s, + unsigned int nbits, u32 *result) +{ + const unsigned nwords = __KERNEL_DIV_ROUND_UP(nbits, 32); + size_t slen = strlen(s); + size_t i; + + /* ignore optional '0x' prefix */ + if ((slen > 2) && ( + (0 == memcmp(s, "0x", 2) + || (0 == memcmp(s, "0X", 2))))) {
memcmp() is a really poor tool for comparing strings. You should use strncasecmp() here.
quoted hunk ↗ jump to hunk
+ slen -= 2; + s += 2; + } + + if (slen > 8*nwords) /* up to 2 digits per byte */
The '*' operator should have spaces around it. Please run checkpatch.pl over your ethtool patches to catch style errors like this (there are many others in this one). [...]
+static void dump_link_caps(const char *prefix, const char *an_prefix,
+ const u32 *mask, int link_mode_only)
{
static const struct {
int same_line; /* print on same line as previous */
- u32 value;
+ unsigned bit_indice;bit_index [...]
quoted hunk ↗ jump to hunk
@@ -558,51 +655,57 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,} } if (did1 == 0) - fprintf(stdout, "Not reported"); + fprintf(stdout, "Not reported");
Good catch, but whitespace fixes belong in another patch. [...]
quoted hunk ↗ jump to hunk
@@ -2248,10 +2360,219 @@ static int do_sfeatures(struct cmd_context *ctx)return 0; } -static int do_gset(struct cmd_context *ctx) +static struct ethtool_link_usettings * +__do_ioctl_glinksettings(struct cmd_context * ctx) +{ + int err; + struct { + struct ethtool_link_settings req; + __u32 link_mode_data[3*__ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; + } ecmd; + struct ethtool_link_usettings *link_usettings; + unsigned u32_offs; + + /* handshake with kernel to determine number of words for link + * mode bitmaps */ + memset(&ecmd, 0, sizeof(ecmd)); + ecmd.req.cmd = ETHTOOL_GLINKSETTINGS; + err = send_ioctl(ctx, &ecmd); + if (err < 0) + return NULL; + + if (ecmd.req.link_mode_masks_nwords >= 0 || ecmd.req.cmd) + return NULL;
Oops, I missed the kernel side of this. Clearing the cmd field is a very strange thing to do and inconsistent with every other ethtool command, so I've just sent a patch to change the kernel side of that. You'll need to change this to match. (I also think the negative size is a bit weird, but don't feel so strongly about it.) [...]
+ link_usettings = malloc(sizeof(*link_usettings)); + if (NULL == link_usettings)
Comparison is the wrong way round.
quoted hunk ↗ jump to hunk
+ return NULL; + + memset(link_usettings, 0, sizeof(*link_usettings));
Could use calloc() instead of malloc() + memset().
quoted hunk ↗ jump to hunk
+ /* keep transceiver 0 */ + memcpy(&link_usettings->base, &ecmd.req, sizeof(link_usettings->base)); + /* remember that ETHTOOL_GLINKSETTINGS was used */ + link_usettings->base.cmd = ETHTOOL_GLINKSETTINGS;
This is redundant as we know that ecmd.req.cmd == ETHTOOL_GLINKSETTINGS. [...]
+static struct ethtool_link_usettings *
+__do_ioctl_gset(struct cmd_context * ctx)
{[...]
quoted hunk ↗ jump to hunk
+ link_usettings->base.link_mode_masks_nwords = 1; + link_usettings->link_modes.supported[0] = ecmd.supported; + link_usettings->link_modes.advertising[0] = ecmd.advertising; + link_usettings->link_modes.lp_advertising[0] = ecmd.lp_advertising; + link_usettings->base.speed = ethtool_cmd_speed(&ecmd); + link_usettings->base.duplex = ecmd.duplex; + link_usettings->base.port = ecmd.port; + link_usettings->base.phy_address = ecmd.phy_address; + link_usettings->deprecated.transceiver = ecmd.transceiver; + link_usettings->base.autoneg = ecmd.autoneg; + link_usettings->base.mdio_support = ecmd.mdio_support; + /* ignored (fully deprecated): maxrxpkt, maxtxpkt */ + link_usettings->base.eth_tp_mdix = ecmd.eth_tp_mdix; + link_usettings->base.eth_tp_mdix_ctrl = ecmd.eth_tp_mdix_ctrl; + link_usettings->deprecated.transceiver = ecmd.transceiver;
Duplicate assignment.
quoted hunk ↗ jump to hunk
+ return link_usettings; +} + +static bool ethtool_link_mode_is_backward_compatible( + const u32 *mask)
Don't wrap function parameter lists or statements that are already under 80 characters. [...]
quoted hunk ↗ jump to hunk
+static void +__free_link_usettings(struct ethtool_link_usettings *link_usettings) { + free(link_usettings); +}
Is this function ever likely to do more than just free()? If not, don't bother abstracting that. [...]
quoted hunk ↗ jump to hunk
@@ -2469,75 +2799,88 @@ static int do_sset(struct cmd_context *ctx)} } - if (full_advertising_wanted < 0) { + if (NULL == full_advertising_wanted) { /* User didn't supply a full advertisement bitfield: * construct one from the specified speed and duplex. */ + int adv_bit = -1; + if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF) - advertising_wanted = ADVERTISED_10baseT_Half; + adv_bit = ETHTOOL_LINK_MODE_10baseT_Half_BIT; else if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_FULL) - advertising_wanted = ADVERTISED_10baseT_Full; + adv_bit = ETHTOOL_LINK_MODE_10baseT_Full_BIT; else if (speed_wanted == SPEED_100 && duplex_wanted == DUPLEX_HALF) - advertising_wanted = ADVERTISED_100baseT_Half; + adv_bit = ETHTOOL_LINK_MODE_100baseT_Half_BIT; else if (speed_wanted == SPEED_100 && duplex_wanted == DUPLEX_FULL) - advertising_wanted = ADVERTISED_100baseT_Full; + adv_bit = ETHTOOL_LINK_MODE_100baseT_Full_BIT; else if (speed_wanted == SPEED_1000 && duplex_wanted == DUPLEX_HALF) - advertising_wanted = ADVERTISED_1000baseT_Half; + adv_bit = ETHTOOL_LINK_MODE_1000baseT_Half_BIT; else if (speed_wanted == SPEED_1000 && duplex_wanted == DUPLEX_FULL) - advertising_wanted = ADVERTISED_1000baseT_Full; + adv_bit = ETHTOOL_LINK_MODE_1000baseT_Full_BIT; else if (speed_wanted == SPEED_2500 && duplex_wanted == DUPLEX_FULL) - advertising_wanted = ADVERTISED_2500baseX_Full; + adv_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT; else if (speed_wanted == SPEED_10000 && duplex_wanted == DUPLEX_FULL) - advertising_wanted = ADVERTISED_10000baseT_Full; - else - /* auto negotiate without forcing, - * all supported speed will be assigned below - */ - advertising_wanted = 0; + adv_bit = ETHTOOL_LINK_MODE_10000baseT_Full_BIT; + + if (adv_bit >= 0) { + advertising_wanted = _mask_advertising_wanted;
If I'm not mistaken, _mask_advertising_wanted is uninitialised at this point so you need to clear it before setting the one bit.
+ ethtool_link_mode_set_bit( + adv_bit, advertising_wanted);
[...]
quoted hunk ↗ jump to hunk
@@ -2549,37 +2892,56 @@ static int do_sset(struct cmd_context *ctx)fprintf(stderr, "\n"); } if (autoneg_wanted == AUTONEG_ENABLE && - advertising_wanted == 0) { + NULL == advertising_wanted) { + unsigned int i; + /* Auto negotiation enabled, but with * unspecified speed and duplex: enable all * supported speeds and duplexes. */ - ecmd.advertising = - (ecmd.advertising & - ~ALL_ADVERTISED_MODES) | - (ALL_ADVERTISED_MODES & ecmd.supported); + ethtool_link_mode_for_each_u32(i) + {
Brace belongs on the previous line (everywhere you use ethtool_link_mode_for_each_u32()). [...]
- } else if (advertising_wanted > 0) {
+ } else if (NULL != advertising_wanted) {
+ unsigned int i;
+
/* Enable all requested modes */
- ecmd.advertising =
- (ecmd.advertising &
- ~ALL_ADVERTISED_MODES) |
- advertising_wanted;
- } else if (full_advertising_wanted > 0) {
- ecmd.advertising = full_advertising_wanted;
+ ethtool_link_mode_for_each_u32(i)
+ {
+ u32 *adv = link_usettings->link_modes.advertising + i;
+ *adv = ((*adv & all_advertised_modes[i])[...] Should be ~all_advertised_modes[i] rather than all_advertised_modes[i]. Ben. -- Ben Hutchings If at first you don't succeed, you're doing about average.
Attachments
- signature.asc [application/pgp-signature] 819 bytes