| Message ID | 20210306031550.26530-6-elder@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: qualcomm: rmnet: stop using C bit-fields | expand |
From: Alex Elder > Sent: 06 March 2021 03:16 > > Replace the use of C bit-fields in the rmnet_map_dl_csum_trailer > structure with a single one-byte field, using constant field masks > to encode or get at embedded values. > > Signed-off-by: Alex Elder <elder@linaro.org> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > .../ethernet/qualcomm/rmnet/rmnet_map_data.c | 2 +- > include/linux/if_rmnet.h | 17 +++++++---------- > 2 files changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c > b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c > index 3291f252d81b0..29d485b868a65 100644 > --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c > +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c > @@ -365,7 +365,7 @@ int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len) > > csum_trailer = (struct rmnet_map_dl_csum_trailer *)(skb->data + len); > > - 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)) { David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 3/8/21 4:13 AM, David Laight wrote: > From: Alex Elder >> Sent: 06 March 2021 03:16 >> >> Replace the use of C bit-fields in the rmnet_map_dl_csum_trailer >> structure with a single one-byte field, using constant field masks >> to encode or get at embedded values. >> >> Signed-off-by: Alex Elder <elder@linaro.org> >> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> >> --- >> .../ethernet/qualcomm/rmnet/rmnet_map_data.c | 2 +- >> include/linux/if_rmnet.h | 17 +++++++---------- >> 2 files changed, 8 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c >> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c >> index 3291f252d81b0..29d485b868a65 100644 >> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c >> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c >> @@ -365,7 +365,7 @@ int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len) >> >> csum_trailer = (struct rmnet_map_dl_csum_trailer *)(skb->data + len); >> >> - 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. Thanks a lot for your comment/question/suggestion. I like it. -Alex > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
... > >> - 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. 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. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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) >
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c index 3291f252d81b0..29d485b868a65 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c @@ -365,7 +365,7 @@ int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len) csum_trailer = (struct rmnet_map_dl_csum_trailer *)(skb->data + len); - if (!csum_trailer->valid) { + if (!u8_get_bits(csum_trailer->flags, MAP_CSUM_DL_VALID_FMASK)) { priv->stats.csum_valid_unset++; return -EINVAL; } diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h index 4824c6328a82c..1fbb7531238b6 100644 --- a/include/linux/if_rmnet.h +++ b/include/linux/if_rmnet.h @@ -19,21 +19,18 @@ struct rmnet_map_header { #define MAP_PAD_LEN_FMASK GENMASK(5, 0) struct rmnet_map_dl_csum_trailer { - u8 reserved1; -#if defined(__LITTLE_ENDIAN_BITFIELD) - u8 valid:1; - u8 reserved2:7; -#elif defined (__BIG_ENDIAN_BITFIELD) - u8 reserved2:7; - u8 valid:1; -#else -#error "Please fix <asm/byteorder.h>" -#endif + u8 reserved1; + u8 flags; /* MAP_CSUM_DL_*_FMASK */ __be16 csum_start_offset; __be16 csum_length; __be16 csum_value; } __aligned(1); +/* rmnet_map_dl_csum_trailer flags field: + * VALID: 1 = checksum and length valid; 0 = ignore them + */ +#define MAP_CSUM_DL_VALID_FMASK GENMASK(0, 0) + struct rmnet_map_ul_csum_header { __be16 csum_start_offset; #if defined(__LITTLE_ENDIAN_BITFIELD)