[02/18] soc: qcom: create "include/soc/qcom/rmnet.h"

Message ID 20190512012508.10608-3-elder@linaro.org
State New
Headers show
Series
  • net: introduce Qualcomm IPA driver
Related show

Commit Message

Alex Elder May 12, 2019, 1:24 a.m.
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/soc/qcom/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   | 24 ------------
 .../qualcomm/rmnet/rmnet_map_command.c        |  1 +
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  |  1 +
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   |  1 +
 include/soc/qcom/rmnet.h                      | 38 +++++++++++++++++++
 6 files changed, 42 insertions(+), 24 deletions(-)
 create mode 100644 include/soc/qcom/rmnet.h

-- 
2.20.1

Comments

Joe Perches May 12, 2019, 2:34 a.m. | #1
On Sat, 2019-05-11 at 20:24 -0500, Alex Elder wrote:
>  include/soc/qcom/rmnet.h


Should this file be added to the MAINTAINERS file
update in patch 16/18 ?
Alex Elder May 12, 2019, 12:15 p.m. | #2
On 5/11/19 9:34 PM, Joe Perches wrote:
> On Sat, 2019-05-11 at 20:24 -0500, Alex Elder wrote:

>>  include/soc/qcom/rmnet.h

> 

> Should this file be added to the MAINTAINERS file

> update in patch 16/18 ?


Sure, that's a good point.  I'll add it when I submit a v2.
Thank you.

					-Alex
Arnd Bergmann May 15, 2019, 6:59 a.m. | #3
On Sun, May 12, 2019 at 3:25 AM Alex Elder <elder@linaro.org> wrote:

> diff --git a/include/soc/qcom/rmnet.h b/include/soc/qcom/rmnet.h

> new file mode 100644

> index 000000000000..80dcd6e68c3d

> --- /dev/null

> +++ b/include/soc/qcom/rmnet.h

> @@ -0,0 +1,38 @@

> +/* SPDX-License-Identifier: GPL-2.0 */

> +

> +/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved.

> + * Copyright (C) 2018-2019 Linaro Ltd.

> + */

> +#ifndef _SOC_QCOM_RMNET_H_

> +#define _SOC_QCOM_RMNET_H_

> +

> +#include <linux/types.h>

> +

> +/* Header structure that precedes packets in ETH_P_MAP protocol */

> +struct rmnet_map_header {

> +       u8  pad_len             : 6;

> +       u8  reserved_bit        : 1;

> +       u8  cd_bit              : 1;

> +       u8  mux_id;

> +       __be16 pkt_len;

> +}  __aligned(1);


If we move this into include/soc/, I want the structure to be portable,
and avoid the bit fields. Please use mask/shift operations or the
include/linux/bits.h macros instead to make this work with big-endian
kernels.

     Arnd
Alex Elder May 15, 2019, 12:03 p.m. | #4
On 5/15/19 1:59 AM, Arnd Bergmann wrote:
> On Sun, May 12, 2019 at 3:25 AM Alex Elder <elder@linaro.org> wrote:

> 

>> diff --git a/include/soc/qcom/rmnet.h b/include/soc/qcom/rmnet.h

>> new file mode 100644

>> index 000000000000..80dcd6e68c3d

>> --- /dev/null

>> +++ b/include/soc/qcom/rmnet.h

>> @@ -0,0 +1,38 @@

>> +/* SPDX-License-Identifier: GPL-2.0 */

>> +

>> +/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved.

>> + * Copyright (C) 2018-2019 Linaro Ltd.

>> + */

>> +#ifndef _SOC_QCOM_RMNET_H_

>> +#define _SOC_QCOM_RMNET_H_

>> +

>> +#include <linux/types.h>

>> +

>> +/* Header structure that precedes packets in ETH_P_MAP protocol */

>> +struct rmnet_map_header {

>> +       u8  pad_len             : 6;

>> +       u8  reserved_bit        : 1;

>> +       u8  cd_bit              : 1;

>> +       u8  mux_id;

>> +       __be16 pkt_len;

>> +}  __aligned(1);

> 

