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

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

[Alex Elder]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/8] net: qualcomm: rmnet: fix struct rmnet_map_header

[Alex Elder]

The C bit-fields in the first byte of the rmnet_map_header structure
are defined in the wrong order.  The first byte should be formatted
this way:
                 +------- reserved_bit
                 | +----- cd_bit
                 | |
                 v v
    +-----------+-+-+
    |  pad_len  |R|C|
    +-----------+-+-+
     7 6 5 4 3 2 1 0  <-- bit position

But the C bit-fields that define the first byte are defined this way:
    u8 pad_len:6;
    u8 reserved_bit:1;
    u8 cd_bit:1;

And although this isn't portable, I can state that when I build it
the result puts the bit-fields in the wrong location (e.g., the
cd_bit is in bit position 7, when it should be position 0).

Fix this by reordering the definitions of these struct members.
Upcoming patches will reimplement these definitions portably.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index 884f1f52dcc2..b1ae9499c0b2 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -40,9 +40,9 @@ enum rmnet_map_commands {
 };
 
 struct rmnet_map_header {
-	u8  pad_len:6;
-	u8  reserved_bit:1;
 	u8  cd_bit:1;
+	u8  reserved_bit:1;
+	u8  pad_len:6;
 	u8  mux_id;
 	__be16 pkt_len;
 }  __aligned(1);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] net: qualcomm: rmnet: fix struct rmnet_map_header

[Bjorn Andersson]

On Mon 20 May 06:53 PDT 2019, Alex Elder wrote:

> The C bit-fields in the first byte of the rmnet_map_header structure
> are defined in the wrong order.  The first byte should be formatted
> this way:
>                  +------- reserved_bit
>                  | +----- cd_bit
>                  | |
>                  v v
>     +-----------+-+-+
>     |  pad_len  |R|C|
>     +-----------+-+-+
>      7 6 5 4 3 2 1 0  <-- bit position
> 
> But the C bit-fields that define the first byte are defined this way:
>     u8 pad_len:6;
>     u8 reserved_bit:1;
>     u8 cd_bit:1;
> 
> And although this isn't portable, I can state that when I build it
> the result puts the bit-fields in the wrong location (e.g., the
> cd_bit is in bit position 7, when it should be position 0).
> 
> Fix this by reordering the definitions of these struct members.
> Upcoming patches will reimplement these definitions portably.
> 

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

Regards,
Bjorn

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> index 884f1f52dcc2..b1ae9499c0b2 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> @@ -40,9 +40,9 @@ enum rmnet_map_commands {
>  };
>  
>  struct rmnet_map_header {
> -	u8  pad_len:6;
> -	u8  reserved_bit:1;
>  	u8  cd_bit:1;
> +	u8  reserved_bit:1;
> +	u8  pad_len:6;
>  	u8  mux_id;
>  	__be16 pkt_len;
>  }  __aligned(1);
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] net: qualcomm: rmnet: fix struct rmnet_map_header

[Subash Abhinov Kasiviswanathan]

On 2019-05-20 07:53, Alex Elder wrote:
> The C bit-fields in the first byte of the rmnet_map_header structure
> are defined in the wrong order.  The first byte should be formatted
> this way:
>                  +------- reserved_bit
>                  | +----- cd_bit
>                  | |
>                  v v
>     +-----------+-+-+
>     |  pad_len  |R|C|
>     +-----------+-+-+
>      7 6 5 4 3 2 1 0  <-- bit position
> 
> But the C bit-fields that define the first byte are defined this way:
>     u8 pad_len:6;
>     u8 reserved_bit:1;
>     u8 cd_bit:1;
> 

If the above illustration is supposed to be in network byte order,
then it is wrong. The documentation has the definition for the MAP
packet.

Packet format -

Bit             0             1           2-7      8 - 15           16 - 
31
Function   Command / Data   Reserved     Pad   Multiplexer ID    Payload 
length
Bit            32 - x
Function     Raw  Bytes

The driver was written assuming that the host was running ARM64, so
the structs are little endian. (I should have made it compatible
with big and little endian earlier so that is my fault).

In any case, this patch on its own will break the data operation on
ARM64, so it needs to be folded with other patches.

> And although this isn't portable, I can state that when I build it
> the result puts the bit-fields in the wrong location (e.g., the
> cd_bit is in bit position 7, when it should be position 0).
> 
> Fix this by reordering the definitions of these struct members.
> Upcoming patches will reimplement these definitions portably.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> index 884f1f52dcc2..b1ae9499c0b2 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> @@ -40,9 +40,9 @@ enum rmnet_map_commands {
>  };
> 
>  struct rmnet_map_header {
> -	u8  pad_len:6;
> -	u8  reserved_bit:1;
>  	u8  cd_bit:1;
> +	u8  reserved_bit:1;
> +	u8  pad_len:6;
>  	u8  mux_id;
>  	__be16 pkt_len;
>  }  __aligned(1);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] net: qualcomm: rmnet: fix struct rmnet_map_header

[Alex Elder]

On 5/20/19 3:11 PM, Subash Abhinov Kasiviswanathan wrote:
> On 2019-05-20 07:53, Alex Elder wrote:
>> The C bit-fields in the first byte of the rmnet_map_header structure
>> are defined in the wrong order.  The first byte should be formatted
>> this way:
>>                  +------- reserved_bit
>>                  | +----- cd_bit
>>                  | |
>>                  v v
>>     +-----------+-+-+
>>     |  pad_len  |R|C|
>>     +-----------+-+-+
>>      7 6 5 4 3 2 1 0  <-- bit position
>>
>> But the C bit-fields that define the first byte are defined this way:
>>     u8 pad_len:6;
>>     u8 reserved_bit:1;
>>     u8 cd_bit:1;
>>
> 
> If the above illustration is supposed to be in network byte order,
> then it is wrong. The documentation has the definition for the MAP
> packet.

Network *bit* order is irrelevant to the host.  Host memory is
byte addressable only, and bit 0 is the low-order bit.

