On Mon, Jul 27, 2020 at 02:51:28PM -0700, Jacob Keller wrote:
>
>
> On 7/27/2020 2:47 PM, Jacob Keller wrote:
> > The ethtool netlink API can send bitsets without an associated bitmask.
> > These do not get displayed properly, because the dump_link_modes, and
> > bitset_get_bit to not check whether the provided bitset is a NOMASK
> > bitset. This results in the inability to display peer advertised link
> > modes.
> >
> > The dump_link_modes and bitset_get_bit functions are designed so they
> > can print either the values or the mask. For a nomask bitmap, this
> > doesn't make sense. There is no mask.
> >
> > Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
> > bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
> > regardless of the request to display the mask or the value. For full
> > size bitmaps, the set of provided bits indicates the valid values,
> > without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
> > bits without this attribute if nomask is set. This essentially means
> > that dump_link_modes will treat a NOMASK bitset as having a mask
> > equivalent to all of its set bits.
> >
> > For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
> > bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
> > For full bitmaps, if nomask is set, then always return true of the bit
> > is in the set, rather than only if it provides an
> > ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
> > bits.
> >
> > This fixes display of link partner advertised fields when using the
> > netlink API.
> >
> > Reported-by: Jamie Gloudon <jamie.glou...@gmx.fr>
> > Signed-off-by: Jacob Keller <jacob.e.kel...@intel.com>
>
> Andrew, could you kindly test this in your setup? I believe it will
> fully resolve the issue.
>
> Michal, I think this is complete based on the docs in
> ethtool-netlink.rst and some tests. Any further insight?
>
> Thanks,
> Jake
>
> > ---
> > netlink/bitset.c | 9 ++++++---
> > netlink/settings.c | 8 +++++---
> > 2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/netlink/bitset.c b/netlink/bitset.c
> > index 130bcdb5b52c..ba5d3ea77ff7 100644
> > --- a/netlink/bitset.c
> > +++ b/netlink/bitset.c
> > @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool
> > mask, unsigned int idx,
> > DECLARE_ATTR_TB_INFO(bitset_tb);
> > const struct nlattr *bits;
> > const struct nlattr *bit;
> > + bool nomask;
> > int ret;
> >
> > *retptr = 0;
> > @@ -57,8 +58,10 @@ bool bitset_get_bit(const struct nlattr *bitset, bool
> > mask, unsigned int idx,
> > if (ret < 0)
> > goto err;
> >
> > - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> > - bitset_tb[ETHTOOL_A_BITSET_VALUE];
> > + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> > +
> > + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> > + bitset_tb[ETHTOOL_A_BITSET_VALUE];
> > if (bits) {
> > const uint32_t *bitmap =
> > (const uint32_t *)mnl_attr_get_payload(bits);
> > @@ -87,7 +90,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool
> > mask, unsigned int idx,
> >
> > my_idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
> > if (my_idx == idx)
> > - return mask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
> > + return mask || nomask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
> > }
> >
> > return false;
> > diff --git a/netlink/settings.c b/netlink/settings.c
> > index 35ba2f5dd6d5..29557653336e 100644
> > --- a/netlink/settings.c
> > +++ b/netlink/settings.c
> > @@ -280,9 +280,11 @@ int dump_link_modes(struct nl_context *nlctx, const
> > struct nlattr *bitset,
> > const struct nlattr *bit;
> > bool first = true;
> > int prev = -2;
> > + bool nomask;
> > int ret;
> >
> > ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info);
> > + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> > bits = bitset_tb[ETHTOOL_A_BITSET_BITS];
> > if (ret < 0)
> > goto err_nonl;
> > @@ -297,8 +299,8 @@ int dump_link_modes(struct nl_context *nlctx, const
> > struct nlattr *bitset,
> > goto err_nonl;
> > lm_strings = global_stringset(ETH_SS_LINK_MODES,
> > nlctx->ethnl2_socket);
> > - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> > - bitset_tb[ETHTOOL_A_BITSET_VALUE];
> > + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> > + bitset_tb[ETHTOOL_A_BITSET_VALUE];
> > ret = -EFAULT;
> > if (!bits || !bitset_tb[ETHTOOL_A_BITSET_SIZE])
> > goto err_nonl;
> > @@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const
> > struct nlattr *bitset,
> > if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] ||
> > !tb[ETHTOOL_A_BITSET_BIT_NAME])
> > goto err;
> > - if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
> > + if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
> > continue;
> >
> > idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
> >
I can confirm that your patch works. As you can see below:
Link partner advertised pause frame use: Symmetric Receive-only
Link partner advertised auto-negotiation: Yes
Link partner advertised FEC modes: No
You can tag this patch Tested-by me. Thanks!
Regards,
Jamie Gloudon
