Re: [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer

[Alex Elder <elder@linaro.org>]

On 3/8/21 7:53 AM, David Laight wrote:
> ...
>>>> -	if (!csum_trailer->valid) {
>>>> +	if (!u8_get_bits(csum_trailer->flags, MAP_CSUM_DL_VALID_FMASK)) {
>>>
>>> Is that just an overcomplicated way of saying:
>>> 	if (!(csum_trailer->flags & MAP_CSUM_DL_VALID_FMASK)) {
>>
>> Yes it is.  I defined and used all the field masks in a
>> consistent way, but I do think it will read better the
>> way you suggest.  Bjorn also asked me privately whether
>> GENMASK(15, 15) was just the same as BIT(15) (it is).
>>
>> I will post version 3 of the series, identifying which
>> fields are single bit/Boolean.  For those I will define
>> the value using BIT() and will set/extract using simple
>> AND/OR operators.  I won't use the _FMASK suffix on such
>> fields.
> 
> Even for the checksum offset a simple 'offset << CONSTANT'
> is enough.

I do not want the code to assume the field resides in the
bottom of the register.

> If it is the bottom bits then even that isn't really needed.
> You might want to mask off high bits - but that is an error
> path that needs to have been checked earlier.

Using u32_get_bits() (etc.) is a general-purpose way of
setting and extracting bit fields from registers.  In a
way, it might be overkill here.  On the other hand, we
can expect additional structures to be defined in
<linux/if_rmnet.h>.  If we handle fields in this common
way now, they can be handled the same way for new
structures.

I think your implication is right, that this could be
done possibly more simply without the helper functions.

Let me do the BIT() fix on version 3.  If others echo
your thoughts about not using the u32_get_bits() family
of helper functions, I can address that in version 4.

					-Alex


> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>