> Packet format -
> 
> Bit             0             1           2-7      8 - 15           16 - 31
> Function   Command / Data   Reserved     Pad   Multiplexer ID    Payload length
> Bit            32 - x
> Function     Raw  Bytes

I don't know how to interpret this.  Are you saying that the low-
order bit of the first byte is the command/data flag?  Or is it
the high-order bit of the first byte?

I'm saying that what I observed when building the code was that
as originally defined, the cd_bit field was the high-order bit
(bit 7) of the first byte, which I understand to be wrong.

If you are telling me that the command/data flag resides at bit
7 of the first byte, I will update the field masks in a later
patch in this series to reflect that.

> The driver was written assuming that the host was running ARM64, so
> the structs are little endian. (I should have made it compatible
> with big and little endian earlier so that is my fault).

Little endian and big endian have no bearing on the host's
interpretation of bits within a byte.

Please clarify.  I want the patches to be correct, and what
you're explaining doesn't really straighten things out for me.

					-Alex

> In any case, this patch on its own will break the data operation on
> ARM64, so it needs to be folded with other patches.
> 
>> And although this isn't portable, I can state that when I build it
>> the result puts the bit-fields in the wrong location (e.g., the
>> cd_bit is in bit position 7, when it should be position 0).
>>
>> Fix this by reordering the definitions of these struct members.
>> Upcoming patches will reimplement these definitions portably.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
>> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
>> index 884f1f52dcc2..b1ae9499c0b2 100644
>> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
>> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
>> @@ -40,9 +40,9 @@ enum rmnet_map_commands {
>>  };
>>
>>  struct rmnet_map_header {
>> -    u8  pad_len:6;
>> -    u8  reserved_bit:1;
>>      u8  cd_bit:1;
>> +    u8  reserved_bit:1;
>> +    u8  pad_len:6;
>>      u8  mux_id;
>>      __be16 pkt_len;
>>  }  __aligned(1);
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] net: qualcomm: rmnet: fix struct rmnet_map_header

[Subash Abhinov Kasiviswanathan]

>> If the above illustration is supposed to be in network byte order,
>> then it is wrong. The documentation has the definition for the MAP
>> packet.
> 
> Network *bit* order is irrelevant to the host.  Host memory is
> byte addressable only, and bit 0 is the low-order bit.
> 
>> Packet format -
>> 
>> Bit             0             1           2-7      8 - 15           16 
>> - 31
>> Function   Command / Data   Reserved     Pad   Multiplexer ID    
>> Payload length
>> Bit            32 - x
>> Function     Raw  Bytes
> 
> I don't know how to interpret this.  Are you saying that the low-
> order bit of the first byte is the command/data flag?  Or is it
> the high-order bit of the first byte?
> 
> I'm saying that what I observed when building the code was that
> as originally defined, the cd_bit field was the high-order bit
> (bit 7) of the first byte, which I understand to be wrong.
> 
> If you are telling me that the command/data flag resides at bit
> 7 of the first byte, I will update the field masks in a later
> patch in this series to reflect that.
> 

Higher order bit is Command / Data.

>> The driver was written assuming that the host was running ARM64, so
>> the structs are little endian. (I should have made it compatible
>> with big and little endian earlier so that is my fault).
> 
> Little endian and big endian have no bearing on the host's
> interpretation of bits within a byte.
> 
> Please clarify.  I want the patches to be correct, and what
> you're explaining doesn't really straighten things out for me.
> 
> 					-Alex

Can't this bitfields just be used similar to how struct tcphdr &
iphdr are currently defined. That way, you dont have to make
these many changes.

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h 
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index 884f1f5..302d1db 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -40,9 +40,17 @@ enum rmnet_map_commands {
  };

  struct rmnet_map_header {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
         u8  pad_len:6;
         u8  reserved_bit:1;
         u8  cd_bit:1;
+#elif defined (__BIG_ENDIAN_BITFIELD)
+       u8  cd_bit:1;
+       u8  reserved_bit:1;
+       u8  pad_len:6;
+#else
+#error "Please fix <asm/byteorder.h>"
+#endif
         u8  mux_id;
         __be16 pkt_len;
  }  __aligned(1);


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] net: qualcomm: rmnet: fix struct rmnet_map_header

[Alex Elder]

On 5/20/19 8:32 PM, Subash Abhinov Kasiviswanathan wrote:
>>
>> If you are telling me that the command/data flag resides at bit
>> 7 of the first byte, I will update the field masks in a later
>> patch in this series to reflect that.
>>
> 
> Higher order bit is Command / Data.

So what this means is that to get the command/data bit we use:

	first_byte & 0x80

If that is correct I will remove this patch from the series and
will update the subsequent patches so bit 7 is the command bit,
bit 6 is reserved, and bits 0-5 are the pad length.

I will post a v2 of the series with these changes, and will
incorporate Bjorn's "Reviewed-by".

					-Alex

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] net: qualcomm: rmnet: fix struct rmnet_map_header

[Bjorn Andersson]

On Mon 20 May 19:30 PDT 2019, Alex Elder wrote:

> On 5/20/19 8:32 PM, Subash Abhinov Kasiviswanathan wrote:
> >>
> >> If you are telling me that the command/data flag resides at bit
> >> 7 of the first byte, I will update the field masks in a later
> >> patch in this series to reflect that.
> >>
> > 
> > Higher order bit is Command / Data.
> 
> So what this means is that to get the command/data bit we use:
> 
> 	first_byte & 0x80
> 
> If that is correct I will remove this patch from the series and
> will update the subsequent patches so bit 7 is the command bit,
> bit 6 is reserved, and bits 0-5 are the pad length.
> 
> I will post a v2 of the series with these changes, and will
> incorporate Bjorn's "Reviewed-by".
> 

But didn't you say that your testing show that the current bit order is
wrong?

I still like the cleanup, if nothing else just to clarify and clearly
document the actual content of this header.

