Please note that v5 patch depends on Ben Hutchings'
http://patchwork.ozlabs.org/patch/596879/ to make any sense.
On Tue, Mar 15, 2016 at 4:42 PM, David Decotigny <ddeco...@gmail.com> wrote:
> Just sent v5 of the series: now only this patch left. Applied all your
> suggestions.
>
> Many thanks for the 2 bugs you caught, path was not covered by my
> tests. v5 was tested on a 10G nic with: ethtool -s eth1 msglvl 0x15
> speed 10000 duplex full
>
> On Sun, Mar 13, 2016 at 6:32 PM, Ben Hutchings <b...@decadent.org.uk> wrote:
>> 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.
>
> done.
>
>>
>>> + 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).
>
> both done. a few new lines over 80 chars that I prefer to keep,
> otherwise line break would make code harder to read imho.
>
>>
>> [...]
>>> +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
>
> done.
>
>>
>> [...]
>>> @@ -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.
>
> removed for now.
>
>>
>> [...]
>>
>>> @@ -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.)
>
> updated based on your patch. sent a suggestion
> http://marc.info/?l=linux-netdev&m=145806871332416 , looks like it was
> not added properly to the thread, because I don't see it as a reply to
> your patch in patchwork. sorry about this.
>
>>
>> [...]
>>> + link_usettings = malloc(sizeof(*link_usettings));
>>> + if (NULL == link_usettings)
>>
>> Comparison is the wrong way round.
>
> done.
>
>>
>>> + return NULL;
>>> +
>>> + memset(link_usettings, 0, sizeof(*link_usettings));
>>
>> Could use calloc() instead of malloc() + memset().
>
> done.
>
>>
>>> + /* 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.
>
> done.
>
>>
>> [...]
>>> +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.
>
> done.
>
>>
>>> + 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.
>
> done.
>
>>
>> [...]
>>> +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.
>
> free() is likely to stay good enough. updated.
>
>>
>> [...]
>>> @@ -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.
>
> great catch! thanks.
>
>>
>>> + 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()).
>
> done.
>
>>
>> [...]
>>> - } 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].
>
> aouch! thanks.
>
>>
>> Ben.
>>
>> --
>> Ben Hutchings
>> If at first you don't succeed, you're doing about average.
