On 7/27/2020 3:00 PM, Michal Kubecek wrote:
> On Mon, Jul 27, 2020 at 02:27:56PM -0700, Jacob Keller wrote:
>>
>>
>> On 7/27/2020 2:08 PM, Michal Kubecek wrote:
>>> On Mon, Jul 27, 2020 at 11:01:41PM +0200, Andrew Lunn wrote:
>>>>> - the exact command you ran (including arguments)
>>>>> - expected output (or at least the relevant part)
>>>>> - actual output (or at least the relevant part)
>>>>> - output with dump of netlink messages, you can get it by enabling
>>>>> debugging flags, e.g. "ethtool --debug 0x12 eth0"
>>>>
>>>> Hi Michal
>>>>
>>>> See if this helps.
>>>>
>>>> This is a Marvel Ethernet switch port using an Marvell PHY.
>>>
>>> Thank you. I think I can see the problem. Can you try the patch below?
>>>
>>> Michal
>>>
>>
>> I think the patch below fixes part of the issue, but isn't completely
>> correct, because NOMASK bitmaps can be sent in compact form as well.
>
> I believe this part is correct; compact NOMASK bitmap would have no
> ETHTOOL_A_BITSET_MASK and ETHTOOL_A_BITSET_BIT_VALUE would contain the
> bits in it so that the code would generate correct output.
>
>> Also, we'll need something to check the NOMASK flag and do the correct
>> thing in all of the dump functions.
>
> You are right, bitset_get_bit needs the same fix, updated version is
> below.
>
> Michal
>
>
This is basically what I got except that I also applied the fix to
compact bitmaps.
See https://lore.kernel.org/netdev/20200727221104.gd1705...@lunn.ch/T/#t
I think we're still missing something tho.
Thanks,
Jake
> diff --git a/netlink/bitset.c b/netlink/bitset.c
> index 130bcdb5b52c..67b45778692c 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;
> @@ -68,6 +69,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask,
> unsigned int idx,
> return bitmap[idx / 32] & (1U << (idx % 32));
> }
>
> + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> bits = bitset_tb[ETHTOOL_A_BITSET_BITS];
> if (!bits)
> goto err;
> @@ -87,7 +89,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..60523ad6edf5 100644
> --- a/netlink/settings.c
> +++ b/netlink/settings.c
> @@ -280,6 +280,7 @@ 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);
> @@ -338,6 +339,7 @@ int dump_link_modes(struct nl_context *nlctx, const
> struct nlattr *bitset,
> goto after;
> }
>
> + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> printf("\t%s", before);
> mnl_attr_for_each_nested(bit, bits) {
> const struct nlattr *tb[ETHTOOL_A_BITSET_BIT_MAX + 1] = {};
> @@ -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]);
>