Regards,
Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] net: qualcomm: rmnet: fix struct rmnet_map_header

[Alex Elder]

On 5/20/19 10:07 PM, Bjorn Andersson wrote:
> On Mon 20 May 19:30 PDT 2019, Alex Elder wrote:
> 
>> On 5/20/19 8:32 PM, Subash Abhinov Kasiviswanathan wrote:
>>>>
>>>> If you are telling me that the command/data flag resides at bit
>>>> 7 of the first byte, I will update the field masks in a later
>>>> patch in this series to reflect that.
>>>>
>>>
>>> Higher order bit is Command / Data.
>>
>> So what this means is that to get the command/data bit we use:
>>
>> 	first_byte & 0x80
>>
>> If that is correct I will remove this patch from the series and
>> will update the subsequent patches so bit 7 is the command bit,
>> bit 6 is reserved, and bits 0-5 are the pad length.
>>
>> I will post a v2 of the series with these changes, and will
>> incorporate Bjorn's "Reviewed-by".
>>
> 
> But didn't you say that your testing show that the current bit order is
> wrong?

I did say that, but it seems I may have been misinterpreting
what the documentation said, namely that "bit 0" in the network
data stream is actually the high-order bit in the first byte.

I did definitely see that bit 7 (0x80) in the first byte was the
one selected by the "cd_bit" C bit-field originally, and I believed
that was wrong.

The other thing I can say is that I never see that bit set in my
use of the rmnet driver for IPA.  On top of that, the pad_len
value is 0.  Given that, either bit order works, because the
whole first byte is 0 either way.  So it turns out the testing
I am able to do is not adequate to verify the change.

I am hoping that Subash has an environment in which QMAP
commands (with the appropriate bit set) are actually used.

I'm going to wait a bit for him to confirm that, but at this
time my plan is to do as I said above--remove this patch and
adjust the ones that follow accordingly.

					-Alex

> I still like the cleanup, if nothing else just to clarify and clearly
> document the actual content of this header.
> 
> Regards,
> Bjorn
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/8] net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros

[Alex Elder]

The following macros, defined in "rmnet_map.h", assume a socket
buffer is provided as an argument without any real indication this
is the case.
    RMNET_MAP_GET_MUX_ID()
    RMNET_MAP_GET_CD_BIT()
    RMNET_MAP_GET_PAD()
    RMNET_MAP_GET_CMD_START()
    RMNET_MAP_GET_LENGTH()
What they hide is pretty trivial accessing of fields in a structure,
and it's much clearer to see this if we do these accesses directly.

So rather than using these accessor macros, assign a local
variable of the map header pointer type to the socket buffer data
pointer, and derereference that pointer variable.

Use the network byte order macros (e.g., ntohs()), not the Linux
byte order functions (e.g. be_to_cpu16()) to convert the big-endian
packet length field, to match the convention used elswhere in the
driver.

There's no need to byte swap 0; it's all zeros irrespective of
endianness.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c |  9 +++++----
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h      | 12 ------------
 .../net/ethernet/qualcomm/rmnet/rmnet_map_command.c  | 11 ++++++++---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c |  4 ++--
 4 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 11167abe5934..4c1b62b72504 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -65,20 +65,21 @@ static void
 __rmnet_map_ingress_handler(struct sk_buff *skb,
 			    struct rmnet_port *port)
 {
+	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_endpoint *ep;
 	u16 len, pad;
 	u8 mux_id;
 
-	if (RMNET_MAP_GET_CD_BIT(skb)) {
+	if (map_header->cd_bit) {
 		if (port->data_format & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
 			return rmnet_map_command(skb, port);
 
 		goto free_skb;
 	}
 
-	mux_id = RMNET_MAP_GET_MUX_ID(skb);
-	pad = RMNET_MAP_GET_PAD(skb);
-	len = RMNET_MAP_GET_LENGTH(skb) - pad;
+	mux_id = map_header->mux_id;
+	pad = map_header->pad_len;
+	len = ntohs(map_header->pkt_len) - pad;
 
 	if (mux_id >= RMNET_MAX_LOGICAL_EP)
 		goto free_skb;
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index b1ae9499c0b2..a30a7b405a11 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -63,18 +63,6 @@ struct rmnet_map_ul_csum_header {
 	u16 csum_enabled:1;
 } __aligned(1);
 
-#define RMNET_MAP_GET_MUX_ID(Y) (((struct rmnet_map_header *) \
-				 (Y)->data)->mux_id)
-#define RMNET_MAP_GET_CD_BIT(Y) (((struct rmnet_map_header *) \
-				(Y)->data)->cd_bit)
-#define RMNET_MAP_GET_PAD(Y) (((struct rmnet_map_header *) \
-				(Y)->data)->pad_len)
-#define RMNET_MAP_GET_CMD_START(Y) ((struct rmnet_map_control_command *) \
-				    ((Y)->data + \
-				      sizeof(struct rmnet_map_header)))
-#define RMNET_MAP_GET_LENGTH(Y) (ntohs(((struct rmnet_map_header *) \
-					(Y)->data)->pkt_len))
-
 #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_command.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
