[PATCH net-next 0/6] net: qualcomm: rmnet: Configuration options

[PATCH net-next 0/6] net: qualcomm: rmnet: Configuration options

[Subash Abhinov Kasiviswanathan]

This series adds support for configuring features on rmnet devices.
The rmnet specific features to be configured here are aggregation and
control commands.

Patch 1 is a cleanup of return codes in the transmit path.
Patch 2 removes some redundant ingress and egress macros.
Patch 3 restricts the creation of rmnet dev to one dev per mux id for a
given real dev.
Patch 4 adds ethernet data path support.
Patches 5-6 add support for configuring features on new and existing
rmnet devices.

Subash Abhinov Kasiviswanathan (6):
  net: qualcomm: rmnet: Remove the rmnet_map_results enum
  net: qualcomm: rmnet: Remove the some redundant macros
  net: qualcomm: rmnet: Allow only one rmnet dev per muxid per real dev
  net: qualcomm: rmnet: Process packets over ethernet
  net: qualcomm: rmnet: Allow to configure flags for new devices
  net: qualcomm: rmnet: Allow to configure flags for existing devices

 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c | 64 ++++++++++++++++++----
 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h |  1 -
 .../net/ethernet/qualcomm/rmnet/rmnet_handlers.c   | 45 +++++++--------
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h    |  9 ---
 .../net/ethernet/qualcomm/rmnet/rmnet_private.h    | 10 +---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c    |  3 +
 6 files changed, 80 insertions(+), 52 deletions(-)

-- 
1.9.1

[PATCH net-next 1/6] net: qualcomm: rmnet: Remove the rmnet_map_results enum

[Subash Abhinov Kasiviswanathan]

Only the success and consumed entries were actually in use.
Use standard error codes instead.

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 .../net/ethernet/qualcomm/rmnet/rmnet_handlers.c    | 21 ++++++++-------------
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h     |  9 ---------
 2 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 29842cc..1e1ea10 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -126,12 +126,12 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
 
 	if (skb_headroom(skb) < required_headroom) {
 		if (pskb_expand_head(skb, required_headroom, 0, GFP_KERNEL))
-			return RMNET_MAP_CONSUMED;
+			goto fail;
 	}
 
 	map_header = rmnet_map_add_map_header(skb, additional_header_len, 0);
 	if (!map_header)
-		return RMNET_MAP_CONSUMED;
+		goto fail;
 
 	if (port->egress_data_format & RMNET_EGRESS_FORMAT_MUXING) {
 		if (mux_id == 0xff)
@@ -142,7 +142,11 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
 
 	skb->protocol = htons(ETH_P_MAP);
 
-	return RMNET_MAP_SUCCESS;
+	return 0;
+
+fail:
+	kfree_skb(skb);
+	return -ENOMEM;
 }
 
 static void
@@ -209,17 +213,8 @@ void rmnet_egress_handler(struct sk_buff *skb)
 	}
 
 	if (port->egress_data_format & RMNET_EGRESS_FORMAT_MAP) {
-		switch (rmnet_map_egress_handler(skb, port, mux_id, orig_dev)) {
-		case RMNET_MAP_CONSUMED:
-			return;
-
-		case RMNET_MAP_SUCCESS:
-			break;
-
-		default:
-			kfree_skb(skb);
+		if (rmnet_map_egress_handler(skb, port, mux_id, orig_dev))
 			return;
-		}
 	}
 
 	rmnet_vnd_tx_fixup(skb, orig_dev);
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index 3af3fe7..4df359d 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -30,15 +30,6 @@ struct rmnet_map_control_command {
 	};
 }  __aligned(1);
 
