| Message ID | 20210304223431.15045-1-elder@linaro.org |
|---|---|
| Headers | show |
| Series | net: qualcomm: rmnet: stop using C bit-fields | expand |
On 3/4/21 4:34 PM, Alex Elder wrote: > This series converts data structures defined in <linux/if_rmnet.h> > so they use integral field values with bitfield masks rather than > rely on C bit-fields. Whoops! I forgot to check if net-next was open. I'm very sorry about that... http://vger.kernel.org/~davem/net-next.html -Alex > I first proposed doing something like this long ago when my confusion > about this code (and the memory layout it was supposed to represent) > led me to believe it was erroneous: > https://lore.kernel.org/netdev/20190520135354.18628-1-elder@linaro.org/ > > It came up again recently, when Sharath Chandra Vurukala proposed > a new structure in "if_rmnet.h", again using C bit-fields. I asked > whether the new structure could use field masks, and Jakub requested > that this be done. > https://lore.kernel.org/netdev/1613079324-20166-1-git-send-email-sharathv@codeaurora.org/ > I volunteered to convert the existing RMNet code to use bitfield > masks, and that is what I'm doing here. > > The first three patches are more or less preparation work for the > last three. > - The first marks two fields in an existing structure explicitly > big endian. They are unused by current code, so this should > have no impact. > - The second simplifies some code that computes the value of a > field in a header in a somewhat obfuscated way. > - The third eliminates some trivial accessor macros, open-coding > them instead. I believe the accessors actually do more harm > than good. > - The last three convert the structures defined in "if_rmnet.h" > so they are defined only with integral fields, each having > well-defined byte order. Where sub-fields are needed, field > masks are defined so they can be encoded or extracted using > functions like be16_get_bits() or u8_encode_bits(), defined > in <linux/bitfield.h>. The three structures converted are, > in order: rmnet_map_header, rmnet_map_dl_csum_trailer, and > rmnet_map_ul_csum_header. > > -Alex > > Alex Elder (6): > net: qualcomm: rmnet: mark trailer field endianness > net: qualcomm: rmnet: simplify some byte order logic > net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros > net: qualcomm: rmnet: use field masks instead of C bit-fields > net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer > net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header > > .../ethernet/qualcomm/rmnet/rmnet_handlers.c | 11 ++-- > .../net/ethernet/qualcomm/rmnet/rmnet_map.h | 12 ---- > .../qualcomm/rmnet/rmnet_map_command.c | 11 +++- > .../ethernet/qualcomm/rmnet/rmnet_map_data.c | 60 ++++++++--------- > include/linux/if_rmnet.h | 65 +++++++++---------- > 5 files changed, 70 insertions(+), 89 deletions(-) >
On 2021-03-04 15:34, Alex Elder wrote: > This series converts data structures defined in <linux/if_rmnet.h> > so they use integral field values with bitfield masks rather than > rely on C bit-fields. > > I first proposed doing something like this long ago when my confusion > about this code (and the memory layout it was supposed to represent) > led me to believe it was erroneous: > > https://lore.kernel.org/netdev/20190520135354.18628-1-elder@linaro.org/ > > It came up again recently, when Sharath Chandra Vurukala proposed > a new structure in "if_rmnet.h", again using C bit-fields. I asked > whether the new structure could use field masks, and Jakub requested > that this be done. > > https://lore.kernel.org/netdev/1613079324-20166-1-git-send-email-sharathv@ > codeaurora.org/ > I volunteered to convert the existing RMNet code to use bitfield > masks, and that is what I'm doing here. > > The first three patches are more or less preparation work for the > last three. > - The first marks two fields in an existing structure explicitly > big endian. They are unused by current code, so this should > have no impact. > - The second simplifies some code that computes the value of a > field in a header in a somewhat obfuscated way. > - The third eliminates some trivial accessor macros, open-coding > them instead. I believe the accessors actually do more harm > than good. > - The last three convert the structures defined in "if_rmnet.h" > so they are defined only with integral fields, each having > well-defined byte order. Where sub-fields are needed, field > masks are defined so they can be encoded or extracted using > functions like be16_get_bits() or u8_encode_bits(), defined > in <linux/bitfield.h>. The three structures converted are, > in order: rmnet_map_header, rmnet_map_dl_csum_trailer, and > rmnet_map_ul_csum_header. > > -Alex > > Alex Elder (6): > net: qualcomm: rmnet: mark trailer field endianness > net: qualcomm: rmnet: simplify some byte order logic > net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros > net: qualcomm: rmnet: use field masks instead of C bit-fields > net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum > trailer > net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header > > .../ethernet/qualcomm/rmnet/rmnet_handlers.c | 11 ++-- > .../net/ethernet/qualcomm/rmnet/rmnet_map.h | 12 ---- > .../qualcomm/rmnet/rmnet_map_command.c | 11 +++- > .../ethernet/qualcomm/rmnet/rmnet_map_data.c | 60 ++++++++--------- > include/linux/if_rmnet.h | 65 +++++++++---------- > 5 files changed, 70 insertions(+), 89 deletions(-) Can you share what all tests have been done with these patches
On 3/4/21 9:44 PM, subashab@codeaurora.org wrote: > > Can you share what all tests have been done with these patches I'm testing with all of them applied and "it works." On the first three I think they're simple enough that you can see by inspection they should be OK. For the rest I tested more carefully. For runtime testing, I have used them on IPA v3.5.1 and IPA v4.2 platforms, running repeated ping and other network traffic tests over an rmnet connection. For unit testing, I did essentially the following. I'll use the MAP header structure as an example, but I did this on all structures I modified. struct rmnet_map_header_new new; struct rmnet_map_header *old = (void *)&new; u8 val; val = u8_encode_bits(1, MAP_CMD_FMASK); val |= u8_encode_bits(0x23, MAP_PAD_LEN_FMASK); new.flags = val; new.mux_id = 0x45; new.pkt_len = htons(0x6789); printk("pad_len: 0x%x (want 0x23)\n", old->pad_len); printk("reserved_bit: 0x%x (want 0x0)\n", old->reserved_bit); printk("cd_bit: 0x%x (want 0x1)\n", old->cd_bit); printk("mux_id: 0x%x (want 0x45)\n", old->mux_id); printk("pkt_len: 0x%x (want 0x6789)\n", ntohs(old->pkt_len)); I didn't do *exactly* or *only* this, but basically the process was manipulating the values assigned using the old structure then verifying it has the same representation in the new structure using the new access methods (and vice versa). I suspect you have a much better ability to test than I do, and I would really prefer to see this get tested rigorously if possible. -Alex
This series converts data structures defined in <linux/if_rmnet.h> so they use integral field values with bitfield masks rather than rely on C bit-fields. I first proposed doing something like this long ago when my confusion about this code (and the memory layout it was supposed to represent) led me to believe it was erroneous: https://lore.kernel.org/netdev/20190520135354.18628-1-elder@linaro.org/ It came up again recently, when Sharath Chandra Vurukala proposed a new structure in "if_rmnet.h", again using C bit-fields. I asked whether the new structure could use field masks, and Jakub requested that this be done. https://lore.kernel.org/netdev/1613079324-20166-1-git-send-email-sharathv@codeaurora.org/ I volunteered to convert the existing RMNet code to use bitfield masks, and that is what I'm doing here. The first three patches are more or less preparation work for the last three. - The first marks two fields in an existing structure explicitly big endian. They are unused by current code, so this should have no impact. - The second simplifies some code that computes the value of a field in a header in a somewhat obfuscated way. - The third eliminates some trivial accessor macros, open-coding them instead. I believe the accessors actually do more harm than good. - The last three convert the structures defined in "if_rmnet.h" so they are defined only with integral fields, each having well-defined byte order. Where sub-fields are needed, field masks are defined so they can be encoded or extracted using functions like be16_get_bits() or u8_encode_bits(), defined in <linux/bitfield.h>. The three structures converted are, in order: rmnet_map_header, rmnet_map_dl_csum_trailer, and rmnet_map_ul_csum_header. -Alex Alex Elder (6): net: qualcomm: rmnet: mark trailer field endianness net: qualcomm: rmnet: simplify some byte order logic net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros net: qualcomm: rmnet: use field masks instead of C bit-fields net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header .../ethernet/qualcomm/rmnet/rmnet_handlers.c | 11 ++-- .../net/ethernet/qualcomm/rmnet/rmnet_map.h | 12 ---- .../qualcomm/rmnet/rmnet_map_command.c | 11 +++- .../ethernet/qualcomm/rmnet/rmnet_map_data.c | 60 ++++++++--------- include/linux/if_rmnet.h | 65 +++++++++---------- 5 files changed, 70 insertions(+), 89 deletions(-) -- 2.20.1