On Fri, 2016-03-11 at 09:58 -0800, David Decotigny wrote:
[...]
> +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.
> + 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
[...]
> @@ -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.
[...]
> @@ -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.
> + return NULL;
> +
> + memset(link_usettings, 0, sizeof(*link_usettings));
Could use calloc() instead of malloc() + memset().
> + /* 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)
> {
[...]
> + 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.
> + 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.
[...]
> +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.
[...]
> @@ -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);
[...]
> @@ -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.
signature.asc
Description: This is a digitally signed message part