-enum rmnet_map_results {
-	RMNET_MAP_SUCCESS,
-	RMNET_MAP_CONSUMED,
-	RMNET_MAP_GENERAL_FAILURE,
-	RMNET_MAP_NOT_ENABLED,
-	RMNET_MAP_FAILED_AGGREGATION,
-	RMNET_MAP_FAILED_MUX
-};
-
 enum rmnet_map_commands {
 	RMNET_MAP_COMMAND_NONE,
 	RMNET_MAP_COMMAND_FLOW_DISABLE,
-- 
1.9.1

[PATCH net-next 2/6] net: qualcomm: rmnet: Remove the some redundant macros

[Subash Abhinov Kasiviswanathan]

Multiplexing is always enabled when transmiting from a rmnet device,
so remove the redundant egress macros. De-multiplexing is always
enabled when receiving packets from a rmnet device, so remove those
ingress macros.

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c   | 10 ++--------
 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h   |  1 -
 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 19 +++++++------------
 drivers/net/ethernet/qualcomm/rmnet/rmnet_private.h  | 10 ++--------
 4 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index 71bee1a..5e530db 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -143,11 +143,7 @@ static int rmnet_newlink(struct net *src_net, struct net_device *dev,
 			 struct nlattr *tb[], struct nlattr *data[],
 			 struct netlink_ext_ack *extack)
 {
-	int ingress_format = RMNET_INGRESS_FORMAT_DEMUXING |
-			     RMNET_INGRESS_FORMAT_DEAGGREGATION |
-			     RMNET_INGRESS_FORMAT_MAP;
-	int egress_format = RMNET_EGRESS_FORMAT_MUXING |
-			    RMNET_EGRESS_FORMAT_MAP;
+	int ingress_format = RMNET_INGRESS_FORMAT_DEAGGREGATION;
 	struct net_device *real_dev;
 	int mode = RMNET_EPMODE_VND;
 	struct rmnet_endpoint *ep;
@@ -181,9 +177,7 @@ static int rmnet_newlink(struct net *src_net, struct net_device *dev,
 	if (err)
 		goto err2;
 
-	netdev_dbg(dev, "data format [ingress 0x%08X] [egress 0x%08X]\n",
-		   ingress_format, egress_format);
-	port->egress_data_format = egress_format;
+	netdev_dbg(dev, "data format [ingress 0x%08X]\n", ingress_format);
 	port->ingress_data_format = ingress_format;
 	port->rmnet_mode = mode;
 
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
index c19259e..2ea9fe3 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
@@ -33,7 +33,6 @@ struct rmnet_endpoint {
 struct rmnet_port {
 	struct net_device *dev;
 	u32 ingress_data_format;
-	u32 egress_data_format;
 	u8 nr_rmnet_devs;
 	u8 rmnet_mode;
 	struct hlist_head muxed_ep[RMNET_MAX_LOGICAL_EP];
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 1e1ea10..a46053c 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -133,12 +133,10 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
 	if (!map_header)
 		goto fail;
 
-	if (port->egress_data_format & RMNET_EGRESS_FORMAT_MUXING) {
-		if (mux_id == 0xff)
-			map_header->mux_id = 0;
-		else
-			map_header->mux_id = mux_id;
-	}
+	if (mux_id == 0xff)
+		map_header->mux_id = 0;
+	else
+		map_header->mux_id = mux_id;
 
 	skb->protocol = htons(ETH_P_MAP);
 
@@ -178,8 +176,7 @@ rx_handler_result_t rmnet_rx_handler(struct sk_buff **pskb)
 
 	switch (port->rmnet_mode) {
 	case RMNET_EPMODE_VND:
-		if (port->ingress_data_format & RMNET_INGRESS_FORMAT_MAP)
-			rmnet_map_ingress_handler(skb, port);
+		rmnet_map_ingress_handler(skb, port);
 		break;
 	case RMNET_EPMODE_BRIDGE:
 		rmnet_bridge_handler(skb, port->bridge_ep);
@@ -212,10 +209,8 @@ void rmnet_egress_handler(struct sk_buff *skb)
 		return;
 	}
 
-	if (port->egress_data_format & RMNET_EGRESS_FORMAT_MAP) {
-		if (rmnet_map_egress_handler(skb, port, mux_id, orig_dev))
-			return;
-	}
+	if (rmnet_map_egress_handler(skb, port, mux_id, orig_dev))
+		return;
 
 	rmnet_vnd_tx_fixup(skb, orig_dev);
 
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_private.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_private.h
index 49102f9..d214280 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_private.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_private.h
@@ -19,14 +19,8 @@
 #define RMNET_TX_QUEUE_LEN         1000
 
 /* Constants */
-#define RMNET_EGRESS_FORMAT_MAP                 BIT(1)
-#define RMNET_EGRESS_FORMAT_AGGREGATION         BIT(2)
-#define RMNET_EGRESS_FORMAT_MUXING              BIT(3)
-
-#define RMNET_INGRESS_FORMAT_MAP                BIT(1)
-#define RMNET_INGRESS_FORMAT_DEAGGREGATION      BIT(2)
-#define RMNET_INGRESS_FORMAT_DEMUXING           BIT(3)
-#define RMNET_INGRESS_FORMAT_MAP_COMMANDS       BIT(4)
+#define RMNET_INGRESS_FORMAT_DEAGGREGATION      BIT(0)
+#define RMNET_INGRESS_FORMAT_MAP_COMMANDS       BIT(1)
 
 /* Replace skb->dev to a virtual rmnet device and pass up the stack */
 #define RMNET_EPMODE_VND (1)
-- 
1.9.1

[PATCH net-next 3/6] net: qualcomm: rmnet: Allow only one rmnet dev per muxid per real dev

[Subash Abhinov Kasiviswanathan]

Upon de-multiplexing data from one real dev, the packets can be sent
to an unique rmnet device for a given mux id.

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index 9caa5e3..5bb29f4 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -185,6 +185,9 @@ int rmnet_vnd_newlink(u8 id, struct net_device *rmnet_dev,
 	if (ep->egress_dev)
 		return -EINVAL;
 
+	if (rmnet_get_endpoint(port, id))
+		return -EBUSY;
+
 	rc = register_netdevice(rmnet_dev);
 	if (!rc) {
 		ep->egress_dev = rmnet_dev;
-- 
1.9.1

[PATCH net-next 4/6] net: qualcomm: rmnet: Process packets over ethernet

[Subash Abhinov Kasiviswanathan]

Add support to send and receive packets over ethernet.
An example of usage is testing the data path on UML. This can be
achieved by setting up two UML instances in multicast mode and
associating rmnet over the UML ethernet device.

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index a46053c..ee20f23 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -15,6 +15,7 @@
 
 #include <linux/netdevice.h>
 #include <linux/netdev_features.h>
+#include <linux/if_arp.h>
 #include "rmnet_private.h"
 #include "rmnet_config.h"
 #include "rmnet_vnd.h"
@@ -104,6 +105,12 @@ static void rmnet_set_skb_proto(struct sk_buff *skb)
 {
 	struct sk_buff *skbn;
 
+	if (skb->dev->type == ARPHRD_ETHER) {
+		if (skb_headroom(skb) < ETH_HLEN)
+			kfree_skb(skb);
+		skb_push(skb, ETH_HLEN);
+	}
+
 	if (port->ingress_data_format & RMNET_INGRESS_FORMAT_DEAGGREGATION) {
 		while ((skbn = rmnet_map_deaggregate(skb)) != NULL)
 			__rmnet_map_ingress_handler(skbn, port);
-- 
1.9.1

[PATCH net-next 5/6] net: qualcomm: rmnet: Allow to configure flags for new devices

[Subash Abhinov Kasiviswanathan]

Add an option to configure the rmnet aggregation and command features
on device creation. This is achieved by using the vlan flags option.

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index 5e530db..2f5f661 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -177,11 +177,20 @@ static int rmnet_newlink(struct net *src_net, struct net_device *dev,
 	if (err)
 		goto err2;
 
-	netdev_dbg(dev, "data format [ingress 0x%08X]\n", ingress_format);
-	port->ingress_data_format = ingress_format;
 	port->rmnet_mode = mode;
 
 	hlist_add_head_rcu(&ep->hlnode, &port->muxed_ep[mux_id]);
+
+	if (data[IFLA_VLAN_FLAGS]) {
+		struct ifla_vlan_flags *flags;
+
+		flags = nla_data(data[IFLA_VLAN_FLAGS]);
+		ingress_format = flags->flags & flags->mask;
+	}
+
+	netdev_dbg(dev, "data format [ingress 0x%08X]\n", ingress_format);
+	port->ingress_data_format = ingress_format;
+
 	return 0;
 
 err2:
@@ -312,7 +321,8 @@ static int rmnet_rtnl_validate(struct nlattr *tb[], struct nlattr *data[],
 
 static size_t rmnet_get_size(const struct net_device *dev)
 {
-	return nla_total_size(2); /* IFLA_VLAN_ID */
+	return nla_total_size(2) /* IFLA_VLAN_ID */ +
+	nla_total_size(sizeof(struct ifla_vlan_flags)); /* IFLA_VLAN_FLAGS */
 }
 
 struct rtnl_link_ops rmnet_link_ops __read_mostly = {
-- 
1.9.1

[PATCH net-next 6/6] net: qualcomm: rmnet: Allow to configure flags for existing devices

[Subash Abhinov Kasiviswanathan]

Add an option to configure the mux id, aggregation and commad feature
for existing rmnet devices. Implement the changelink netlink
operation for this.

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index 2f5f661..3ec4092 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -319,6 +319,45 @@ static int rmnet_rtnl_validate(struct nlattr *tb[], struct nlattr *data[],
 	return 0;
 }
 
+static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
+			    struct nlattr *data[],
+			    struct netlink_ext_ack *extack)
+{
+	struct rmnet_priv *priv = netdev_priv(dev);
+	struct net_device *real_dev;
+	struct rmnet_endpoint *ep;
+	struct rmnet_port *port;
+	u16 mux_id;
+
+	real_dev = __dev_get_by_index(dev_net(dev),
+				      nla_get_u32(tb[IFLA_LINK]));
+
+	if (!real_dev || !dev || !rmnet_is_real_dev_registered(real_dev))
+		return -ENODEV;
+
+	port = rmnet_get_port_rtnl(real_dev);
+
+	if (data[IFLA_VLAN_ID]) {
+		mux_id = nla_get_u16(data[IFLA_VLAN_ID]);
+		ep = rmnet_get_endpoint(port, priv->mux_id);
+
+		hlist_del_init_rcu(&ep->hlnode);
+		hlist_add_head_rcu(&ep->hlnode, &port->muxed_ep[mux_id]);
+
+		ep->mux_id = mux_id;
+		priv->mux_id = mux_id;
+	}
+
+	if (data[IFLA_VLAN_FLAGS]) {
+		struct ifla_vlan_flags *flags;
+
+		flags = nla_data(data[IFLA_VLAN_FLAGS]);
+		port->ingress_data_format = flags->flags & flags->mask;
+	}
+
+	return 0;
+}
+
 static size_t rmnet_get_size(const struct net_device *dev)
 {
 	return nla_total_size(2) /* IFLA_VLAN_ID */ +
@@ -334,6 +373,7 @@ struct rtnl_link_ops rmnet_link_ops __read_mostly = {
 	.newlink	= rmnet_newlink,
 	.dellink	= rmnet_dellink,
 	.get_size	= rmnet_get_size,
+	.changelink     = rmnet_changelink,
 };
 
 /* Needs either rcu_read_lock() or rtnl lock */
-- 
1.9.1

Re: [PATCH net-next 1/6] net: qualcomm: rmnet: Remove the rmnet_map_results enum

[David Miller]

From: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Date: Sun,  3 Dec 2017 23:37:03 -0700

> Only the success and consumed entries were actually in use.
> Use standard error codes instead.
> 
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>

That is definitely not the only thing this change is doing:

> @@ -126,12 +126,12 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
>  
>  	if (skb_headroom(skb) < required_headroom) {
>  		if (pskb_expand_head(skb, required_headroom, 0, GFP_KERNEL))
> -			return RMNET_MAP_CONSUMED;
> +			goto fail;
>  	}
>  
>  	map_header = rmnet_map_add_map_header(skb, additional_header_len, 0);
>  	if (!map_header)
> -		return RMNET_MAP_CONSUMED;
> +		goto fail;
>  

Previously these paths would return "RMNET_MAP_CONSUMED":

> @@ -209,17 +213,8 @@ void rmnet_egress_handler(struct sk_buff *skb)
>  	}
>  
>  	if (port->egress_data_format & RMNET_EGRESS_FORMAT_MAP) {
> -		switch (rmnet_map_egress_handler(skb, port, mux_id, orig_dev)) {
> -		case RMNET_MAP_CONSUMED:
> -			return;
> -
> -		case RMNET_MAP_SUCCESS:
> -			break;
> -
> -		default:
> -			kfree_skb(skb);
> +		if (rmnet_map_egress_handler(skb, port, mux_id, orig_dev))
>  			return;
> -		}
>  	}

Causing this code to return.

Now, instead the code jumps to fail:

> @@ -142,7 +142,11 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
>  
>  	skb->protocol = htons(ETH_P_MAP);
>  
> -	return RMNET_MAP_SUCCESS;
> +	return 0;
> +
> +fail:
> +	kfree_skb(skb);
> +	return -ENOMEM;
>  }
>  

Which frees the SKB.

This is a behavioral change, perhaps a bug fix.

Well, nobody knows because you do not explain this at all in your
commit message.

Do not mix functional changes with cleanups.  If you want to change how
freeing the SKB is done, do it in a separate change from the patch that
removes this enumeration.

Thank you.

Re: [PATCH net-next 4/6] net: qualcomm: rmnet: Process packets over ethernet

[David Miller]

From: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Date: Sun,  3 Dec 2017 23:37:06 -0700

> Add support to send and receive packets over ethernet.
> An example of usage is testing the data path on UML. This can be
> achieved by setting up two UML instances in multicast mode and
> associating rmnet over the UML ethernet device.
> 
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> ---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index a46053c..ee20f23 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> @@ -15,6 +15,7 @@
>  
>  #include <linux/netdevice.h>
>  #include <linux/netdev_features.h>
> +#include <linux/if_arp.h>
>  #include "rmnet_private.h"
>  #include "rmnet_config.h"
>  #include "rmnet_vnd.h"
> @@ -104,6 +105,12 @@ static void rmnet_set_skb_proto(struct sk_buff *skb)
>  {
>  	struct sk_buff *skbn;
>  
> +	if (skb->dev->type == ARPHRD_ETHER) {
> +		if (skb_headroom(skb) < ETH_HLEN)
> +			kfree_skb(skb);
> +		skb_push(skb, ETH_HLEN);
> +	}

This will crash, you didn't test the new code path that does the
kfree_skb().

You should use an SKB helper function which will realloc the headroom
if ETH_HLEN is not available, instead of failing.

Re: [PATCH net-next 1/6] net: qualcomm: rmnet: Remove the rmnet_map_results enum

[Subash Abhinov Kasiviswanathan]

> This will crash, you didn't test the new code path that does the
> kfree_skb().
> 
> You should use an SKB helper function which will realloc the headroom
> if ETH_HLEN is not available, instead of failing.
...
> That is definitely not the only thing this change is doing:
> Previously these paths would return "RMNET_MAP_CONSUMED":
> Causing this code to return.
> Now, instead the code jumps to fail:
> 
>> @@ -142,7 +142,11 @@ static int rmnet_map_egress_handler(struct 
>> sk_buff *skb,
>> 
>>  	skb->protocol = htons(ETH_P_MAP);
>> 
>> -	return RMNET_MAP_SUCCESS;
>> +	return 0;
>> +
>> +fail:
>> +	kfree_skb(skb);
>> +	return -ENOMEM;
>>  }
>> 
> 
> Which frees the SKB.
> 
> This is a behavioral change, perhaps a bug fix.
> 
> Well, nobody knows because you do not explain this at all in your
> commit message.
> 
> Do not mix functional changes with cleanups.  If you want to change how
> freeing the SKB is done, do it in a separate change from the patch that
> removes this enumeration.
> 
> Thank you.

Hi David

I will send the change in freeing behavior as a bug fix for net and then
post updates on this series after it.

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

Re: [PATCH net-next 5/6] net: qualcomm: rmnet: Allow to configure flags for new devices

[Dan Williams]

On Sun, 2017-12-03 at 23:37 -0700, Subash Abhinov Kasiviswanathan
wrote:
> Add an option to configure the rmnet aggregation and command features
> on device creation. This is achieved by using the vlan flags option.

Does this overload the VLAN flags item with different meanings than
VLAN_FLAG_* that are specific to rmnet?

Dan

> Signed-off-by: Subash Abhinov Kasiviswanathan
> <subashab@codeaurora.org>
> ---
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c | 16
> +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> index 5e530db..2f5f661 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> @@ -177,11 +177,20 @@ static int rmnet_newlink(struct net *src_net,
> struct net_device *dev,
>  	if (err)
>  		goto err2;
>  
> -	netdev_dbg(dev, "data format [ingress 0x%08X]\n",
> ingress_format);
> -	port->ingress_data_format = ingress_format;
>  	port->rmnet_mode = mode;
>  
>  	hlist_add_head_rcu(&ep->hlnode, &port->muxed_ep[mux_id]);
> +
> +	if (data[IFLA_VLAN_FLAGS]) {
> +		struct ifla_vlan_flags *flags;
> +
> +		flags = nla_data(data[IFLA_VLAN_FLAGS]);
> +		ingress_format = flags->flags & flags->mask;
> +	}
> +
> +	netdev_dbg(dev, "data format [ingress 0x%08X]\n",
> ingress_format);
> +	port->ingress_data_format = ingress_format;
> +
>  	return 0;
>  
>  err2:
> @@ -312,7 +321,8 @@ static int rmnet_rtnl_validate(struct nlattr
> *tb[], struct nlattr *data[],
>  
>  static size_t rmnet_get_size(const struct net_device *dev)
>  {
> -	return nla_total_size(2); /* IFLA_VLAN_ID */
> +	return nla_total_size(2) /* IFLA_VLAN_ID */ +
> +	nla_total_size(sizeof(struct ifla_vlan_flags)); /*
> IFLA_VLAN_FLAGS */
>  }
>  
>  struct rtnl_link_ops rmnet_link_ops __read_mostly = {

Re: [PATCH net-next 5/6] net: qualcomm: rmnet: Allow to configure flags for new devices

[Subash Abhinov Kasiviswanathan]

On 2017-12-08 09:26, Dan Williams wrote:
> On Sun, 2017-12-03 at 23:37 -0700, Subash Abhinov Kasiviswanathan
> wrote:
>> Add an option to configure the rmnet aggregation and command features
>> on device creation. This is achieved by using the vlan flags option.
> 
> Does this overload the VLAN flags item with different meanings than
> VLAN_FLAG_* that are specific to rmnet?
> 
> Dan

Hi Dan

Yes, that's right. I am using it to configure rmnet specific features.

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