On Thu, Jul 04, 2019 at 02:21:52PM +0200, Johannes Berg wrote:

On Thu, 2019-07-04 at 14:17 +0200, Michal Kubecek wrote:

quoted

On Thu, Jul 04, 2019 at 02:03:02PM +0200, Johannes Berg wrote:

quoted

On Thu, 2019-07-04 at 13:52 +0200, Michal Kubecek wrote:

quoted

There is still the question if it it should be implemented as a nested
attribute which could look like the current compact form without the
"list" flag (if there is no mask, it's a list). Or an unstructured data
block consisting of u32 bit length 

You wouldn't really need the length, since the attribute has a length
already :-)

It has byte length, not bit length. The bitmaps we are dealing with
can have any bit length, not necessarily multiples of 8 (or even 32).

Not sure why that matters? You have the mask, so you don't really need
to additionally say that you're only going up to a certain bit?

I mean, say you want to set some bits <=17, why would you need to say
that they're <=17 if you have a
 value: 0b00000000'000000xx'xxxxxxxx'xxxxxxxx
 mask:  0b00000000'00000011'11111111'11111111

One scenario that I can see from the top of my head would be user
running

  ethtool -s <dev> advertise 0x...

with hex value representing some subset of link modes. Now if ethtool
version is behind kernel and recognizes fewer link modes than kernel
but in a way that the number rounded up to bytes or words would be the
same, kernel has no way to recognize of those zero bits on top of the
mask are zero on purpose or just because userspace doesn't know about
them. In general, I believe the absence of bit length information is
something protocols would have to work around sometimes.

The submitted implementation doesn't have this problem as it can tell
kernel "this is a list" (i.e. I'm not sending a value/mask pair, I want
exactly these bits to be set). Thus it can easily implement requests of
both types (value/mask or just value):

  ethtool -s <dev> advertise 0x2f
  ethtool -s <dev> advertise 0x08/0x0c
  ethtool -s <dev> advertise 100baseT/Full off 1000baseT/Full on

and could be as easily extended to support also

  ethtool -s <dev> advertise 100baseT/Full 1000baseT/Full

Michal