Re: [PATCH net-next v3 03/17] net: ethtool: add new ETHTOOL_GSETTINGS/SSETTINGS API

[David Decotigny]

Hello,

There is a set of conversion routines ulong[]<->u32[] to address this
32/64-bit compat issue. Using a u32-based bitmap would require drivers
to handle the u32 bitmaps themselves, this might be confusing,
considering there is a standard bitmap api; and might be error-prone
as well. Plus there is %*pb[l] format that's very helpful for
debugging. That's why I preferred to handle the relative complexity of
u32 bitmaps with the CPP conditionals in the non-driver code that
handles the user/kernel interactions, and drivers can use the standard
bitmap api transparently.

I was currently moving/rewriting the u32/ulong conversion code to
bitmap.{c,h} as Ben Hutchings was suggesting, which should hopefully
make the code more digestible; and could possibly be used for other
user/kernel interfaces. How about I send an updated version with this
solution, and if it's still not right, I'll revisit with either u32[]
everywhere or fixed-size bitmap instead of variable-size as here? Or
maybe another option would be to implement a new u32[]
bitmap_u32.{c,h} api, possibly using a set of macro tricks to share
code with bitmap.{c,h}?

On Tue, Dec 1, 2015 at 7:13 PM, David Miller <davem@xxxxxxxxxxxxx> wrote:
> From: David Decotigny <ddecotig@xxxxxxxxx>
> Date: Mon, 30 Nov 2015 14:05:41 -0800
>
>> This patch defines a new ETHTOOL_GSETTINGS/SSETTINGS API, handled by
>> the new get_ksettings/set_ksettings callbacks. This API provides
>> support for most legacy ethtool_cmd fields, adds support for larger
>> link mode masks (up to 4064 bits, variable length), and removes
>> ethtool_cmd deprecated fields (transceiver/maxrxpkt/maxtxpkt).
>
> Please do not define the mask using a non-fixed type.  I know it makes
> it easier to use the various bitmap helper routines if you use 'long',
> but here it is clearly superior to use "u32" for the bitmap type and
> do the bit operations by hand if necessary.
>
> Otherwise you have to have all of this ulong size CPP conditional code
> which is incredibly ugly.
>
> Furthermore you have to use fixed sized types anyways so that we don't
> need compat code to deal with 32-bit userspace applications making
> these ethtool calls into a 64-bit kernel.
>
> THanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/