> If we move this into include/soc/, I want the structure to be portable,

> and avoid the bit fields. Please use mask/shift operations or the

> include/linux/bits.h macros instead to make this work with big-endian

> kernels.


Sure, I'll do that.  I did that everywhere else in the driver,
but here I just tried to preserve the original code as I moved
it.  I will update at least these structures, and all existing
code (plus the IPA code) to use fields masks.

					-Alex
> 

>      Arnd

>
Subash Abhinov Kasiviswanathan May 16, 2019, 1:09 a.m. | #5
>>> +#ifndef _SOC_QCOM_RMNET_H_

>>> +#define _SOC_QCOM_RMNET_H_

>>> +

>>> +#include <linux/types.h>

>>> +

>>> +/* Header structure that precedes packets in ETH_P_MAP protocol */

>>> +struct rmnet_map_header {

>>> +       u8  pad_len             : 6;

>>> +       u8  reserved_bit        : 1;

>>> +       u8  cd_bit              : 1;

>>> +       u8  mux_id;

>>> +       __be16 pkt_len;

>>> +}  __aligned(1);

>> 

>> If we move this into include/soc/, I want the structure to be 

>> portable,

>> and avoid the bit fields. Please use mask/shift operations or the

>> include/linux/bits.h macros instead to make this work with big-endian

>> kernels.

> 

> Sure, I'll do that.  I did that everywhere else in the driver,

> but here I just tried to preserve the original code as I moved

> it.  I will update at least these structures, and all existing

> code (plus the IPA code) to use fields masks.

> 

> 					-Alex

>> 

>>      Arnd

>> 


Hi Alex

Could we instead have the rmnet header definition in
include/linux/if_rmnet.h

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Alex Elder May 17, 2019, 5:27 p.m. | #6
On 5/15/19 8:09 PM, Subash Abhinov Kasiviswanathan wrote:
. . .
> Hi Alex

> 

> Could we instead have the rmnet header definition in

> include/linux/if_rmnet.h


I have no objection to that, but I don't actually know what
the criteria are for putting a file in that directory.

Glancing at other "if_*" files there it seems sensible, but
because I don't know, I'd like to have a little better
justification.

Can you provide a good explanation about why these
definitions belong in "include/linux/if_rmnet.h" instead
of "include/soc/qcom/rmnet.h"?

Thanks.

					-Alex
Subash Abhinov Kasiviswanathan May 17, 2019, 6:08 p.m. | #7
On 2019-05-17 11:27, Alex Elder wrote:
> On 5/15/19 8:09 PM, Subash Abhinov Kasiviswanathan wrote:

> . . .

>> Hi Alex

>> 

>> Could we instead have the rmnet header definition in

>> include/linux/if_rmnet.h

> 

> I have no objection to that, but I don't actually know what

> the criteria are for putting a file in that directory.

> 

> Glancing at other "if_*" files there it seems sensible, but

> because I don't know, I'd like to have a little better

> justification.

> 

> Can you provide a good explanation about why these

> definitions belong in "include/linux/if_rmnet.h" instead

> of "include/soc/qcom/rmnet.h"?

> 

> Thanks.

> 

> 					-Alex


rmnet was designed similar to vlan / macvlan / ipvlan / bridge.
These drivers support creation of virtual netdevices,
define custom rtnl_link_ops, expose netlink attributes to
uapi via if_link.h and register rx_handlers.

They expose some common structs and helpers via if_vlan.h /
if_macvlan.h / if_bridge.h. I would prefer rmnet to use if_rmnet.h
similar to them.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Alex Elder May 19, 2019, 5:37 p.m. | #8
On 5/17/19 1:08 PM, Subash Abhinov Kasiviswanathan wrote:
> On 2019-05-17 11:27, Alex Elder wrote:

. . .
>> Can you provide a good explanation about why these

>> definitions belong in "include/linux/if_rmnet.h" instead

>> of "include/soc/qcom/rmnet.h"?

>>

>> Thanks.

>>

>>                     -Alex

> 

> rmnet was designed similar to vlan / macvlan / ipvlan / bridge.

> These drivers support creation of virtual netdevices,

> define custom rtnl_link_ops, expose netlink attributes to

> uapi via if_link.h and register rx_handlers.

> 

> They expose some common structs and helpers via if_vlan.h /

> if_macvlan.h / if_bridge.h. I would prefer rmnet to use if_rmnet.h

> similar to them.


OK, I will name the file "include/linux/if_rmnet.h" as you suggest.
It will still only define the three structures that I need in the
IPA driver; I won't expose anything else from the rmnet_data driver.

I will mention now that, to facilitate addressing Arnd's concerns
about the portability of using C bit-fields in these structures,
I made a set of other changes (including a bug fix in one of
the structure definitions).  As a preview, here are the subject
lines for that series:
    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
    soc: qcom: ipa: get rid of a variable in rmnet_map_ipv4_ul_csum_header()
    net: create "include/linux/if_rmnet.h"

I will be posting that as a separate series now and will have
the IPA driver series mention a dependence on that.

					-Alex

Patch

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 11167abe5934..3aa79b7ed539 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -17,6 +17,7 @@ 
 #include <linux/netdev_features.h>
 #include <linux/if_arp.h>
 #include <net/sock.h>
+#include <soc/qcom/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 884f1f52dcc2..39d0be99a771 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -39,30 +39,6 @@  enum rmnet_map_commands {
 	RMNET_MAP_COMMAND_ENUM_LENGTH
 };
 
-struct rmnet_map_header {
-	u8  pad_len:6;
-	u8  reserved_bit:1;
-	u8  cd_bit:1;
-	u8  mux_id;
-	__be16 pkt_len;
-}  __aligned(1);
-
-struct rmnet_map_dl_csum_trailer {
-	u8  reserved1;
-	u8  valid:1;
-	u8  reserved2:7;
-	u16 csum_start_offset;
-	u16 csum_length;
-	__be16 csum_value;
-} __aligned(1);
-
-struct rmnet_map_ul_csum_header {
-	__be16 csum_start_offset;
-	u16 csum_insert_offset:14;
-	u16 udp_ip4_ind:1;
-	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 *) \
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
index f6cf59aee212..54b86a8be570 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 <soc/qcom/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 57a9c314a665..e3fb4035820c 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -17,6 +17,7 @@ 
 #include <linux/ip.h>
 #include <linux/ipv6.h>
 #include <net/ip6_checksum.h>
+#include <soc/qcom/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..b8df36e827d4 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 <soc/qcom/rmnet.h>
 #include "rmnet_config.h"
 #include "rmnet_handlers.h"
 #include "rmnet_private.h"
diff --git a/include/soc/qcom/rmnet.h b/include/soc/qcom/rmnet.h
new file mode 100644
index 000000000000..80dcd6e68c3d
--- /dev/null
+++ b/include/soc/qcom/rmnet.h
@@ -0,0 +1,38 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2018-2019 Linaro Ltd.
+ */
+#ifndef _SOC_QCOM_RMNET_H_
+#define _SOC_QCOM_RMNET_H_
+
+#include <linux/types.h>
+
+/* Header structure that precedes packets in ETH_P_MAP protocol */
+struct rmnet_map_header {
+	u8  pad_len		: 6;
+	u8  reserved_bit	: 1;
+	u8  cd_bit		: 1;
+	u8  mux_id;
+	__be16 pkt_len;
+}  __aligned(1);
+
+/* Checksum offload metadata header for outbound packets*/
+struct rmnet_map_ul_csum_header {
+	__be16 csum_start_offset;
+	u16 csum_insert_offset	: 14;
+	u16 udp_ip4_ind		: 1;
+	u16 csum_enabled	: 1;
+} __aligned(1);
+
+/* Checksum offload metadata trailer for inbound packets */
+struct rmnet_map_dl_csum_trailer {
+	u8  reserved1;
+	u8  valid		: 1;
+	u8  reserved2		: 7;
+	u16 csum_start_offset;
+	u16 csum_length;
+	__be16 csum_value;
+} __aligned(1);
+
+#endif /* _SOC_QCOM_RMNET_H_ */