Message-Id: <20210306031550.26530-1-elder@linaro.org>
Date:   Fri,  5 Mar 2021 21:15:44 -0600
From:   Alex Elder <elder@...aro.org>
To:     subashab@...eaurora.org, stranche@...eaurora.org,
        davem@...emloft.net, kuba@...nel.org
Cc:     sharathv@...eaurora.org, bjorn.andersson@...aro.org,
        evgreen@...omium.org, cpratapa@...eaurora.org, elder@...nel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH net-next v2 0/6] net: qualcomm: rmnet: stop using C bit-fields

Version 2 of this series fixes the code in the final patch that
encoded the RMNet checksum offload header inserted for outgoing
packets.  It was erroneously using be16_encode_bits() to generate
the value to be written into the header, where it should have been
using u16_encode_bits().  Bjorn noticed this, as did the Intel
kernel test robot.

This version of these patches has been tested as follows:
  - ICMP (ping) and TCP (wget), and UDP (iperf) packet transfer in
    both directions over an RMNet link (using IPA, with IPv4 only).
    This used QMAPv4, with checksum offload in both directions
    enabled (and aggregation enabled for inbound data).  Inbound
    checksum values were verified good (for TCP and UDP).  I
    presume the TX checksum was acceptable but did nothing special
    to confirm that.  Checksum verification was enabled with:
      ethtool -K rmnet_data0 tx on rx on
  - Format of the new and old structures were compared, bit by bit,
    after assigning various values to the fields using both the
    old and new structure definitions (and access methods).
  - The patches were all run through sparse.  No new errors are
    reported.  (A "no newline at end of file" warning is reported
    for "rmnet_vnd.c"; and ip_fast_csum() defined in ARM64
    "checksum.h" produces a warning.)

I've added Reviewed-by from Bjorn to the first five patches, and
Reported-by from the kernel test robot on the last one

Below is description that was sent with version 1.

					-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.27.0

Powered by blists - more mailing lists