index f6cf59aee212..f675f47c3495 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
@@ -20,12 +20,13 @@ static u8 rmnet_map_do_flow_control(struct sk_buff *skb,
 				    struct rmnet_port *port,
 				    int enable)
 {
+	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_endpoint *ep;
 	struct net_device *vnd;
 	u8 mux_id;
 	int r;
 
-	mux_id = RMNET_MAP_GET_MUX_ID(skb);
+	mux_id = map_header->mux_id;
 
 	if (mux_id >= RMNET_MAX_LOGICAL_EP) {
 		kfree_skb(skb);
@@ -57,6 +58,7 @@ static void rmnet_map_send_ack(struct sk_buff *skb,
 			       unsigned char type,
 			       struct rmnet_port *port)
 {
+	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_map_control_command *cmd;
 	struct net_device *dev = skb->dev;
 
@@ -66,7 +68,8 @@ static void rmnet_map_send_ack(struct sk_buff *skb,
 
 	skb->protocol = htons(ETH_P_MAP);
 
-	cmd = RMNET_MAP_GET_CMD_START(skb);
+	/* Command data immediately follows the header */
+	cmd = (struct rmnet_map_control_command *)(map_header + 1);
 	cmd->cmd_type = type & 0x03;
 
 	netif_tx_lock(dev);
@@ -79,11 +82,13 @@ static void rmnet_map_send_ack(struct sk_buff *skb,
  */
 void rmnet_map_command(struct sk_buff *skb, struct rmnet_port *port)
 {
+	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_map_control_command *cmd;
 	unsigned char command_name;
 	unsigned char rc = 0;
 
-	cmd = RMNET_MAP_GET_CMD_START(skb);
+	/* Command data immediately follows the header */
+	cmd = (struct rmnet_map_control_command *)(map_header + 1);
 	command_name = cmd->command_name;
 
 	switch (command_name) {
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 57a9c314a665..498f20ba1826 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -323,7 +323,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
 		return NULL;
 
 	maph = (struct rmnet_map_header *)skb->data;
-	packet_len = ntohs(maph->pkt_len) + sizeof(struct rmnet_map_header);
+	packet_len = ntohs(maph->pkt_len) + sizeof(*maph);
 
 	if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
 		packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
@@ -332,7 +332,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
 		return NULL;
 
 	/* Some hardware can send us empty frames. Catch them */
-	if (ntohs(maph->pkt_len) == 0)
+	if (!maph->pkt_len)
 		return NULL;
 
 	skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros

[Bjorn Andersson]

On Mon 20 May 06:53 PDT 2019, Alex Elder wrote:

> The following macros, defined in "rmnet_map.h", assume a socket
> buffer is provided as an argument without any real indication this
> is the case.
>     RMNET_MAP_GET_MUX_ID()
>     RMNET_MAP_GET_CD_BIT()
>     RMNET_MAP_GET_PAD()
>     RMNET_MAP_GET_CMD_START()
>     RMNET_MAP_GET_LENGTH()
> What they hide is pretty trivial accessing of fields in a structure,
> and it's much clearer to see this if we do these accesses directly.
> 
> So rather than using these accessor macros, assign a local
> variable of the map header pointer type to the socket buffer data
> pointer, and derereference that pointer variable.
> 
> Use the network byte order macros (e.g., ntohs()), not the Linux
> byte order functions (e.g. be_to_cpu16()) to convert the big-endian
> packet length field, to match the convention used elswhere in the
> driver.
> 
> There's no need to byte swap 0; it's all zeros irrespective of
> endianness.
> 

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

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c |  9 +++++----
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h      | 12 ------------
>  .../net/ethernet/qualcomm/rmnet/rmnet_map_command.c  | 11 ++++++++---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c |  4 ++--
>  4 files changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index 11167abe5934..4c1b62b72504 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> @@ -65,20 +65,21 @@ static void
>  __rmnet_map_ingress_handler(struct sk_buff *skb,
>  			    struct rmnet_port *port)
>  {
> +	struct rmnet_map_header *map_header = (void *)skb->data;
>  	struct rmnet_endpoint *ep;
>  	u16 len, pad;
>  	u8 mux_id;
>  
> -	if (RMNET_MAP_GET_CD_BIT(skb)) {
> +	if (map_header->cd_bit) {
>  		if (port->data_format & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
>  			return rmnet_map_command(skb, port);
>  
>  		goto free_skb;
>  	}
>  
> -	mux_id = RMNET_MAP_GET_MUX_ID(skb);
> -	pad = RMNET_MAP_GET_PAD(skb);
> -	len = RMNET_MAP_GET_LENGTH(skb) - pad;
> +	mux_id = map_header->mux_id;
> +	pad = map_header->pad_len;
> +	len = ntohs(map_header->pkt_len) - pad;
>  
>  	if (mux_id >= RMNET_MAX_LOGICAL_EP)
>  		goto free_skb;
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> index b1ae9499c0b2..a30a7b405a11 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> @@ -63,18 +63,6 @@ struct rmnet_map_ul_csum_header {
>  	u16 csum_enabled:1;
>  } __aligned(1);
>  
> -#define RMNET_MAP_GET_MUX_ID(Y) (((struct rmnet_map_header *) \
> -				 (Y)->data)->mux_id)
> -#define RMNET_MAP_GET_CD_BIT(Y) (((struct rmnet_map_header *) \
> -				(Y)->data)->cd_bit)
> -#define RMNET_MAP_GET_PAD(Y) (((struct rmnet_map_header *) \
> -				(Y)->data)->pad_len)
> -#define RMNET_MAP_GET_CMD_START(Y) ((struct rmnet_map_control_command *) \
> -				    ((Y)->data + \
> -				      sizeof(struct rmnet_map_header)))
> -#define RMNET_MAP_GET_LENGTH(Y) (ntohs(((struct rmnet_map_header *) \
> -					(Y)->data)->pkt_len))
> -
>  #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_command.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
> index f6cf59aee212..f675f47c3495 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
> @@ -20,12 +20,13 @@ static u8 rmnet_map_do_flow_control(struct sk_buff *skb,
>  				    struct rmnet_port *port,
>  				    int enable)
>  {
> +	struct rmnet_map_header *map_header = (void *)skb->data;
>  	struct rmnet_endpoint *ep;
>  	struct net_device *vnd;
>  	u8 mux_id;
>  	int r;
>  
> -	mux_id = RMNET_MAP_GET_MUX_ID(skb);
> +	mux_id = map_header->mux_id;
>  
>  	if (mux_id >= RMNET_MAX_LOGICAL_EP) {
>  		kfree_skb(skb);
> @@ -57,6 +58,7 @@ static void rmnet_map_send_ack(struct sk_buff *skb,
>  			       unsigned char type,
>  			       struct rmnet_port *port)
>  {
> +	struct rmnet_map_header *map_header = (void *)skb->data;
>  	struct rmnet_map_control_command *cmd;
>  	struct net_device *dev = skb->dev;
>  
> @@ -66,7 +68,8 @@ static void rmnet_map_send_ack(struct sk_buff *skb,
>  
>  	skb->protocol = htons(ETH_P_MAP);
>  
> -	cmd = RMNET_MAP_GET_CMD_START(skb);
> +	/* Command data immediately follows the header */
> +	cmd = (struct rmnet_map_control_command *)(map_header + 1);
>  	cmd->cmd_type = type & 0x03;
>  
>  	netif_tx_lock(dev);
> @@ -79,11 +82,13 @@ static void rmnet_map_send_ack(struct sk_buff *skb,
>   */
>  void rmnet_map_command(struct sk_buff *skb, struct rmnet_port *port)
>  {
> +	struct rmnet_map_header *map_header = (void *)skb->data;
>  	struct rmnet_map_control_command *cmd;
>  	unsigned char command_name;
>  	unsigned char rc = 0;
>  
> -	cmd = RMNET_MAP_GET_CMD_START(skb);
> +	/* Command data immediately follows the header */
> +	cmd = (struct rmnet_map_control_command *)(map_header + 1);
>  	command_name = cmd->command_name;
>  
>  	switch (command_name) {
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 57a9c314a665..498f20ba1826 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -323,7 +323,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
>  		return NULL;
>  
>  	maph = (struct rmnet_map_header *)skb->data;
> -	packet_len = ntohs(maph->pkt_len) + sizeof(struct rmnet_map_header);
> +	packet_len = ntohs(maph->pkt_len) + sizeof(*maph);
>  
>  	if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
>  		packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
> @@ -332,7 +332,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
>  		return NULL;
>  
>  	/* Some hardware can send us empty frames. Catch them */
> -	if (ntohs(maph->pkt_len) == 0)
> +	if (!maph->pkt_len)
>  		return NULL;
>  
>  	skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/8] net: qualcomm: rmnet: use field masks instead of C bit-fields

[Alex Elder]

Using C bitfields (e.g. int foo : 3) is not portable.  So stop
using them for the command/data flag and the pad length fields in
the rmnet_map structure.  Instead, use the functions defined in
<linux/bitfield.h> along with field mask constants to extract or
assign values within an integral structure member of a known size.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 5 +++--
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h      | 8 +++++---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 5 ++++-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 4c1b62b72504..5fff6c78ecd5 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -13,6 +13,7 @@
  *
  */
 
+#include <linux/bitfield.h>
 #include <linux/netdevice.h>
 #include <linux/netdev_features.h>
 #include <linux/if_arp.h>
@@ -70,7 +71,7 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 	u16 len, pad;
 	u8 mux_id;
 
-	if (map_header->cd_bit) {
+	if (u8_get_bits(map_header->cmd_pad_len, RMNET_MAP_CMD_FMASK)) {
 		if (port->data_format & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
 			return rmnet_map_command(skb, port);
 
@@ -78,7 +79,7 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 	}
 
 	mux_id = map_header->mux_id;
-	pad = map_header->pad_len;
+	pad = u8_get_bits(map_header->cmd_pad_len, RMNET_MAP_PAD_LEN_FMASK);
 	len = ntohs(map_header->pkt_len) - pad;
 
 	if (mux_id >= RMNET_MAX_LOGICAL_EP)
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index a30a7b405a11..a56209645c81 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -40,13 +40,15 @@ enum rmnet_map_commands {
 };
 
 struct rmnet_map_header {
-	u8  cd_bit:1;
-	u8  reserved_bit:1;
-	u8  pad_len:6;
+	u8  cmd_pad_len;	/* RMNET_MAP_* */
 	u8  mux_id;
 	__be16 pkt_len;
 }  __aligned(1);
 
+#define RMNET_MAP_CMD_FMASK		GENMASK(0, 0)   /* 0: data; 1: cmd */
+#define RMNET_MAP_RESERVED_FMASK	GENMASK(1, 1)
+#define RMNET_MAP_PAD_LEN_FMASK		GENMASK(7, 2)
+
 struct rmnet_map_dl_csum_trailer {
 	u8  reserved1;
 	u8  valid:1;
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 498f20ba1826..10d2d582a9ce 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -13,6 +13,7 @@
  *
  */
 
+#include <linux/bitfield.h>
 #include <linux/netdevice.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
@@ -301,7 +302,9 @@ struct rmnet_map_header *rmnet_map_add_map_header(struct sk_buff *skb,
 
 done:
 	map_header->pkt_len = htons(map_datalen + padding);
-	map_header->pad_len = padding & 0x3F;
+	/* This is a data packet, so cmd field is 0 */
+	map_header->cmd_pad_len =
+			u8_encode_bits(padding, RMNET_MAP_PAD_LEN_FMASK);
 
 	return map_header;
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] net: qualcomm: rmnet: use field masks instead of C bit-fields

[Bjorn Andersson]

On Mon 20 May 06:53 PDT 2019, Alex Elder wrote:

> Using C bitfields (e.g. int foo : 3) is not portable.  So stop
> using them for the command/data flag and the pad length fields in
> the rmnet_map structure.  Instead, use the functions defined in
> <linux/bitfield.h> along with field mask constants to extract or
> assign values within an integral structure member of a known size.
> 

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

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 5 +++--
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h      | 8 +++++---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 5 ++++-
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index 4c1b62b72504..5fff6c78ecd5 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> @@ -13,6 +13,7 @@
>   *
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/netdevice.h>
>  #include <linux/netdev_features.h>
>  #include <linux/if_arp.h>
> @@ -70,7 +71,7 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
>  	u16 len, pad;
>  	u8 mux_id;
>  
> -	if (map_header->cd_bit) {
> +	if (u8_get_bits(map_header->cmd_pad_len, RMNET_MAP_CMD_FMASK)) {
>  		if (port->data_format & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
>  			return rmnet_map_command(skb, port);
>  
> @@ -78,7 +79,7 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
>  	}
>  
>  	mux_id = map_header->mux_id;
> -	pad = map_header->pad_len;
> +	pad = u8_get_bits(map_header->cmd_pad_len, RMNET_MAP_PAD_LEN_FMASK);
>  	len = ntohs(map_header->pkt_len) - pad;
>  
>  	if (mux_id >= RMNET_MAX_LOGICAL_EP)
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> index a30a7b405a11..a56209645c81 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> @@ -40,13 +40,15 @@ enum rmnet_map_commands {
>  };
>  
>  struct rmnet_map_header {
> -	u8  cd_bit:1;
> -	u8  reserved_bit:1;
> -	u8  pad_len:6;
> +	u8  cmd_pad_len;	/* RMNET_MAP_* */
>  	u8  mux_id;
>  	__be16 pkt_len;
>  }  __aligned(1);
>  
> +#define RMNET_MAP_CMD_FMASK		GENMASK(0, 0)   /* 0: data; 1: cmd */
> +#define RMNET_MAP_RESERVED_FMASK	GENMASK(1, 1)
> +#define RMNET_MAP_PAD_LEN_FMASK		GENMASK(7, 2)
> +
>  struct rmnet_map_dl_csum_trailer {
>  	u8  reserved1;
>  	u8  valid:1;
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 498f20ba1826..10d2d582a9ce 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -13,6 +13,7 @@
>   *
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/netdevice.h>
>  #include <linux/ip.h>
>  #include <linux/ipv6.h>
> @@ -301,7 +302,9 @@ struct rmnet_map_header *rmnet_map_add_map_header(struct sk_buff *skb,
>  
>  done:
>  	map_header->pkt_len = htons(map_datalen + padding);
> -	map_header->pad_len = padding & 0x3F;
> +	/* This is a data packet, so cmd field is 0 */
> +	map_header->cmd_pad_len =
> +			u8_encode_bits(padding, RMNET_MAP_PAD_LEN_FMASK);
>  
>  	return map_header;
>  }
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header

[Alex Elder]

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().

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header

[Bjorn Andersson]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Alex Elder]

Replace the use of C bit-fields in the rmnet_map_dl_csum_trailer
structure with a single integral field, using field masks to
encode or get at sub-field values.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h      | 6 ++++--
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index f3231c26badd..fb1cdb4ec41f 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -51,13 +51,15 @@ struct rmnet_map_header {
 
 struct rmnet_map_dl_csum_trailer {
 	u8  reserved1;
-	u8  valid:1;
-	u8  reserved2:7;
+	u8  flags;		/* RMNET_MAP_DL_* */
 	u16 csum_start_offset;
 	u16 csum_length;
 	__be16 csum_value;
 } __aligned(1);
 
+#define RMNET_MAP_DL_CSUM_VALID_FMASK	GENMASK(0, 0)
+#define RMNET_MAP_DL_RESERVED_FMASK	GENMASK(7, 1)
+
 struct rmnet_map_ul_csum_header {
 	__be16 csum_start_offset;
 	__be16 csum_info;	/* RMNET_MAP_UL_* */
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 72b64114505a..a95111cdcd29 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -362,7 +362,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, RMNET_MAP_DL_CSUM_VALID_FMASK)) {
 		priv->stats.csum_valid_unset++;
 		return -EINVAL;
 	}
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Andersson]

On Mon 20 May 06:53 PDT 2019, Alex Elder wrote:

> Replace the use of C bit-fields in the rmnet_map_dl_csum_trailer
> structure with a single integral field, using field masks to
> encode or get at sub-field values.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h      | 6 ++++--
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> index f3231c26badd..fb1cdb4ec41f 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> @@ -51,13 +51,15 @@ struct rmnet_map_header {
>  
>  struct rmnet_map_dl_csum_trailer {
>  	u8  reserved1;
> -	u8  valid:1;
> -	u8  reserved2:7;
> +	u8  flags;		/* RMNET_MAP_DL_* */
>  	u16 csum_start_offset;
>  	u16 csum_length;
>  	__be16 csum_value;
>  } __aligned(1);
>  
> +#define RMNET_MAP_DL_CSUM_VALID_FMASK	GENMASK(0, 0)
> +#define RMNET_MAP_DL_RESERVED_FMASK	GENMASK(7, 1)

I presume that the reserved define won't ever be referenced, but it's
good to have it "documented".

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

Regards,
Bjorn

> +
>  struct rmnet_map_ul_csum_header {
>  	__be16 csum_start_offset;
>  	__be16 csum_info;	/* RMNET_MAP_UL_* */
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 72b64114505a..a95111cdcd29 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -362,7 +362,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, RMNET_MAP_DL_CSUM_VALID_FMASK)) {
>  		priv->stats.csum_valid_unset++;
>  		return -EINVAL;
>  	}
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/8] net: qualcomm: rmnet: get rid of a variable in rmnet_map_ipv4_ul_csum_header()

[Alex Elder]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/8] net: qualcomm: rmnet: get rid of a variable in rmnet_map_ipv4_ul_csum_header()

[Bjorn Andersson]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/8] net: qualcomm: rmnet: mark endianness of struct rmnet_map_dl_csum_trailer fields

[Alex Elder]

Two 16-bit fields (csum_start_offset and csum_length) in the
rmnet_map_dl_csum_trailer structure are currently defined to have
type u16.  But they are in fact big endian values, so should be
properly represented as __be16 values.

No existing code actually references these fields (they're ignored by
rmnet_map_ipv4_dl_csum_trailer() and rmnet_map_ipv6_dl_csum_trailer()).
Changing their type therefore causes no harm, so just fix them.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index fb1cdb4ec41f..775b98d34e94 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -52,8 +52,8 @@ struct rmnet_map_header {
 struct rmnet_map_dl_csum_trailer {
 	u8  reserved1;
 	u8  flags;		/* RMNET_MAP_DL_* */
-	u16 csum_start_offset;
-	u16 csum_length;
+	__be16 csum_start_offset;
+	__be16 csum_length;
 	__be16 csum_value;
 } __aligned(1);
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] net: qualcomm: rmnet: mark endianness of struct rmnet_map_dl_csum_trailer fields

[Bjorn Andersson]

On Mon 20 May 06:53 PDT 2019, Alex Elder wrote:

> Two 16-bit fields (csum_start_offset and csum_length) in the
> rmnet_map_dl_csum_trailer structure are currently defined to have
> type u16.  But they are in fact big endian values, so should be
> properly represented as __be16 values.
> 
> No existing code actually references these fields (they're ignored by
> rmnet_map_ipv4_dl_csum_trailer() and rmnet_map_ipv6_dl_csum_trailer()).
> Changing their type therefore causes no harm, so just fix them.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>

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

> ---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> index fb1cdb4ec41f..775b98d34e94 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> @@ -52,8 +52,8 @@ struct rmnet_map_header {
>  struct rmnet_map_dl_csum_trailer {
>  	u8  reserved1;
>  	u8  flags;		/* RMNET_MAP_DL_* */
> -	u16 csum_start_offset;
> -	u16 csum_length;
> +	__be16 csum_start_offset;
> +	__be16 csum_length;
>  	__be16 csum_value;
>  } __aligned(1);
>  
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Alex Elder]

The IPA driver requires some (but not all) symbols defined in
"drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h".  Create a new
public header file "include/linux/if_rmnet.h" and move the needed
definitions there.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  |  1 +
 .../net/ethernet/qualcomm/rmnet/rmnet_map.h   | 31 -------------
 .../qualcomm/rmnet/rmnet_map_command.c        |  1 +
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  |  1 +
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   |  1 +
 include/linux/if_rmnet.h                      | 45 +++++++++++++++++++
 6 files changed, 49 insertions(+), 31 deletions(-)
 create mode 100644 include/linux/if_rmnet.h

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 5fff6c78ecd5..8e00e14f4ac9 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -18,6 +18,7 @@
 #include <linux/netdev_features.h>
 #include <linux/if_arp.h>
 #include <net/sock.h>
+#include <linux/if_rmnet.h>
 #include "rmnet_private.h"
 #include "rmnet_config.h"
 #include "rmnet_vnd.h"
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index 775b98d34e94..d101cabb04c3 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -39,37 +39,6 @@ enum rmnet_map_commands {
 	RMNET_MAP_COMMAND_ENUM_LENGTH
 };
 
-struct rmnet_map_header {
-	u8  cmd_pad_len;	/* RMNET_MAP_* */
-	u8  mux_id;
-	__be16 pkt_len;
-}  __aligned(1);
-
-#define RMNET_MAP_CMD_FMASK		GENMASK(0, 0)   /* 0: data; 1: cmd */
-#define RMNET_MAP_RESERVED_FMASK	GENMASK(1, 1)
-#define RMNET_MAP_PAD_LEN_FMASK		GENMASK(7, 2)
-
-struct rmnet_map_dl_csum_trailer {
-	u8  reserved1;
-	u8  flags;		/* RMNET_MAP_DL_* */
-	__be16 csum_start_offset;
-	__be16 csum_length;
-	__be16 csum_value;
-} __aligned(1);
-
-#define RMNET_MAP_DL_CSUM_VALID_FMASK	GENMASK(0, 0)
-#define RMNET_MAP_DL_RESERVED_FMASK	GENMASK(7, 1)
-
-struct rmnet_map_ul_csum_header {
-	__be16 csum_start_offset;
-	__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_command.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
index f675f47c3495..6832c5939cae 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/netdevice.h>
+#include <linux/if_rmnet.h>
 #include "rmnet_config.h"
 #include "rmnet_map.h"
 #include "rmnet_private.h"
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 61b7dbab2056..370aee7402e0 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -18,6 +18,7 @@
 #include <linux/ip.h>
 #include <linux/ipv6.h>
 #include <net/ip6_checksum.h>
+#include <linux/if_rmnet.h>
 #include "rmnet_config.h"
 #include "rmnet_map.h"
 #include "rmnet_private.h"
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index d11c16aeb19a..6b39d4d8e523 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -17,6 +17,7 @@
 #include <linux/etherdevice.h>
 #include <linux/if_arp.h>
 #include <net/pkt_sched.h>
+#include <linux/if_rmnet.h>
 #include "rmnet_config.h"
 #include "rmnet_handlers.h"
 #include "rmnet_private.h"
diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
new file mode 100644
index 000000000000..ae60472ecc79
--- /dev/null
+++ b/include/linux/if_rmnet.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2019 Linaro Ltd.
+ */
+#ifndef _LINUX_IF_RMNET_H_
+#define _LINUX_IF_RMNET_H_
+
+#include <linux/types.h>
+
+/* Header structure that precedes packets in ETH_P_MAP protocol */
+struct rmnet_map_header {
+	u8  cmd_pad_len;	/* RMNET_MAP_* */
+	u8  mux_id;
+	__be16 pkt_len;
+}  __aligned(1);
+
+#define RMNET_MAP_CMD_FMASK		GENMASK(0, 0)   /* 0: data; 1: cmd */
+#define RMNET_MAP_RESERVED_FMASK	GENMASK(1, 1)
+#define RMNET_MAP_PAD_LEN_FMASK		GENMASK(7, 2)
+
+/* Checksum offload metadata header for outbound packets*/
+struct rmnet_map_ul_csum_header {
+	__be16 csum_start_offset;
+	__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)
+
+/* Checksum offload metadata trailer for inbound packets */
+struct rmnet_map_dl_csum_trailer {
+	u8  reserved1;
+	u8  flags;		/* RMNET_MAP_DL_* */
+	__be16 csum_start_offset;
+	__be16 csum_length;
+	__be16 csum_value;
+} __aligned(1);
+
+#define RMNET_MAP_DL_CSUM_VALID_FMASK	GENMASK(0, 0)
+#define RMNET_MAP_DL_RESERVED_FMASK	GENMASK(7, 1)
+
+#endif /* _LINUX_IF_RMNET_H_ */
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Andersson]

On Mon 20 May 06:53 PDT 2019, Alex Elder wrote:

> The IPA driver requires some (but not all) symbols defined in
> "drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h".  Create a new
> public header file "include/linux/if_rmnet.h" and move the needed
> definitions there.
> 

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

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  .../ethernet/qualcomm/rmnet/rmnet_handlers.c  |  1 +
>  .../net/ethernet/qualcomm/rmnet/rmnet_map.h   | 31 -------------
>  .../qualcomm/rmnet/rmnet_map_command.c        |  1 +
>  .../ethernet/qualcomm/rmnet/rmnet_map_data.c  |  1 +
>  .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   |  1 +
>  include/linux/if_rmnet.h                      | 45 +++++++++++++++++++
>  6 files changed, 49 insertions(+), 31 deletions(-)
>  create mode 100644 include/linux/if_rmnet.h
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index 5fff6c78ecd5..8e00e14f4ac9 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> @@ -18,6 +18,7 @@
>  #include <linux/netdev_features.h>
>  #include <linux/if_arp.h>
>  #include <net/sock.h>
> +#include <linux/if_rmnet.h>
>  #include "rmnet_private.h"
>  #include "rmnet_config.h"
>  #include "rmnet_vnd.h"
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> index 775b98d34e94..d101cabb04c3 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> @@ -39,37 +39,6 @@ enum rmnet_map_commands {
>  	RMNET_MAP_COMMAND_ENUM_LENGTH
>  };
>  
> -struct rmnet_map_header {
> -	u8  cmd_pad_len;	/* RMNET_MAP_* */
> -	u8  mux_id;
> -	__be16 pkt_len;
> -}  __aligned(1);
> -
> -#define RMNET_MAP_CMD_FMASK		GENMASK(0, 0)   /* 0: data; 1: cmd */
> -#define RMNET_MAP_RESERVED_FMASK	GENMASK(1, 1)
> -#define RMNET_MAP_PAD_LEN_FMASK		GENMASK(7, 2)
> -
> -struct rmnet_map_dl_csum_trailer {
> -	u8  reserved1;
> -	u8  flags;		/* RMNET_MAP_DL_* */
> -	__be16 csum_start_offset;
> -	__be16 csum_length;
> -	__be16 csum_value;
> -} __aligned(1);
> -
> -#define RMNET_MAP_DL_CSUM_VALID_FMASK	GENMASK(0, 0)
> -#define RMNET_MAP_DL_RESERVED_FMASK	GENMASK(7, 1)
> -
> -struct rmnet_map_ul_csum_header {
> -	__be16 csum_start_offset;
> -	__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_command.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
> index f675f47c3495..6832c5939cae 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include <linux/netdevice.h>
> +#include <linux/if_rmnet.h>
>  #include "rmnet_config.h"
>  #include "rmnet_map.h"
>  #include "rmnet_private.h"
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 61b7dbab2056..370aee7402e0 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -18,6 +18,7 @@
>  #include <linux/ip.h>
>  #include <linux/ipv6.h>
>  #include <net/ip6_checksum.h>
> +#include <linux/if_rmnet.h>
>  #include "rmnet_config.h"
>  #include "rmnet_map.h"
>  #include "rmnet_private.h"
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> index d11c16aeb19a..6b39d4d8e523 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> @@ -17,6 +17,7 @@
>  #include <linux/etherdevice.h>
>  #include <linux/if_arp.h>
>  #include <net/pkt_sched.h>
> +#include <linux/if_rmnet.h>
>  #include "rmnet_config.h"
>  #include "rmnet_handlers.h"
>  #include "rmnet_private.h"
> diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
> new file mode 100644
> index 000000000000..ae60472ecc79
> --- /dev/null
> +++ b/include/linux/if_rmnet.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2019 Linaro Ltd.
> + */
> +#ifndef _LINUX_IF_RMNET_H_
> +#define _LINUX_IF_RMNET_H_
> +
> +#include <linux/types.h>
> +
> +/* Header structure that precedes packets in ETH_P_MAP protocol */
> +struct rmnet_map_header {
> +	u8  cmd_pad_len;	/* RMNET_MAP_* */
> +	u8  mux_id;
> +	__be16 pkt_len;
> +}  __aligned(1);
> +
> +#define RMNET_MAP_CMD_FMASK		GENMASK(0, 0)   /* 0: data; 1: cmd */
> +#define RMNET_MAP_RESERVED_FMASK	GENMASK(1, 1)
> +#define RMNET_MAP_PAD_LEN_FMASK		GENMASK(7, 2)
> +
> +/* Checksum offload metadata header for outbound packets*/
> +struct rmnet_map_ul_csum_header {
> +	__be16 csum_start_offset;
> +	__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)
> +
> +/* Checksum offload metadata trailer for inbound packets */
> +struct rmnet_map_dl_csum_trailer {
> +	u8  reserved1;
> +	u8  flags;		/* RMNET_MAP_DL_* */
> +	__be16 csum_start_offset;
> +	__be16 csum_length;
> +	__be16 csum_value;
> +} __aligned(1);
> +
> +#define RMNET_MAP_DL_CSUM_VALID_FMASK	GENMASK(0, 0)
> +#define RMNET_MAP_DL_RESERVED_FMASK	GENMASK(7, 1)
> +
> +#endif /* _LINUX_IF_RMNET_H_ */
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Alex Elder]

On 5/20/19 8:53 AM, Alex Elder wrote:
> 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.

. . .

Bjorn, thank you very much for your quick reviews.	-Alex

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel