[0/8] net: introduce "include/linux/if_rmnet.h"

Message ID 20190520135354.18628-1-elder@linaro.org
Headers show
Series
  • net: introduce "include/linux/if_rmnet.h"
Related show

Message

Alex Elder May 20, 2019, 1:53 p.m.
The main objective of this series was originally to define a single
public header file containing a few structure definitions that are
currently defined privately for the Qualcomm "rmnet" driver.  In
review, Arnd Bergmann said that before making them public, the
structures should avoid using C bit-fields in their definitions.

To facilitate implementing that suggestion I rearranged some other
code, including eliminating some accessor macros that I believe
reduce rather than improve the clarity of the code that uses them.

I also discovered a bug (concievably due to non-portable behavior)
in the way one of the structures is defined, so I fixed that.  And
finally I ensured all of the fields in these structures were defined
with proper annotation of their big endianness.

A form of the code in this series was present in this patch:
  https://lore.kernel.org/lkml/20190512012508.10608-3-elder@linaro.org/
This series is available here, based on kernel v5.2-rc1:
  remote: https://git.linaro.org/people/elder/linux.git
  branch: ipa-rmnet-v1_kernel-5.2-rc1
    acbcb18302a net: introduce "include/linux/if_rmnet.h"

					-Alex

Alex Elder (8):
  net: qualcomm: rmnet: fix struct rmnet_map_header
  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 header
  net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer
  net: qualcomm: rmnet: get rid of a variable in
    rmnet_map_ipv4_ul_csum_header()
  net: qualcomm: rmnet: mark endianness of struct
    rmnet_map_dl_csum_trailer fields
  net: introduce "include/linux/if_rmnet.h"

 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  | 11 ++--
 .../net/ethernet/qualcomm/rmnet/rmnet_map.h   | 36 ----------
 .../qualcomm/rmnet/rmnet_map_command.c        | 12 +++-
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 65 +++++++++----------
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   |  1 +
 include/linux/if_rmnet.h                      | 45 +++++++++++++
 6 files changed, 91 insertions(+), 79 deletions(-)
 create mode 100644 include/linux/if_rmnet.h

-- 
2.20.1

Comments

Bjorn Andersson May 20, 2019, 3:49 p.m. | #1
On Mon 20 May 06:53 PDT 2019, Alex Elder wrote:

> Replace the use of C bit-fields in the rmnet_map_ul_csum_header

> structure with a single integral structure member, and use field

> masks to encode or get values within that member.

> 

> Note that the previous C bit-fields were defined with CPU local

> endianness.  Their values were computed and then forecfully converted

> to network byte order in rmnet_map_ipv4_ul_csum_header().  Simplify

> that function, and properly define the new csum_info member as a big

> endian value.

> 

> Make similar simplifications in rmnet_map_ipv6_ul_csum_header().

> 


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


> Signed-off-by: Alex Elder <elder@linaro.org>

> ---

>  .../net/ethernet/qualcomm/rmnet/rmnet_map.h   |  9 ++--

>  .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 50 ++++++++-----------

>  2 files changed, 26 insertions(+), 33 deletions(-)

> 

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h

> index a56209645c81..f3231c26badd 100644

> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h

> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h

> @@ -60,11 +60,14 @@ struct rmnet_map_dl_csum_trailer {

>  

>  struct rmnet_map_ul_csum_header {

>  	__be16 csum_start_offset;

> -	u16 csum_insert_offset:14;

> -	u16 udp_ip4_ind:1;

> -	u16 csum_enabled:1;

> +	__be16 csum_info;	/* RMNET_MAP_UL_* */

>  } __aligned(1);

>  

> +/* NOTE:  These field masks are defined in CPU byte order */

> +#define RMNET_MAP_UL_CSUM_INSERT_FMASK	GENMASK(13, 0)

> +#define RMNET_MAP_UL_CSUM_UDP_FMASK	GENMASK(14, 14)   /* 0: IP; 1: UDP */

> +#define RMNET_MAP_UL_CSUM_ENABLED_FMASK	GENMASK(15, 15)

> +

>  #define RMNET_MAP_COMMAND_REQUEST     0

>  #define RMNET_MAP_COMMAND_ACK         1

>  #define RMNET_MAP_COMMAND_UNSUPPORTED 2

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

> index 10d2d582a9ce..72b64114505a 100644

> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

> @@ -207,22 +207,18 @@ rmnet_map_ipv4_ul_csum_header(void *iphdr,

>  			      struct rmnet_map_ul_csum_header *ul_header,

>  			      struct sk_buff *skb)

>  {

> -	struct iphdr *ip4h = (struct iphdr *)iphdr;

> -	__be16 *hdr = (__be16 *)ul_header, offset;

> +	struct iphdr *ip4h = iphdr;

> +	u16 offset;

> +	u16 val;

>  

> -	offset = htons((__force u16)(skb_transport_header(skb) -

> -				     (unsigned char *)iphdr));

> -	ul_header->csum_start_offset = offset;

> -	ul_header->csum_insert_offset = skb->csum_offset;

> -	ul_header->csum_enabled = 1;

> +	offset = skb_transport_header(skb) - (unsigned char *)iphdr;

> +	ul_header->csum_start_offset = htons(offset);

> +

> +	val = u16_encode_bits(skb->csum_offset, RMNET_MAP_UL_CSUM_INSERT_FMASK);

> +	val |= RMNET_MAP_UL_CSUM_ENABLED_FMASK;

>  	if (ip4h->protocol == IPPROTO_UDP)

> -		ul_header->udp_ip4_ind = 1;

> -	else

> -		ul_header->udp_ip4_ind = 0;

> -

> -	/* Changing remaining fields to network order */

> -	hdr++;

> -	*hdr = htons((__force u16)*hdr);

> +		val |= RMNET_MAP_UL_CSUM_UDP_FMASK;

> +	ul_header->csum_info = htons(val);

>  

>  	skb->ip_summed = CHECKSUM_NONE;

>  

> @@ -249,18 +245,16 @@ rmnet_map_ipv6_ul_csum_header(void *ip6hdr,

>  			      struct rmnet_map_ul_csum_header *ul_header,

>  			      struct sk_buff *skb)

>  {

> -	__be16 *hdr = (__be16 *)ul_header, offset;

> +	u16 offset;

> +	u16 val;

>  

> -	offset = htons((__force u16)(skb_transport_header(skb) -

> -				     (unsigned char *)ip6hdr));

> -	ul_header->csum_start_offset = offset;

> -	ul_header->csum_insert_offset = skb->csum_offset;

> -	ul_header->csum_enabled = 1;

> -	ul_header->udp_ip4_ind = 0;

> +	offset = skb_transport_header(skb) - (unsigned char *)ip6hdr;

> +	ul_header->csum_start_offset = htons(offset);

>  

> -	/* Changing remaining fields to network order */

> -	hdr++;

> -	*hdr = htons((__force u16)*hdr);

> +	val = u16_encode_bits(skb->csum_offset, RMNET_MAP_UL_CSUM_INSERT_FMASK);

> +	val |= RMNET_MAP_UL_CSUM_ENABLED_FMASK;

> +	/* Not UDP, so that field is 0 */

> +	ul_header->csum_info = htons(val);

>  

>  	skb->ip_summed = CHECKSUM_NONE;

>  

> @@ -400,8 +394,7 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,

>  	struct rmnet_map_ul_csum_header *ul_header;

>  	void *iphdr;

>  

> -	ul_header = (struct rmnet_map_ul_csum_header *)

> -		    skb_push(skb, sizeof(struct rmnet_map_ul_csum_header));

> +	ul_header = skb_push(skb, sizeof(*ul_header));

>  

>  	if (unlikely(!(orig_dev->features &

>  		     (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))))

> @@ -428,10 +421,7 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,

>  	}

>  

>  sw_csum:

> -	ul_header->csum_start_offset = 0;

> -	ul_header->csum_insert_offset = 0;

> -	ul_header->csum_enabled = 0;

> -	ul_header->udp_ip4_ind = 0;

> +	memset(ul_header, 0, sizeof(*ul_header));

>  

>  	priv->stats.csum_sw++;

>  }

> -- 

> 2.20.1

>
Bjorn Andersson May 20, 2019, 5:17 p.m. | #2
On Mon 20 May 06:53 PDT 2019, Alex Elder wrote:

> The value passed as an argument to rmnet_map_ipv4_ul_csum_header()

> is always an IPv4 header.  Just have the type of the argument

> reflect that rather than obscuring that with a void pointer.  Rename

> it to be consistent with rmnet_map_ipv6_ul_csum_header().

> 

> Signed-off-by: Alex Elder <elder@linaro.org>


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


> ---

>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 9 ++++-----

>  1 file changed, 4 insertions(+), 5 deletions(-)

> 

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

> index a95111cdcd29..61b7dbab2056 100644

> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

> @@ -203,26 +203,25 @@ static void rmnet_map_complement_ipv4_txporthdr_csum_field(void *iphdr)

>  }

>  

>  static void

> -rmnet_map_ipv4_ul_csum_header(void *iphdr,

> +rmnet_map_ipv4_ul_csum_header(struct iphdr *ip4hdr,

>  			      struct rmnet_map_ul_csum_header *ul_header,

>  			      struct sk_buff *skb)

>  {

> -	struct iphdr *ip4h = iphdr;

>  	u16 offset;

>  	u16 val;

>  

> -	offset = skb_transport_header(skb) - (unsigned char *)iphdr;

> +	offset = skb_transport_header(skb) - (unsigned char *)ip4hdr;

>  	ul_header->csum_start_offset = htons(offset);

>  

>  	val = u16_encode_bits(skb->csum_offset, RMNET_MAP_UL_CSUM_INSERT_FMASK);

>  	val |= RMNET_MAP_UL_CSUM_ENABLED_FMASK;

> -	if (ip4h->protocol == IPPROTO_UDP)

> +	if (ip4hdr->protocol == IPPROTO_UDP)

>  		val |= RMNET_MAP_UL_CSUM_UDP_FMASK;

>  	ul_header->csum_info = htons(val);

>  

>  	skb->ip_summed = CHECKSUM_NONE;

>  

> -	rmnet_map_complement_ipv4_txporthdr_csum_field(iphdr);

> +	rmnet_map_complement_ipv4_txporthdr_csum_field(ip4hdr);

>  }

>  

>  #if IS_ENABLED(CONFIG_IPV6)

> -- 

> 2.20.1

>