[PATCH net-next v3 0/8] Add HW GRO handling in rmnet

[PATCH net-next v3 0/8] Add HW GRO handling in rmnet

[Subash Abhinov Kasiviswanathan]

This patch series adds downlink packet coalescing support to the rmnet driver.

Hardware can coalesce TCP and UDP packets into a single MAPv5 frame.
A frame can contain up to 48 packets grouped into up to six NLOs.
Packets within one NLO have the same length, while different NLOs may have
different lengths. Since an skb supports only one GSO size, the driver
reconstructs separate skbs for each NLO or packet run.

The coalescing header is a fixed 28-byte structure containing all six NLO
slots. The checksum error bitmap is a single 48-bit, frame-wide stream
consumed least significant bit first. This allows the driver to identify
checksum errors across NLO boundaries and for NLOs containing more than
eight packets.

The driver drops coalesced segments reported with bad transport checksums.
IPv4 UDP packets with a zero checksum remain valid because that checksum is
optional. Multi-packet coalesced frames require RX checksum offload and
hardware GRO support. Single-packet frames are delivered as normal non-GSO
skbs when those features are disabled.

v2->v3:
  - Fix build warnings in the patch "net: qualcomm: rmnet: Add DL coalescing statistics"
  - No changes to the other patches

v2:
https://lore.kernel.org/all/20261008005543.2630828-1-subash.a.kasiviswanathan@oss.qualcomm.com/

v1:
https://lore.kernel.org/all/20260930051345.857443-1-subash.a.kasiviswanathan@oss.qualcomm.com/

Assisted-by: LLM

Subash Abhinov Kasiviswanathan (8):
  net: qualcomm: rmnet: Update MTU handling during format changes
  uapi: if_link: Add RMNET_FLAGS_INGRESS_COALESCE
  net: qualcomm: rmnet: Process MAPv5 frames as a list
  net: qualcomm: rmnet: Restrict supported MAP checksum configurations
  net: qualcomm: rmnet: Add DL packet coalescing support
  net: qualcomm: rmnet: Work around coalescing hardware quirks
  net: qualcomm: rmnet: Add DL coalescing statistics
  docs: networking: Add documentation for the coalescing support in
    rmnet

 .../cellular/qualcomm/rmnet.rst               | 153 ++-
 .../ethernet/qualcomm/rmnet/rmnet_config.c    |  92 +-
 .../ethernet/qualcomm/rmnet/rmnet_config.h    | 104 +-
 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  |  25 +-
 .../net/ethernet/qualcomm/rmnet/rmnet_map.h   |  20 +-
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 983 +++++++++++++++++-
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   | 199 +++-
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.h   |   4 +-
 include/linux/if_rmnet.h                      |  31 +
 include/uapi/linux/if_link.h                  |   1 +
 10 files changed, 1486 insertions(+), 126 deletions(-)

-- 
2.34.1

[PATCH net-next v3 1/8] net: qualcomm: rmnet: Update MTU handling during format changes

[Subash Abhinov Kasiviswanathan]

Calculate the MTU headroom from the requested data format when creating
an rmnet device. Resize existing endpoint MTUs when a newlink or changelink
requests a format that requires a smaller MTU.

Pass the requested format through the MTU helpers so the first device and
existing devices use the same headroom calculation. Validate the target MTU
before resizing existing endpoints and publish the new shared data format
only after all MTUs have been updated.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
v3: no change
v2: https://lore.kernel.org/all/20261008005543.2630828-2-subash.a.kasiviswanathan@oss.qualcomm.com/

 .../ethernet/qualcomm/rmnet/rmnet_config.c    | 55 +++++++++------
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   | 68 +++++++++++++------
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.h   |  4 +-
 3 files changed, 81 insertions(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index 61b04c6c0390..e5a6289b018a 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -143,6 +143,14 @@ static int rmnet_newlink(struct net_device *dev,
 		return -ENODEV;
 	}
 
+	if (data[IFLA_RMNET_FLAGS]) {
+		struct ifla_rmnet_flags *flags;
+
+		flags = nla_data(data[IFLA_RMNET_FLAGS]);
+		data_format &= ~flags->mask;
+		data_format |= flags->flags & flags->mask;
+	}
+
 	ep = kzalloc_obj(*ep);
 	if (!ep)
 		return -ENOMEM;
@@ -154,7 +162,8 @@ static int rmnet_newlink(struct net_device *dev,
 		goto err0;
 
 	port = rmnet_get_port_rtnl(real_dev);
-	err = rmnet_vnd_newlink(mux_id, dev, port, real_dev, ep, extack);
+	err = rmnet_vnd_newlink(mux_id, dev, port, real_dev, ep, extack,
+				data_format);
 	if (err)
 		goto err1;
 
@@ -162,24 +171,23 @@ static int rmnet_newlink(struct net_device *dev,
 	if (err < 0)
 		goto err2;
 
+	/* Update existing MTUs before publishing the shared data format. */
+	err = rmnet_vnd_update_dev_mtu(port, real_dev, data_format);
+	if (err)
+		goto err3;
+
 	port->rmnet_mode = mode;
 	port->rmnet_dev = dev;
 
 	hlist_add_head_rcu(&ep->hlnode, &port->muxed_ep[mux_id]);
 
-	if (data[IFLA_RMNET_FLAGS]) {
-		struct ifla_rmnet_flags *flags;
-
-		flags = nla_data(data[IFLA_RMNET_FLAGS]);
-		data_format &= ~flags->mask;
-		data_format |= flags->flags & flags->mask;
-	}
-
 	netdev_dbg(dev, "data format [0x%08X]\n", data_format);
 	WRITE_ONCE(port->data_format, data_format);
 
 	return 0;
 
+err3:
+	netdev_upper_dev_unlink(real_dev, dev);
 err2:
 	unregister_netdevice(dev);
 	rmnet_vnd_dellink(mux_id, port, ep);
@@ -301,9 +309,13 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
 			    struct netlink_ext_ack *extack)
 {
 	struct rmnet_priv *priv = netdev_priv(dev);
+	struct ifla_rmnet_flags *flags;
 	struct net_device *real_dev;
 	struct rmnet_port *port;
+	u32 old_data_format;
+	u32 data_format;
 	u16 mux_id;
+	int err;
 
 	if (!dev)
 		return -ENODEV;
@@ -320,6 +332,13 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
 
 	port = rmnet_get_port_rtnl(real_dev);
 
+	if (data[IFLA_RMNET_FLAGS]) {
+		old_data_format = READ_ONCE(port->data_format);
+		flags = nla_data(data[IFLA_RMNET_FLAGS]);
+		data_format = old_data_format & ~flags->mask;
+		data_format |= flags->flags & flags->mask;
+	}
+
 	if (data[IFLA_RMNET_MUX_ID]) {
 		mux_id = nla_get_u16(data[IFLA_RMNET_MUX_ID]);
 
@@ -346,21 +365,13 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
 	}
 
 	if (data[IFLA_RMNET_FLAGS]) {
-		struct ifla_rmnet_flags *flags;
-		u32 old_data_format;
-		u32 data_format;
-
-		old_data_format = port->data_format;
-		flags = nla_data(data[IFLA_RMNET_FLAGS]);
-		data_format = old_data_format & ~flags->mask;
-		data_format |= flags->flags & flags->mask;
-		WRITE_ONCE(port->data_format, data_format);
-
-		if (rmnet_vnd_update_dev_mtu(port, real_dev)) {
-			WRITE_ONCE(port->data_format, old_data_format);
+		err = rmnet_vnd_update_dev_mtu(port, real_dev, data_format);
+		if (err) {
 			NL_SET_ERR_MSG_MOD(extack, "Invalid MTU on real dev");
-			return -EINVAL;
+			return err;
 		}
+
+		WRITE_ONCE(port->data_format, data_format);
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index 5f921cddf82b..d23f74b0aa47 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -66,27 +66,25 @@ static netdev_tx_t rmnet_vnd_start_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
-static int rmnet_vnd_headroom(struct rmnet_port *port)
+static int rmnet_vnd_headroom(u32 data_format)
 {
 	u32 headroom;
 
 	headroom = sizeof(struct rmnet_map_header);
 
-	if (port->data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
+	if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
 		headroom += sizeof(struct rmnet_map_ul_csum_header);
 
 	return headroom;
 }
 
-static int rmnet_vnd_change_mtu(struct net_device *rmnet_dev, int new_mtu)
+static int rmnet_vnd_change_mtu_with_format(struct net_device *rmnet_dev,
+					    int new_mtu, u32 data_format)
 {
 	struct rmnet_priv *priv = netdev_priv(rmnet_dev);
-	struct rmnet_port *port;
 	u32 headroom;
 
-	port = rmnet_get_port_rtnl(priv->real_dev);
-
-	headroom = rmnet_vnd_headroom(port);
+	headroom = rmnet_vnd_headroom(data_format);
 
 	if (new_mtu < 0 || new_mtu > RMNET_MAX_PACKET_SIZE ||
 	    new_mtu > (priv->real_dev->mtu - headroom))
@@ -96,6 +94,17 @@ static int rmnet_vnd_change_mtu(struct net_device *rmnet_dev, int new_mtu)
 	return 0;
 }
 
+static int rmnet_vnd_change_mtu(struct net_device *rmnet_dev, int new_mtu)
+{
+	struct rmnet_priv *priv = netdev_priv(rmnet_dev);
+	struct rmnet_port *port;
+
+	port = rmnet_get_port_rtnl(priv->real_dev);
+
+	return rmnet_vnd_change_mtu_with_format(rmnet_dev, new_mtu,
+						READ_ONCE(port->data_format));
+}
+
 static int rmnet_vnd_get_iflink(const struct net_device *dev)
 {
 	struct rmnet_priv *priv = netdev_priv(dev);
@@ -308,8 +317,7 @@ int rmnet_vnd_newlink(u8 id, struct net_device *rmnet_dev,
 		      struct rmnet_port *port,
 		      struct net_device *real_dev,
 		      struct rmnet_endpoint *ep,
-		      struct netlink_ext_ack *extack)
-
+		      struct netlink_ext_ack *extack, u32 data_format)
 {
 	struct rmnet_priv *priv = netdev_priv(rmnet_dev);
 	u32 headroom;
@@ -326,9 +334,11 @@ int rmnet_vnd_newlink(u8 id, struct net_device *rmnet_dev,
 
 	priv->real_dev = real_dev;
 
-	headroom = rmnet_vnd_headroom(port);
+	headroom = rmnet_vnd_headroom(data_format);
 
-	if (rmnet_vnd_change_mtu(rmnet_dev, real_dev->mtu - headroom)) {
+	if (rmnet_vnd_change_mtu_with_format(rmnet_dev,
+					     real_dev->mtu - headroom,
+					     data_format)) {
 		NL_SET_ERR_MSG_MOD(extack, "Invalid MTU on real dev");
 		return -EINVAL;
 	}
@@ -375,17 +385,15 @@ int rmnet_vnd_do_flow_control(struct net_device *rmnet_dev, int enable)
 	return 0;
 }
 
-int rmnet_vnd_validate_real_dev_mtu(struct net_device *real_dev)
+static int rmnet_vnd_validate_mtu(struct rmnet_port *port,
+				  struct net_device *real_dev, u32 data_format)
 {
 	struct hlist_node *tmp_ep;
 	struct rmnet_endpoint *ep;
-	struct rmnet_port *port;
 	unsigned long bkt_ep;
 	u32 headroom;
 
-	port = rmnet_get_port_rtnl(real_dev);
-
-	headroom = rmnet_vnd_headroom(port);
+	headroom = rmnet_vnd_headroom(data_format);
 
 	hash_for_each_safe(port->muxed_ep, bkt_ep, tmp_ep, ep, hlnode) {
 		if (ep->egress_dev->mtu > (real_dev->mtu - headroom))
@@ -395,23 +403,39 @@ int rmnet_vnd_validate_real_dev_mtu(struct net_device *real_dev)
 	return 0;
 }
 
+int rmnet_vnd_validate_real_dev_mtu(struct net_device *real_dev)
+{
+	struct rmnet_port *port;
+
+	port = rmnet_get_port_rtnl(real_dev);
+
+	return rmnet_vnd_validate_mtu(port, real_dev,
+				      READ_ONCE(port->data_format));
+}
+
 int rmnet_vnd_update_dev_mtu(struct rmnet_port *port,
-			     struct net_device *real_dev)
+			     struct net_device *real_dev, u32 data_format)
 {
 	struct hlist_node *tmp_ep;
 	struct rmnet_endpoint *ep;
 	unsigned long bkt_ep;
 	u32 headroom;
+	int new_mtu;
 
-	headroom = rmnet_vnd_headroom(port);
+	headroom = rmnet_vnd_headroom(data_format);
+	if (real_dev->mtu < headroom)
+		return -EINVAL;
+
+	new_mtu = real_dev->mtu - headroom;
+	if (new_mtu > RMNET_MAX_PACKET_SIZE)
+		return -EINVAL;
 
 	hash_for_each_safe(port->muxed_ep, bkt_ep, tmp_ep, ep, hlnode) {
-		if (ep->egress_dev->mtu <= (real_dev->mtu - headroom))
+		if (ep->egress_dev->mtu <= new_mtu)
 			continue;
 
-		if (rmnet_vnd_change_mtu(ep->egress_dev,
-					 real_dev->mtu - headroom))
-			return -1;
+		/* The target MTU was validated before updating any endpoint. */
+		WRITE_ONCE(ep->egress_dev->mtu, new_mtu);
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h
index c2b2baf86894..901ef36900ac 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h
@@ -12,7 +12,7 @@ int rmnet_vnd_newlink(u8 id, struct net_device *rmnet_dev,
 		      struct rmnet_port *port,
 		      struct net_device *real_dev,
 		      struct rmnet_endpoint *ep,
-		      struct netlink_ext_ack *extack);
+		      struct netlink_ext_ack *extack, u32 data_format);
 int rmnet_vnd_dellink(u8 id, struct rmnet_port *port,
 		      struct rmnet_endpoint *ep);
 void rmnet_vnd_rx_fixup(struct sk_buff *skb, struct net_device *dev);
@@ -21,5 +21,5 @@ void rmnet_vnd_tx_fixup(struct sk_buff *skb, struct net_device *dev);
 void rmnet_vnd_setup(struct net_device *dev);
 int rmnet_vnd_validate_real_dev_mtu(struct net_device *real_dev);
 int rmnet_vnd_update_dev_mtu(struct rmnet_port *port,
-			     struct net_device *real_dev);
+			     struct net_device *real_dev, u32 data_format);
 #endif /* _RMNET_VND_H_ */
-- 
2.34.1

Re: [PATCH net-next v3 1/8] net: qualcomm: rmnet: Update MTU handling during format changes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] rmnet_vnd_headroom(u32 data_format) in rmnet_vnd.c checks only
  RMNET_FLAGS_EGRESS_MAP_CKSUMV4.
- [Medium] rmnet_vnd_update_dev_mtu()
  (drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c) now returns -EINVAL…
- [Medium] rmnet_vnd_update_dev_mtu() shrinks registered endpoint devices
  with a bare `WRITE_ONCE(ep->egress_dev->mtu, new_mtu)`…
- [Low] The patch fixes a user-visible MTU bug, but the commit message
  calls it an 'update' and there is no Fixes: tag.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  rmnet_vnd_validate_mtu() (rmnet_vnd.c:399) computes `real_dev->mtu -…

--

net: qualcomm: rmnet: Update MTU handling during format changes

When an rmnet device is created, this patch computes the MTU headroom
from the requested data format. On newlink or changelink, it shrinks
existing endpoint MTUs when the new format needs more headroom. The new
shared data format is published only after the MTUs have been updated.

> Calculate the MTU headroom from the requested data format when creating
> an rmnet device. Resize existing endpoint MTUs when a newlink or changelink
> requests a format that requires a smaller MTU.

[Severity: Low]
This looks like a bug fix, but the commit message describes it as an update.

Before this patch, rmnet_newlink() called rmnet_vnd_newlink() before it
applied IFLA_RMNET_FLAGS. For the first device on a port, the headroom
therefore came from port->data_format == 0, which gives 4 bytes.

A device created with RMNET_FLAGS_EGRESS_MAP_CKSUMV4 got an MTU of
real_dev->mtu - 4 instead of real_dev->mtu - 8. Its egress frames were
then 4 bytes larger than the real device MTU.

Should the commit message describe that symptom and carry a tag like this?

Fixes: b7f5eb6ba21b ("net: qualcomm: rmnet: Update rmnet device MTU based on real device")

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> index 5f921cddf82b4..d23f74b0aa471 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> @@ -66,27 +66,25 @@ static netdev_tx_t rmnet_vnd_start_xmit(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> -static int rmnet_vnd_headroom(struct rmnet_port *port)
> +static int rmnet_vnd_headroom(u32 data_format)
>  {
>  	u32 headroom;
>  
>  	headroom = sizeof(struct rmnet_map_header);
>  
> -	if (port->data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
> +	if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
>  		headroom += sizeof(struct rmnet_map_ul_csum_header);
>  
>  	return headroom;
>  }

[Severity: High]
Should this headroom calculation also handle
RMNET_FLAGS_EGRESS_MAP_CKSUMV5?

rmnet_map_egress_handler() in rmnet_handlers.c reserves room for the V5
header:

	} else if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5) {
		additional_header_len = sizeof(struct rmnet_map_v5_csum_header);
		csum_type = RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
	}

rmnet_map_v5_checksum_uplink_packet() always skb_push()es that header.
The 4 byte MAP header is added after it.

With egress MAPv5, rmnet_vnd_headroom() returns 4 instead of 8. So
rmnet_vnd_newlink() sets the default MTU to real_dev->mtu - 4, and
full-size uplink frames are 4 bytes larger than the real device MTU.

For the same reason, rmnet_vnd_update_dev_mtu() does not shrink existing
endpoints when a newlink or changelink switches to egress MAPv5.

The commit message says MTUs are resized "when a newlink or changelink
requests a format that requires a smaller MTU". Isn't egress MAPv5 one of
those formats? The later patches in the series don't seem to change
rmnet_vnd_headroom() either.

[ ... ]

> @@ -375,17 +385,15 @@ int rmnet_vnd_do_flow_control(struct net_device *rmnet_dev, int enable)
>  	return 0;
>  }
>  
> -int rmnet_vnd_validate_real_dev_mtu(struct net_device *real_dev)
> +static int rmnet_vnd_validate_mtu(struct rmnet_port *port,
> +				  struct net_device *real_dev, u32 data_format)
>  {

[ ... ]

> -	headroom = rmnet_vnd_headroom(port);
> +	headroom = rmnet_vnd_headroom(data_format);
>  
>  	hash_for_each_safe(port->muxed_ep, bkt_ep, tmp_ep, ep, hlnode) {
>  		if (ep->egress_dev->mtu > (real_dev->mtu - headroom))

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
can real_dev->mtu - headroom wrap here?

Both operands are unsigned. dummy has min_mtu = 0, so its MTU can be set
below the headroom, for example to 1. The subtraction then wraps to a
very large value and this check never fires. As a result, the
NETDEV_CHANGEMTU veto in rmnet_config_notify_cb() accepts the invalid
lower MTU.

rmnet_vnd_change_mtu_with_format() has the same comparison:

	if (new_mtu < 0 || new_mtu > RMNET_MAX_PACKET_SIZE ||
	    new_mtu > (priv->real_dev->mtu - headroom))

After that, it accepts any endpoint MTU up to RMNET_MAX_PACKET_SIZE.

This patch adds the real_dev->mtu < headroom guard only to
rmnet_vnd_update_dev_mtu(). Would these two helpers need the same check?

> @@ -395,23 +403,39 @@ int rmnet_vnd_validate_real_dev_mtu(struct net_device *real_dev)

[ ... ]

>  int rmnet_vnd_update_dev_mtu(struct rmnet_port *port,
> -			     struct net_device *real_dev)
> +			     struct net_device *real_dev, u32 data_format)
>  {
>  	struct hlist_node *tmp_ep;
>  	struct rmnet_endpoint *ep;
>  	unsigned long bkt_ep;
>  	u32 headroom;
> +	int new_mtu;
>  
> -	headroom = rmnet_vnd_headroom(port);
> +	headroom = rmnet_vnd_headroom(data_format);
> +	if (real_dev->mtu < headroom)
> +		return -EINVAL;
> +
> +	new_mtu = real_dev->mtu - headroom;
> +	if (new_mtu > RMNET_MAX_PACKET_SIZE)
> +		return -EINVAL;

[Severity: Medium]
Does this new check reject format changes that used to succeed?

rmnet_vnd_change_mtu_with_format() already caps endpoint MTUs at
RMNET_MAX_PACKET_SIZE. So when real_dev->mtu - headroom was above 16384,
the old loop took the continue path for every endpoint and returned 0.

The real device MTU can be raised after rmnet devices exist, because this
call chain only rejects an MTU that becomes too small:

rmnet_config_notify_cb()->rmnet_vnd_validate_real_dev_mtu()->
rmnet_vnd_validate_mtu()

For example, dummy has max_mtu = 0.

Once that happens, every flags change through rmnet_changelink() fails
here:

		err = rmnet_vnd_update_dev_mtu(port, real_dev, data_format);
		if (err) {
			NL_SET_ERR_MSG_MOD(extack, "Invalid MTU on real dev");
			return err;
		}

By that point, any IFLA_RMNET_MUX_ID change in the same request has
already been applied. The newlink path is not affected, since
rmnet_vnd_newlink() already rejects this case.

Would it keep the old behavior to clamp the target to
min(real_dev->mtu - headroom, RMNET_MAX_PACKET_SIZE), or to return 0
when the target is above the cap?

>  
>  	hash_for_each_safe(port->muxed_ep, bkt_ep, tmp_ep, ep, hlnode) {
> -		if (ep->egress_dev->mtu <= (real_dev->mtu - headroom))
> +		if (ep->egress_dev->mtu <= new_mtu)
>  			continue;
>  
> -		if (rmnet_vnd_change_mtu(ep->egress_dev,
> -					 real_dev->mtu - headroom))
> -			return -1;
> +		/* The target MTU was validated before updating any endpoint. */
> +		WRITE_ONCE(ep->egress_dev->mtu, new_mtu);

[Severity: Medium]
Is it safe to change the MTU of a registered netdev with a bare
WRITE_ONCE() here?

This bypasses dev_set_mtu(), so:

  - the NETDEV_PRECHANGEMTU and NETDEV_CHANGEMTU notifiers don't run
  - min_mtu isn't checked
  - no RTM_NEWLINK is sent to userspace

The new call in rmnet_newlink() has a side effect. Creating a second
rmnet device that requests RMNET_FLAGS_EGRESS_MAP_CKSUMV4 now silently
lowers the MTU of existing sibling devices, which may be running. The
changelink path already behaved this way before this patch; the newlink
caller is new.

addrconf_notify() updates route MTUs and idev->cnf.mtu6 only on
NETDEV_CHANGEMTU, and ip6_dst_mtu_maybe_forward() reads cnf.mtu6.

For example, take an endpoint at 1496 on a 1500 byte real device. It is
shrunk to 1492, but IPv6 keeps sending 1496 byte packets, which become
1504 bytes after MAP encapsulation.

If the shrink takes the device below IPV6_MIN_MTU or 68, the
addrconf_ifdown() and inetdev teardown don't happen either.

Would calling dev_set_mtu() on each endpoint work instead? The target is
smaller than the current MTU, so it should still pass
rmnet_vnd_change_mtu() validation.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009084351.317632-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next v3 2/8] uapi: if_link: Add RMNET_FLAGS_INGRESS_COALESCE

[Subash Abhinov Kasiviswanathan]

Add bit 6 of the rmnet ingress flags to allow rmnet to process incoming
packets in the MAPv5 coalescing header format.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
v3: no change
v2: https://lore.kernel.org/all/20261008005543.2630828-3-subash.a.kasiviswanathan@oss.qualcomm.com/
v1: https://lore.kernel.org/all/20260930051345.857443-2-subash.a.kasiviswanathan@oss.qualcomm.com/

 include/uapi/linux/if_link.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 245b36204525..349fcddc6622 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -2022,6 +2022,7 @@ enum {
 #define RMNET_FLAGS_EGRESS_MAP_CKSUMV4            (1U << 3)
 #define RMNET_FLAGS_INGRESS_MAP_CKSUMV5           (1U << 4)
 #define RMNET_FLAGS_EGRESS_MAP_CKSUMV5            (1U << 5)
+#define RMNET_FLAGS_INGRESS_COALESCE              (1U << 6)
 
 enum {
 	IFLA_RMNET_UNSPEC,
-- 
2.34.1

Re: [PATCH net-next v3 2/8] uapi: if_link: Add RMNET_FLAGS_INGRESS_COALESCE

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] rmnet_vnd_headroom() in
  drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c adds sizeof(struct…
- [Low] Not a valid finding (baseline decision false).

--

uapi: if_link: Add RMNET_FLAGS_INGRESS_COALESCE

This patch adds bit 6 to the rmnet data format flags, named
RMNET_FLAGS_INGRESS_COALESCE. The bit lets rmnet be configured to process
ingress packets in the MAPv5 coalescing header format. Later patches in
the series add the kernel support for it.

> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 245b362045253..349fcddc6622f 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -2022,6 +2022,7 @@ enum {
>  #define RMNET_FLAGS_EGRESS_MAP_CKSUMV4            (1U << 3)
>  #define RMNET_FLAGS_INGRESS_MAP_CKSUMV5           (1U << 4)
>  #define RMNET_FLAGS_EGRESS_MAP_CKSUMV5            (1U << 5)
> +#define RMNET_FLAGS_INGRESS_COALESCE              (1U << 6)

[Severity: Low]
At this commit nothing under drivers/net/ethernet/qualcomm/rmnet reads the
new bit:

- rmnet_newlink() and rmnet_changelink() accept it and store it in
  port->data_format.
- rmnet_fill_info() reports it back to user space.
- The ingress path ignores it.
- Nothing ties it to RMNET_FLAGS_INGRESS_MAP_CKSUMV5.

A later patch in the series, "net: qualcomm: rmnet: Add DL packet
coalescing support", closes this gap. It reads the flag in
__rmnet_map_ingress_handler(), rmnet_map_validate_packet_len() and
rmnet_map_process_next_hdr_packet(). It also makes
rmnet_config_data_format_valid() reject COALESCE unless
INGRESS_MAP_CKSUMV5 is set.

Before this patch, bit 6 was already accepted silently like any other bit.
The define on its own changes no kernel behaviour.

Is it intended for the uAPI bit to land before its implementation? Or
would it be simpler to fold the define into the patch that adds the users?

I also have a question about the previous patch in this range, commit
5a64ed6e084a ("net: qualcomm: rmnet: Update MTU handling during format
changes"):

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> index 5f921cddf82b4..d23f74b0aa471 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> @@ -66,27 +66,25 @@ static netdev_tx_t rmnet_vnd_start_xmit(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> -static int rmnet_vnd_headroom(struct rmnet_port *port)
> +static int rmnet_vnd_headroom(u32 data_format)
>  {
>  	u32 headroom;
>  
>  	headroom = sizeof(struct rmnet_map_header);
>  
> -	if (port->data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
> +	if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
>  		headroom += sizeof(struct rmnet_map_ul_csum_header);
>  
>  	return headroom;
>  }

[Severity: High]
Should rmnet_vnd_headroom() also count RMNET_FLAGS_EGRESS_MAP_CKSUMV5?

For that format, rmnet_map_egress_handler() adds a 4 byte struct
rmnet_map_v5_csum_header on top of the MAP header:

	} else if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5) {
		additional_header_len = sizeof(struct rmnet_map_v5_csum_header);
		csum_type = RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
	}

That makes the overhead on the wire 8 bytes. rmnet_vnd_headroom() returns
only sizeof(struct rmnet_map_header), which is 4 bytes.

The CKSUMV4-only check was there before this series. However, this patch
now uses it to resize endpoints whenever a newlink or changelink asks for
a format that needs a smaller MTU.

With EGRESS_MAP_CKSUMV5 enabled, rmnet_vnd_newlink() and
rmnet_vnd_update_dev_mtu() leave endpoints at real_dev->mtu - 4. A
full-size egress packet then becomes a MAP frame of real_dev->mtu + 4
bytes on the real device.

If the real device or modem enforces its MTU, could it drop these frames?

"Restrict supported MAP checksum configurations" comes later in the
series. After it, a MAPv5 device (which coalescing requires) can only use
no checksum or CKSUMV5 on egress, so the common MAPv5 setup hits this.
rmnet_vnd_headroom() is still unchanged at the tip of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009084351.317632-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next v3 3/8] net: qualcomm: rmnet: Process MAPv5 frames as a list

[Subash Abhinov Kasiviswanathan]

Add support for handling the MAP packets as a list in preparation for
the subsequent patch in the series  which adds support for handling the
coalescing packets. This is needed as a single coalescing MAP packet could
yield multiple IP packets.

There is no functional change in the handling of the previously
supported MAP packet formats.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
v3: no change
v2: https://lore.kernel.org/all/20261008005543.2630828-4-subash.a.kasiviswanathan@oss.qualcomm.com/
v1: https://lore.kernel.org/all/20260930051345.857443-3-subash.a.kasiviswanathan@oss.qualcomm.com/

 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  | 22 +++++--
 .../net/ethernet/qualcomm/rmnet/rmnet_map.h   |  4 +-
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 57 ++++++++++++-------
 3 files changed, 58 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index aa5523f4618e..ab1dfbd833e3 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -50,6 +50,16 @@ rmnet_deliver_skb(struct sk_buff *skb)
 	gro_cells_receive(&priv->gro_cells, skb);
 }
 
+static void rmnet_deliver_skb_list(struct sk_buff_head *head)
+{
+	struct sk_buff *skb;
+
+	while ((skb = __skb_dequeue(head))) {
+		rmnet_set_skb_proto(skb);
+		rmnet_deliver_skb(skb);
+	}
+}
+
 /* MAP handler */
 
 static void
@@ -59,6 +69,7 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 {
 	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_endpoint *ep;
+	struct sk_buff_head list;
 	u16 len, pad;
 	u8 mux_id;
 
@@ -83,12 +94,12 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 
 	skb->dev = ep->egress_dev;
 
+	__skb_queue_head_init(&list);
+
 	if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
 	    (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
-		if (rmnet_map_process_next_hdr_packet(skb, len))
+		if (rmnet_map_process_next_hdr_packet(skb, &list, len))
 			goto free_skb;
-		skb_pull(skb, sizeof(*map_header));
-		rmnet_set_skb_proto(skb);
 	} else {
 		/* Subtract MAP header */
 		skb_pull(skb, sizeof(*map_header));
@@ -96,10 +107,11 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 		if (data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4 &&
 		    !rmnet_map_checksum_downlink_packet(skb, len + pad))
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
+		skb_trim(skb, len);
+		__skb_queue_tail(&list, skb);
 	}
 
-	skb_trim(skb, len);
-	rmnet_deliver_skb(skb);
+	rmnet_deliver_skb_list(&list);
 	return;
 
 free_skb:
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index 0977e495f591..ef738ce015d4 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -53,7 +53,9 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,
 				      struct rmnet_port *port,
 				      struct net_device *orig_dev,
 				      int csum_type);
-int rmnet_map_process_next_hdr_packet(struct sk_buff *skb, u16 len);
+int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
+				      struct sk_buff_head *list,
+				      u16 len);
 unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port,
 				    struct net_device *orig_dev);
 void rmnet_map_tx_aggregate_init(struct rmnet_port *port);
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 39d6d084e73f..577f2758e385 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -497,35 +497,54 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,
 	}
 }
 
-/* Process a MAPv5 packet header */
+static struct rmnet_map_v5_csum_header *
+rmnet_map_get_next_hdr(struct sk_buff *skb)
+{
+	return (struct rmnet_map_v5_csum_header *)(skb->data +
+						   sizeof(struct rmnet_map_header));
+}
+
+static u8 rmnet_map_get_next_hdr_type(struct sk_buff *skb)
+{
+	struct rmnet_map_v5_csum_header *hdr = rmnet_map_get_next_hdr(skb);
+
+	return u8_get_bits(hdr->header_info, MAPV5_HDRINFO_HDR_TYPE_FMASK);
+}
+
+static bool rmnet_map_get_csum_valid(struct sk_buff *skb)
+{
+	struct rmnet_map_v5_csum_header *hdr = rmnet_map_get_next_hdr(skb);
+
+	return !!(hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG);
+}
+
 int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
+				      struct sk_buff_head *list,
 				      u16 len)
 {
 	struct rmnet_priv *priv = netdev_priv(skb->dev);
-	struct rmnet_map_v5_csum_header *next_hdr;
-	u8 nexthdr_type;
 
-	next_hdr = (struct rmnet_map_v5_csum_header *)(skb->data +
-			sizeof(struct rmnet_map_header));
+	switch (rmnet_map_get_next_hdr_type(skb)) {
+	case RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD:
+		if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) {
+			priv->stats.csum_sw++;
+		} else if (rmnet_map_get_csum_valid(skb)) {
+			priv->stats.csum_ok++;
+			skb->ip_summed = CHECKSUM_UNNECESSARY;
+		} else {
+			priv->stats.csum_valid_unset++;
+		}
 
-	nexthdr_type = u8_get_bits(next_hdr->header_info,
-				   MAPV5_HDRINFO_HDR_TYPE_FMASK);
+		skb_pull(skb, sizeof(struct rmnet_map_header) +
+			      sizeof(struct rmnet_map_v5_csum_header));
+		skb_trim(skb, len);
+		__skb_queue_tail(list, skb);
+		break;
 
-	if (nexthdr_type != RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
+	default:
 		return -EINVAL;
-
-	if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) {
-		priv->stats.csum_sw++;
-	} else if (next_hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG) {
-		priv->stats.csum_ok++;
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
-	} else {
-		priv->stats.csum_valid_unset++;
 	}
 
-	/* Pull csum v5 header */
-	skb_pull(skb, sizeof(*next_hdr));
-
 	return 0;
 }
 
-- 
2.34.1

[PATCH net-next v3 4/8] net: qualcomm: rmnet: Restrict supported MAP checksum configurations

[Subash Abhinov Kasiviswanathan]

RmNet only supports three MAP data format configurations: MAPv1 (no
checksum offload), MAPv4 (v4 checksum offload) and MAPv5 (v5 checksum
offload). MAP command support is orthogonal and may be combined with any
of the three. Checksum offload versions are selected independently for
ingress and egress, but only one version may be enabled across the device.
Mixing the v4 and v5 checksum offload flags together, whether on the same
direction or across directions, is not a valid configuration. A direction
may leave checksum offload disabled.

Validate the requested data format in both rmnet_newlink() and
rmnet_changelink() and reject any combination that sets both the v4 and
v5 checksum offload flags at the same time, regardless of direction. This
is in preparation for the next patch where coalescing support needs to be
allowed with MAPv5 format only.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
v3: no change
v2: https://lore.kernel.org/all/20261008005543.2630828-5-subash.a.kasiviswanathan@oss.qualcomm.com/
v1: https://lore.kernel.org/all/20260930051345.857443-4-subash.a.kasiviswanathan@oss.qualcomm.com/

 .../ethernet/qualcomm/rmnet/rmnet_config.c    | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index e5a6289b018a..b68f5561680f 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -26,6 +26,23 @@ static int rmnet_is_real_dev_registered(const struct net_device *real_dev)
 	return rcu_access_pointer(real_dev->rx_handler) == rmnet_rx_handler;
 }
 
+/* Only three MAP configurations are supported: MAPv1 (no checksum
+ * offload), MAPv4 (v4 checksum offload) and MAPv5 (v5 checksum
+ * offload). MAP command support is orthogonal and permitted with any
+ * of the three. Checksum offload versions are selected independently
+ * for ingress and egress, but only one version may be enabled across
+ * the device. Mixing v4 and v5 checksum flags is not supported.
+ */
+static bool rmnet_config_data_format_valid(u32 data_format)
+{
+	u32 v4_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV4 |
+		      RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
+	u32 v5_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV5 |
+		      RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
+
+	return !(data_format & v4_mask) || !(data_format & v5_mask);
+}
+
 /* Needs rtnl lock */
 struct rmnet_port*
 rmnet_get_port_rtnl(const struct net_device *real_dev)
@@ -151,6 +168,12 @@ static int rmnet_newlink(struct net_device *dev,
 		data_format |= flags->flags & flags->mask;
 	}
 
+	if (!rmnet_config_data_format_valid(data_format)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "unsupported MAP checksum flag combination");
+		return -EINVAL;
+	}
+
 	ep = kzalloc_obj(*ep);
 	if (!ep)
 		return -ENOMEM;
@@ -337,6 +360,12 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
 		flags = nla_data(data[IFLA_RMNET_FLAGS]);
 		data_format = old_data_format & ~flags->mask;
 		data_format |= flags->flags & flags->mask;
+
+		if (!rmnet_config_data_format_valid(data_format)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "unsupported MAP checksum flag combination");
+			return -EINVAL;
+		}
 	}
 
 	if (data[IFLA_RMNET_MUX_ID]) {
-- 
2.34.1

Re: [PATCH net-next v3 4/8] net: qualcomm: rmnet: Restrict supported MAP checksum configurations

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message presents the new check in
  rmnet_config_data_format_valid()…

--

net: qualcomm: rmnet: Restrict supported MAP checksum configurations

This commit adds rmnet_config_data_format_valid(). It is called from
rmnet_newlink() and rmnet_changelink() to reject any data format that sets
both a v4 and a v5 MAP checksum offload flag, in either direction. The
commit message presents this as preparation for a following coalescing
patch.

> Validate the requested data format in both rmnet_newlink() and
> rmnet_changelink() and reject any combination that sets both the v4 and
> v5 checksum offload flags at the same time, regardless of direction. This
> is in preparation for the next patch where coalescing support needs to be
> allowed with MAPv5 format only.

[Severity: Low]
Is this only preparation, or does it also fix a regression in the current
code? Before this patch, IFLA_RMNET_FLAGS could set both the v4 and v5
checksum flags through rmnet_newlink() or rmnet_changelink(). The datapath
does not handle that state consistently.

On egress, with both RMNET_FLAGS_EGRESS_MAP_CKSUMV4 and
RMNET_FLAGS_EGRESS_MAP_CKSUMV5 set, rmnet_map_egress_handler() picks v4
first:

drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:rmnet_map_egress_handler() {
    ...
	if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4) {
		additional_header_len = sizeof(struct rmnet_map_ul_csum_header);
		csum_type = RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
	} else if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5) {
    ...
}

However, rmnet_map_add_map_header() still sets the next header flag:

drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:rmnet_map_add_map_header() {
    ...
	if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5)
		map_header->flags |= MAP_NEXT_HEADER_FLAG;
    ...
}

The MAP header then says a v5 next header follows, but a v4 UL checksum
header is what actually follows. Would the modem receive malformed frames
in that case?

On ingress, with both ingress flags set, rmnet_map_validate_packet_len()
checks v4 first and adds the v4 DL trailer length.
__rmnet_map_ingress_handler() checks v5 first:

drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:__rmnet_map_ingress_handler() {
    ...
	if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
	    (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
    ...
}

So the length check and the parser disagree on how the frame is laid out.

This patch closes off those states. Should the commit message describe it
as a fix and include a Fixes: tag, so stable maintainers can consider a
backport?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009084351.317632-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next v3 5/8] net: qualcomm: rmnet: Add DL packet coalescing support

[Subash Abhinov Kasiviswanathan]

Hardware can coalesce multiple IP packets of a flow into a single MAP
frame carrying a MAPv5 coalescing header and a set of Number Length
Objects (NLOs) that describe the count and length of the individual
segments within. The MAP coalescing header encapsulates a single IP
packet of type IPv4 and IPv6 for protocols TCP and UDP. Up to 48 IP
packets can be coalesced into a single coalescing packet.

This means that hardware can coalesce up to 6 different sizes within a
single IP packet. Additionally, hardware can continue to coalesce
packets even though it may have detected an issue with the checksum. The
checksum bitmap will give information on the exact segments which
happened to encounter a bad checksum. In these cases, the packets need
to be segmented out from the whole IP packet and only the packets with
same size need to be queued to the stack for valid checksum packets
while invalid checksum packets are dropped in the driver.

Coalesced frames containing multiple packets are accepted only when
both RXCSUM and GRO_HW are enabled. Such packets are either queued
through the GRO fast path or segmented in the driver. The resulting SKBs
carry CHECKSUM_PARTIAL and SKBs representing multiple packets also carry
the appropriate GSO metadata.

GRO_HW is advertised as an optional hardware feature and can be enabled
through the normal netdev feature controls. A single-packet frame is
passed with CHECKSUM_UNNECESSARY when both features are enabled and with
CHECKSUM_NONE when either feature is disabled. Multi-packet coalesced
frames are dropped when either RXCSUM or GRO_HW is disabled.

If a packet is segmented, the IP length and checksum are also updated
accordingly.

DL packet coalescing relies on the MAPv5 checksum header being present
on ingress, so reject any configuration which requests coalescing without
also enabling MAPv5 ingress checksum offload.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
v3: no change
v2: https://lore.kernel.org/all/20261008005543.2630828-6-subash.a.kasiviswanathan@oss.qualcomm.com/
v1: https://lore.kernel.org/all/20260930051345.857443-5-subash.a.kasiviswanathan@oss.qualcomm.com/

 .../ethernet/qualcomm/rmnet/rmnet_config.c    |  12 +-
 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  |   5 +-
 .../net/ethernet/qualcomm/rmnet/rmnet_map.h   |  18 +-
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 560 +++++++++++++++++-
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   |   1 +
 include/linux/if_rmnet.h                      |  31 +
 6 files changed, 611 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index b68f5561680f..ec882ad371ab 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -31,7 +31,8 @@ static int rmnet_is_real_dev_registered(const struct net_device *real_dev)
  * offload). MAP command support is orthogonal and permitted with any
  * of the three. Checksum offload versions are selected independently
  * for ingress and egress, but only one version may be enabled across
- * the device. Mixing v4 and v5 checksum flags is not supported.
+ * the device. Mixing v4 and v5 checksum flags is not supported. DL
+ * packet coalescing additionally requires a MAPv5 configuration.
  */
 static bool rmnet_config_data_format_valid(u32 data_format)
 {
@@ -40,7 +41,14 @@ static bool rmnet_config_data_format_valid(u32 data_format)
 	u32 v5_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV5 |
 		      RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
 
-	return !(data_format & v4_mask) || !(data_format & v5_mask);
+	if ((data_format & v4_mask) && (data_format & v5_mask))
+		return false;
+
+	if ((data_format & RMNET_FLAGS_INGRESS_COALESCE) &&
+	    !(data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5))
+		return false;
+
+	return true;
 }
 
 /* Needs rtnl lock */
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index ab1dfbd833e3..bb867ceb01c1 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -96,9 +96,10 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 
 	__skb_queue_head_init(&list);
 
-	if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
+	if ((data_format &
+	     (RMNET_FLAGS_INGRESS_MAP_CKSUMV5 | RMNET_FLAGS_INGRESS_COALESCE)) &&
 	    (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
-		if (rmnet_map_process_next_hdr_packet(skb, &list, len))
+		if (rmnet_map_process_next_hdr_packet(skb, &list, len, data_format))
 			goto free_skb;
 	} else {
 		/* Subtract MAP header */
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index ef738ce015d4..77ddac6023c0 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -6,6 +6,22 @@
 #define _RMNET_MAP_H_
 #include <linux/if_rmnet.h>
 
+enum rmnet_map_v5_close_type {
+	RMNET_MAP_COAL_CLOSE_NON_COAL,
+	RMNET_MAP_COAL_CLOSE_IP_MISS,
+	RMNET_MAP_COAL_CLOSE_TRANS_MISS,
+	RMNET_MAP_COAL_CLOSE_HW,
+	RMNET_MAP_COAL_CLOSE_COAL,
+};
+
+enum rmnet_map_v5_close_value {
+	RMNET_MAP_COAL_CLOSE_HW_NL,
+	RMNET_MAP_COAL_CLOSE_HW_PKT,
+	RMNET_MAP_COAL_CLOSE_HW_BYTE,
+	RMNET_MAP_COAL_CLOSE_HW_TIME,
+	RMNET_MAP_COAL_CLOSE_HW_EVICT,
+};
+
 struct rmnet_map_control_command {
 	u8  command_name;
 	u8  cmd_type:2;
@@ -55,7 +71,7 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,
 				      int csum_type);
 int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
 				      struct sk_buff_head *list,
-				      u16 len);
+				      u16 len, u32 data_format);
 unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port,
 				    struct net_device *orig_dev);
 void rmnet_map_tx_aggregate_init(struct rmnet_port *port);
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 577f2758e385..5ffb811d7ef1 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -7,7 +7,11 @@
 #include <linux/netdevice.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
+#include <net/ip.h>
 #include <net/ip6_checksum.h>
+#include <net/ipv6.h>
 #include <linux/bitfield.h>
 #include "rmnet_config.h"
 #include "rmnet_map.h"
@@ -17,6 +21,19 @@
 #define RMNET_MAP_DEAGGR_SPACING  64
 #define RMNET_MAP_DEAGGR_HEADROOM (RMNET_MAP_DEAGGR_SPACING / 2)
 
+struct rmnet_map_coal_metadata {
+	void *ip_header;
+	void *trans_header;
+	u16 ip_len;
+	u16 trans_len;
+	u16 data_offset;
+	u16 data_len;
+	u8 ip_proto;
+	u8 trans_proto;
+	u8 pkt_count;
+	bool zero_csum;
+};
+
 static __sum16 *rmnet_map_get_csum_field(unsigned char protocol,
 					 const void *txporthdr)
 {
@@ -337,8 +354,8 @@ u32 rmnet_map_validate_packet_len(struct sk_buff *skb, u32 data_format)
 {
 	struct rmnet_map_v5_csum_header *next_hdr = NULL;
 	struct rmnet_map_header *maph;
-	void *data = skb->data;
 	u32 packet_len;
+	u8 hdr_type;
 
 	if (skb->len < sizeof(*maph))
 		return 0;
@@ -353,24 +370,28 @@ u32 rmnet_map_validate_packet_len(struct sk_buff *skb, u32 data_format)
 
 	if (data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) {
 		packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
-	} else if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
+	} else if ((data_format &
+		    (RMNET_FLAGS_INGRESS_MAP_CKSUMV5 | RMNET_FLAGS_INGRESS_COALESCE)) &&
 		   !(maph->flags & MAP_CMD_FLAG)) {
-		/* Mapv5 data pkt without csum hdr is invalid */
 		if (!(maph->flags & MAP_NEXT_HEADER_FLAG))
 			return 0;
 
-		packet_len += sizeof(*next_hdr);
-		next_hdr = data + sizeof(*maph);
+		if (skb->len < sizeof(*maph) + sizeof(*next_hdr))
+			return 0;
+
+		next_hdr = (struct rmnet_map_v5_csum_header *)(skb->data + sizeof(*maph));
+		hdr_type = u8_get_bits(next_hdr->header_info,
+				       MAPV5_HDRINFO_HDR_TYPE_FMASK);
+
+		if (hdr_type == RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
+			packet_len += sizeof(*next_hdr);
+		else if (hdr_type != RMNET_MAP_HEADER_TYPE_COALESCING)
+			return 0;
 	}
 
 	if (skb->len < packet_len)
 		return 0;
 
-	if (next_hdr &&
-	    u8_get_bits(next_hdr->header_info, MAPV5_HDRINFO_HDR_TYPE_FMASK) !=
-	    RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
-		return 0;
-
 	return packet_len;
 }
 
@@ -518,13 +539,530 @@ static bool rmnet_map_get_csum_valid(struct sk_buff *skb)
 	return !!(hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG);
 }
 
+/* Stamp GSO metadata so the network stack can segment a coalesced SKB. */
+static void rmnet_map_gso_stamp(struct sk_buff *skb,
+				struct rmnet_map_coal_metadata *coal_meta)
+{
+	struct skb_shared_info *shinfo = skb_shinfo(skb);
+
+	if (coal_meta->trans_proto == IPPROTO_TCP)
+		shinfo->gso_type = (coal_meta->ip_proto == 4) ?
+				   SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
+	else
+		shinfo->gso_type = SKB_GSO_UDP_L4;
+
+	shinfo->gso_size = coal_meta->data_len;
+	shinfo->gso_segs = coal_meta->pkt_count;
+}
+
+/* Set the transport checksum to the pseudo-header checksum and request
+ * partial checksum offload, letting the NIC or stack finish it.
+ */
+static void rmnet_map_partial_csum(struct sk_buff *skb,
+				   struct rmnet_map_coal_metadata *coal_meta)
+{
+	u16 pkt_len = skb->len - coal_meta->ip_len;
+	unsigned char *data = skb->data;
+	__sum16 pseudo;
+
+	if (coal_meta->ip_proto == 4) {
+		struct iphdr *iph = (struct iphdr *)data;
+
+		pseudo = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
+					    pkt_len, coal_meta->trans_proto, 0);
+	} else {
+		struct ipv6hdr *ip6h = (struct ipv6hdr *)data;
+
+		pseudo = ~csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr,
+					  pkt_len, coal_meta->trans_proto, 0);
+	}
+
+	if (coal_meta->trans_proto == IPPROTO_TCP) {
+		struct tcphdr *tp = (struct tcphdr *)(data + coal_meta->ip_len);
+
+		tp->check = pseudo;
+		skb->csum_offset = offsetof(struct tcphdr, check);
+	} else {
+		struct udphdr *up = (struct udphdr *)(data + coal_meta->ip_len);
+
+		up->check = pseudo;
+		skb->csum_offset = offsetof(struct udphdr, check);
+	}
+
+	skb->ip_summed = CHECKSUM_PARTIAL;
+	skb->csum_start = skb->data + coal_meta->ip_len - skb->head;
+}
+
+/* Carve one logical segment from a coalesced SKB and append it to the list.
+ * Adjusts TCP sequence numbers, IP IDs/lengths, and checksum state.
+ */
+static void
+__rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
+			     struct rmnet_map_coal_metadata *coal_meta,
+			     struct sk_buff_head *list, u8 pkt_id,
+			     bool csum_valid)
+{
+	u32 dlen = coal_meta->data_len * coal_meta->pkt_count;
+	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
+	struct sk_buff *skbn;
+
+	/* RFC 768: UDP checksum is optional for IPv4, and is 0 if unused.
+	 * Such packets are never actually bad, regardless of what the
+	 * checksum bitmap says.
+	 */
+	if (!csum_valid && coal_meta->zero_csum)
+		csum_valid = true;
+
+	if (!csum_valid)
+		goto next_pkt;
+
+	skbn = alloc_skb(hlen + dlen + RMNET_MAP_DEAGGR_HEADROOM, GFP_ATOMIC);
+	if (!skbn)
+		goto next_pkt;
+
+	skb_reserve(skbn, hlen + RMNET_MAP_DEAGGR_HEADROOM);
+	skb_put_data(skbn,
+		     coal_skb->data + coal_meta->ip_len + coal_meta->trans_len +
+		     coal_meta->data_offset,
+		     dlen);
+
+	/* Restore transport header */
+	skb_push(skbn, coal_meta->trans_len);
+	memcpy(skbn->data, coal_meta->trans_header, coal_meta->trans_len);
+	skb_reset_transport_header(skbn);
+
+	if (coal_meta->trans_proto == IPPROTO_TCP) {
+		struct tcphdr *th = tcp_hdr(skbn);
+
+		th->seq = htonl(ntohl(th->seq) + coal_meta->data_offset);
+		/* Strip dangerous flags from non-final segments */
+		if ((th->fin || th->psh) &&
+		    hlen + coal_meta->data_offset + dlen < coal_skb->len) {
+			th->fin = 0;
+			th->psh = 0;
+		}
+	} else if (coal_meta->trans_proto == IPPROTO_UDP) {
+		struct udphdr *uh = udp_hdr(skbn);
+
+		uh->len = htons(skbn->len);
+	}
+
+	/* Restore IP header */
+	skb_push(skbn, coal_meta->ip_len);
+	memcpy(skbn->data, coal_meta->ip_header, coal_meta->ip_len);
+	skb_reset_network_header(skbn);
+
+	if (coal_meta->ip_proto == 4) {
+		struct iphdr *iph = ip_hdr(skbn);
+
+		iph->id = htons(ntohs(iph->id) + pkt_id);
+		iph->tot_len = htons(skbn->len);
+		iph->check = 0;
+		iph->check = ip_fast_csum(iph, iph->ihl);
+	} else {
+		ipv6_hdr(skbn)->payload_len =
+			htons(skbn->len - sizeof(struct ipv6hdr));
+	}
+
+	rmnet_map_partial_csum(skbn, coal_meta);
+
+	skbn->dev = coal_skb->dev;
+
+	if (coal_meta->pkt_count > 1)
+		rmnet_map_gso_stamp(skbn, coal_meta);
+
+	__skb_queue_tail(list, skbn);
+
+next_pkt:
+	coal_meta->data_offset += dlen;
+	coal_meta->pkt_count = 0;
+}
+
+/* Parse the IP header of the coalesced frame and perform basic
+ * validation of header fields.
+ */
+static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,
+					struct rmnet_map_coal_metadata *meta,
+					bool *gro)
+{
+	struct ipv6hdr *ip6h;
+	struct iphdr *iph;
+	__be16 frag_off;
+	u8 protocol;
+	int ret;
+
+	if (coal_skb->len < sizeof(*iph))
+		return false;
+
+	iph = (struct iphdr *)coal_skb->data;
+
+	if (iph->version == 4) {
+		meta->ip_proto = 4;
+		meta->ip_len = iph->ihl * 4;
+		meta->trans_proto = iph->protocol;
+		meta->ip_header = iph;
+		if (meta->ip_len < sizeof(*iph) || coal_skb->len < meta->ip_len)
+			return false;
+
+		if (ip_is_fragment(iph))
+			return false;
+
+		if (iph->ihl != 5)
+			*gro = false;
+	} else if (iph->version == 6) {
+		if (coal_skb->len < sizeof(*ip6h))
+			return false;
+
+		ip6h = (struct ipv6hdr *)iph;
+		protocol = ip6h->nexthdr;
+		meta->ip_proto = 6;
+		ret = ipv6_skip_exthdr(coal_skb, sizeof(*ip6h), &protocol,
+				       &frag_off);
+		if (ret < 0 || frag_off || ret > U16_MAX)
+			return false;
+
+		meta->ip_len = (u16)ret;
+		meta->trans_proto = protocol;
+		meta->ip_header = ip6h;
+		if (coal_skb->len < meta->ip_len)
+			return false;
+
+		if (meta->ip_len > sizeof(*ip6h))
+			*gro = false;
+	} else {
+		return false;
+	}
+
+	return true;
+}
+
+/* Parse the transport header following the IP header into coal_meta. The
+ * available length is checked before any field is read.
+ */
+static bool rmnet_map_coal_parse_trans_hdr(struct sk_buff *coal_skb,
+					   struct rmnet_map_coal_metadata *meta)
+{
+	struct udphdr *uh;
+	struct tcphdr *th;
+	u32 avail;
+	u8 *base;
+
+	if (meta->ip_len > coal_skb->len)
+		return false;
+
+	base = (u8 *)meta->ip_header + meta->ip_len;
+	avail = coal_skb->len - meta->ip_len;
+
+	if (meta->trans_proto == IPPROTO_TCP) {
+		if (avail < sizeof(*th))
+			return false;
+
+		th = (struct tcphdr *)base;
+		meta->trans_len = th->doff * 4;
+		meta->trans_header = th;
+		if (meta->trans_len < sizeof(*th) || avail < meta->trans_len)
+			return false;
+	} else if (meta->trans_proto == IPPROTO_UDP) {
+		if (avail < sizeof(*uh))
+			return false;
+
+		uh = (struct udphdr *)base;
+		meta->trans_len = sizeof(*uh);
+		meta->trans_header = uh;
+		if (meta->ip_proto == 4 && !uh->check)
+			meta->zero_csum = true;
+	} else {
+		return false;
+	}
+
+	return true;
+}
+
+/* Reject the frame unless the total data bytes claimed by all NLOs
+ * exactly match the actual payload in the SKB. Each pkt_len covers
+ * IP+transport headers plus per-packet data. Headers appear once, so
+ * subtract hlen per packet and track the running sum against available
+ * data. An exact match is required, not just an upper bound, so that
+ * GSO metadata stamped from the NLOs stays consistent with the real
+ * SKB length.
+ */
+static bool rmnet_map_coal_validate_bounds(struct sk_buff *coal_skb,
+					   struct rmnet_map_v5_coal_header *coal_hdr,
+					   u8 num_nlos, u32 hlen, bool *gro)
+{
+	u32 total_data = 0;
+	u32 nlo_len;
+	u16 plen;
+	u8 i;
+
+	if (hlen > coal_skb->len)
+		return false;
+
+	for (i = 0; i < num_nlos; i++) {
+		plen = ntohs(coal_hdr->nl_pairs[i].pkt_len);
+
+		if (plen < hlen)
+			return false;
+
+		/* Zero-payload packets cannot use GSO with a zero gso_size. */
+		if (plen == hlen)
+			*gro = false;
+
+		nlo_len = (u32)(plen - hlen) * coal_hdr->nl_pairs[i].num_packets;
+		if (nlo_len > coal_skb->len - hlen - total_data)
+			return false;
+
+		total_data += nlo_len;
+	}
+
+	return total_data == coal_skb->len - hlen;
+}
+
+/* Attempt the GRO-friendly fast path for a single-NLO, checksum-valid frame
+ * by reusing the original SKB and stamping GSO metadata instead of copying
+ * out each segment.  Returns true if the frame was consumed via the fast
+ * path, or false if the caller should fall back to full segmentation.
+ */
+static bool rmnet_map_coal_gro_fast_path(struct sk_buff *coal_skb,
+					 struct rmnet_map_v5_coal_header *coal_hdr,
+					 struct rmnet_map_coal_metadata *coal_meta,
+					 struct sk_buff_head *list,
+					 u8 num_nlos, bool gro)
+{
+	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
+
+	if (!gro || num_nlos != 1 ||
+	    !(coal_hdr->coal_info & MAPV5_COALINFO_CSUM_VALID_FLAG))
+		return false;
+
+	coal_meta->data_len = ntohs(coal_hdr->nl_pairs[0].pkt_len) - hlen;
+	coal_meta->pkt_count = coal_hdr->nl_pairs[0].num_packets;
+
+	coal_skb->ip_summed = CHECKSUM_UNNECESSARY;
+	if (coal_meta->pkt_count > 1) {
+		rmnet_map_partial_csum(coal_skb, coal_meta);
+		rmnet_map_gso_stamp(coal_skb, coal_meta);
+	}
+
+	__skb_queue_tail(list, coal_skb);
+	return true;
+}
+
+/* NLO packet lengths are already bounds-checked by
+ * rmnet_map_coal_validate_bounds() so no further validation is needed here.
+ */
+static void rmnet_map_coal_segment_loop(struct sk_buff *coal_skb,
+					struct rmnet_map_v5_coal_header *coal_hdr,
+					struct rmnet_map_coal_metadata *coal_meta,
+					struct sk_buff_head *list,
+					u64 nlo_err_mask, bool gro, u8 num_nlos)
+{
+	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
+	u8 pkt, total_pkt = 0;
+	bool csum_err;
+	u16 pkt_len;
+	u8 nlo;
+
+	for (nlo = 0; nlo < num_nlos; nlo++) {
+		pkt_len = ntohs(coal_hdr->nl_pairs[nlo].pkt_len);
+		pkt_len -= hlen;
+		coal_meta->data_len = pkt_len;
+
+		/* nlo_err_mask is one flat bitstream across all NLOs. Shift
+		 * it once per packet in absolute frame order and do not
+		 * re-align at the NLO boundary above. See the comment on
+		 * rmnet_map_data_check_coal_header() for why.
+		 */
+		for (pkt = 0; pkt < coal_hdr->nl_pairs[nlo].num_packets;
+		     pkt++, total_pkt++, nlo_err_mask >>= 1) {
+			csum_err = nlo_err_mask & 1;
+
+			if (!gro) {
+				coal_meta->pkt_count = 1;
+				__rmnet_map_segment_coal_skb(coal_skb, coal_meta,
+							     list, total_pkt,
+							     !csum_err);
+				continue;
+			}
+
+			if (csum_err) {
+				if (coal_meta->pkt_count)
+					__rmnet_map_segment_coal_skb(coal_skb,
+								     coal_meta,
+								     list,
+								     total_pkt -
+								     coal_meta->pkt_count,
+								     true);
+				coal_meta->pkt_count = 1;
+				__rmnet_map_segment_coal_skb(coal_skb, coal_meta,
+							     list, total_pkt,
+							     false);
+			} else {
+				coal_meta->pkt_count++;
+			}
+		}
+
+		/* Flush remaining packets from this NLO */
+		if (coal_meta->pkt_count)
+			__rmnet_map_segment_coal_skb(coal_skb, coal_meta, list,
+						     total_pkt - coal_meta->pkt_count, true);
+	}
+}
+
+/* Expand a coalesced SKB into individual IP packets placed on the list.
+ * NLOs with checksum errors are dropped. __rmnet_map_ingress_handler will
+ * free the SKB in the error case.
+ */
+static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
+				      u64 nlo_err_mask,
+				      struct sk_buff_head *list,
+				      u16 len, u16 total_pkts)
+{
+	bool gro_hw = coal_skb->dev->features & NETIF_F_GRO_HW;
+	bool rxcsum = coal_skb->dev->features & NETIF_F_RXCSUM;
+	struct rmnet_map_v5_coal_header *coal_hdr;
+	struct rmnet_map_coal_metadata coal_meta;
+	bool gro = gro_hw;
+	u8 num_nlos;
+	u32 hlen;
+
+	memset(&coal_meta, 0, sizeof(coal_meta));
+
+	/* Drop any MAP frame padding. The coal header is counted in len */
+	skb_pull(coal_skb, sizeof(struct rmnet_map_header));
+	skb_trim(coal_skb, len);
+	coal_hdr = (struct rmnet_map_v5_coal_header *)coal_skb->data;
+	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
+	skb_pull(coal_skb, sizeof(*coal_hdr));
+
+	if (!rmnet_map_coal_parse_ip_hdr(coal_skb, &coal_meta, &gro))
+		return -EINVAL;
+
+	if (!rmnet_map_coal_parse_trans_hdr(coal_skb, &coal_meta))
+		return -EINVAL;
+
+	hlen = coal_meta.ip_len + coal_meta.trans_len;
+
+	if (!rmnet_map_coal_validate_bounds(coal_skb, coal_hdr, num_nlos, hlen,
+					    &gro))
+		return -EINVAL;
+
+	/* Device capability gates coalesced delivery. Packet format can still
+	 * disable GSO and use the per-packet fallback below.
+	 */
+	if (total_pkts > 1 && (!rxcsum || !gro_hw))
+		return -EINVAL;
+
+	if (total_pkts == 1 && (!rxcsum || !gro_hw)) {
+		coal_skb->ip_summed = CHECKSUM_NONE;
+		__skb_queue_tail(list, coal_skb);
+		return 0;
+	}
+
+	if (rmnet_map_coal_gro_fast_path(coal_skb, coal_hdr, &coal_meta, list,
+					 num_nlos, gro))
+		return 0;
+
+	rmnet_map_coal_segment_loop(coal_skb, coal_hdr, &coal_meta, list,
+				    nlo_err_mask, gro, num_nlos);
+
+	return 0;
+}
+
+/* Validate the coalescing header and build the checksum error mask.
+ *
+ * Checks performed:
+ *  - MAP pkt_len accommodates the coal header (coal header is counted in
+ *    pkt_len. pkt_len < sizeof(*coal_hdr) means no payload is possible).
+ *  - num_nlos is in [1, RMNET_MAP_V5_MAX_NLOS].
+ *  - Total packet count does not exceed RMNET_MAP_V5_MAX_PACKETS.
+ *
+ * nlo_err_mask is NOT six independent per-NLO bitmaps. Each nl_pairs
+ * slot only has room for an 8 bit csum_error_bitmap, but a single NLO
+ * can carry more than 8 packets (up to RMNET_MAP_V5_MAX_PACKETS), so
+ * hardware spills a wide NLO's error bits into the csum_error_bitmap
+ * bytes of the following slots rather than truncating them. The
+ * six bitmap bytes are therefore always concatenated in slot order
+ * into one flat RMNET_MAP_V5_MAX_NLOS * 8 = RMNET_MAP_V5_MAX_PACKETS
+ * bit value, addressed by a packet's absolute position in the frame,
+ * regardless of how many NLOs are actually in use. Receivers must
+ * walk it as a single contiguous stream and must not expect it to
+ * align it at NLO boundaries.
+ */
+static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
+					    u64 *nlo_err_mask,
+					    u16 *num_pkts)
+{
+	struct rmnet_map_header *maph = (struct rmnet_map_header *)skb->data;
+	struct rmnet_map_v5_coal_header *coal_hdr;
+	u8 num_nlos;
+	u16 pkts = 0;
+	u64 mask = 0;
+	int i;
+
+	/* coal header is counted in pkt_len */
+	if (ntohs(maph->pkt_len) < sizeof(*coal_hdr))
+		return -EINVAL;
+
+	coal_hdr = (struct rmnet_map_v5_coal_header *)(skb->data + sizeof(*maph));
+	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
+
+	if (num_nlos == 0 || num_nlos > RMNET_MAP_V5_MAX_NLOS)
+		return -EINVAL;
+
+	for (i = 0; i < RMNET_MAP_V5_MAX_NLOS; i++) {
+		u8 err = coal_hdr->nl_pairs[i].csum_error_bitmap;
+		u8 pkt = coal_hdr->nl_pairs[i].num_packets;
+
+		mask |= ((u64)err) << (8 * i);
+		if (i < num_nlos) {
+			pkts += pkt;
+			if (pkts > RMNET_MAP_V5_MAX_PACKETS)
+				return -EINVAL;
+		}
+	}
+
+	*nlo_err_mask = mask;
+	*num_pkts = pkts;
+	return 0;
+}
+
 int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
 				      struct sk_buff_head *list,
-				      u16 len)
+				      u16 len, u32 data_format)
 {
 	struct rmnet_priv *priv = netdev_priv(skb->dev);
+	u64 nlo_err_mask;
+	u16 num_pkts;
+	int rc;
 
 	switch (rmnet_map_get_next_hdr_type(skb)) {
+	case RMNET_MAP_HEADER_TYPE_COALESCING:
+		if (!(data_format & RMNET_FLAGS_INGRESS_COALESCE))
+			return -EINVAL;
+
+		rc = rmnet_map_data_check_coal_header(skb, &nlo_err_mask,
+						      &num_pkts);
+		if (rc)
+			return rc;
+
+		/* Coalescing rewrites shared packet data and GSO metadata. */
+		if (skb_unclone(skb, GFP_ATOMIC))
+			return -ENOMEM;
+
+		rc = rmnet_map_segment_coal_skb(skb, nlo_err_mask, list, len,
+						num_pkts);
+		if (rc)
+			return rc;
+
+		if (skb_peek(list) != skb) {
+			if (skb_queue_empty(list))
+				kfree_skb(skb);
+			else
+				consume_skb(skb);
+		}
+		break;
+
 	case RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD:
 		if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) {
 			priv->stats.csum_sw++;
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index d23f74b0aa47..b8542d2f03b2 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -331,6 +331,7 @@ int rmnet_vnd_newlink(u8 id, struct net_device *rmnet_dev,
 	rmnet_dev->hw_features = NETIF_F_RXCSUM;
 	rmnet_dev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	rmnet_dev->hw_features |= NETIF_F_SG;
+	rmnet_dev->hw_features |= NETIF_F_GRO_HW;
 
 	priv->real_dev = real_dev;
 
diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
index c44bf6e80ecb..5df4d417f3d2 100644
--- a/include/linux/if_rmnet.h
+++ b/include/linux/if_rmnet.h
@@ -70,5 +70,36 @@ struct rmnet_map_v5_csum_header {
 #define MAPV5_HDRINFO_HDR_TYPE_FMASK	GENMASK(7, 1)
 #define MAPV5_CSUMINFO_VALID_FLAG	BIT(7)
 
+#define RMNET_MAP_HEADER_TYPE_COALESCING   1
 #define RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD 2
+
+/* MAPv5 coalescing header */
+#define RMNET_MAP_V5_MAX_NLOS		6
+#define RMNET_MAP_V5_MAX_PACKETS	48
+
+/* One Number-Length Object pair: per-NLO packet count and length. */
+struct rmnet_map_v5_nl_pair {
+	__be16 pkt_len;
+	u8  csum_error_bitmap;
+	u8  num_packets;
+} __aligned(1);
+
+/* MAPv5 coalescing header: describes up to RMNET_MAP_V5_MAX_NLOS NLOs.
+ * The header immediately follows the MAP header in the frame and is
+ * included in the MAP pkt_len field.
+ */
+struct rmnet_map_v5_coal_header {
+	u8  header_info;	/* MAPV5_HDRINFO_NXT_HDR_FLAG, MAPV5_HDRINFO_HDR_TYPE_FMASK */
+	u8  coal_info;		/* MAPV5_COALINFO_* */
+	u8  close_info;		/* MAPV5_CLOSEINFO_* */
+	u8  veid_info;		/* MAPV5_VEIDINFO_* */
+	struct rmnet_map_v5_nl_pair nl_pairs[RMNET_MAP_V5_MAX_NLOS];
+} __aligned(1);
+
+#define MAPV5_COALINFO_NUM_NLOS_FMASK		GENMASK(6, 4)
+#define MAPV5_COALINFO_CSUM_VALID_FLAG		BIT(7)
+#define MAPV5_CLOSEINFO_CLOSE_TYPE_FMASK	GENMASK(3, 0)
+#define MAPV5_CLOSEINFO_CLOSE_VALUE_FMASK	GENMASK(7, 4)
+#define MAPV5_VEIDINFO_VEID_FMASK		GENMASK(3, 0)
+
 #endif /* !(_LINUX_IF_RMNET_H_) */
-- 
2.34.1

Re: [PATCH net-next v3 5/8] net: qualcomm: rmnet: Add DL packet coalescing support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 4

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Clearing NETIF_F_GRO_HW (or NETIF_F_RXCSUM) on an rmnet device
  does not stop the modem from coalescing.
- [Medium] rmnet_map_coal_gro_fast_path() hands the original coalesced
  skb to the stack with gso_size/gso_segs stamped from the NLOs and the…
- [Medium] rmnet_vnd_rx_fixup() adds 1 to rx_pkts per delivered skb.
- [Medium] rmnet_map_gso_stamp() sets gso_type to only SKB_GSO_TCPV4 or
  SKB_GSO_TCPV6 and never adds SKB_GSO_TCP_ACCECN when the retained TCP…
- [Low] The commit message says 'A single-packet frame is passed with
  CHECKSUM_UNNECESSARY when both features are enabled'.
- [Low] Several pieces of text no longer match the code after this patch.
- [Low] __rmnet_map_segment_coal_skb() decides whether a segment is the
  last one (and so keeps FIN/PSH) by comparing byte offsets: `hlen +…
- [Low] rmnet_map_coal_parse_ip_hdr() validates the IPv4 version, ihl and
  fragment bits, but never verifies the original IPv4 header checksum.

--

net: qualcomm: rmnet: Add DL packet coalescing support

This commit adds support for MAPv5 downlink coalescing frames. In these
frames, hardware merges up to 48 packets of a flow into one MAP frame
described by up to 6 NLOs. Coalesced frames are delivered as GSO skbs
through a fast path, or segmented in the driver using a per-packet
checksum error bitmap. The commit also advertises NETIF_F_GRO_HW and
requires MAPv5 ingress checksum offload when coalescing is requested.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 577f2758e3855..5ffb811d7ef1a 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

[ ... ]

> @@ -518,13 +539,530 @@ static bool rmnet_map_get_csum_valid(struct sk_buff *skb)
>  	return !!(hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG);
>  }
>  
> +/* Stamp GSO metadata so the network stack can segment a coalesced SKB. */
> +static void rmnet_map_gso_stamp(struct sk_buff *skb,
> +				struct rmnet_map_coal_metadata *coal_meta)
> +{
> +	struct skb_shared_info *shinfo = skb_shinfo(skb);
> +
> +	if (coal_meta->trans_proto == IPPROTO_TCP)
> +		shinfo->gso_type = (coal_meta->ip_proto == 4) ?
> +				   SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
> +	else
> +		shinfo->gso_type = SKB_GSO_UDP_L4;

[Severity: Medium]
Should SKB_GSO_TCP_ACCECN be set here when the retained TCP header has CWR
set? tcp_gro_complete() sets it for software aggregates:

net/ipv4/tcp_offload.c:tcp_gro_complete() {
    ...
	if (th->cwr)
		shinfo->gso_type |= SKB_GSO_TCP_ACCECN;
}

When the flag is absent, tcp_gso_segment() clears CWR on every segment
after the first:

	ecn_cwr_mask = !!(skb_shinfo(gso_skb)->gso_type & SKB_GSO_TCP_ACCECN);
	...
		th->cwr &= ecn_cwr_mask;

Suppose the hardware coalesces CWR-bearing AccECN packets and the
aggregate is then forwarded (for example, tethering) and segmented in
software. Segments 2..N would lose CWR, which changes the ACE feedback
they carry.

Frames with PSH or FIN are flushed through GRO_NORMAL without
tcp_gro_complete(), so software GRO wouldn't restore the flag either.

[ ... ]

> +static void
> +__rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
> +			     struct rmnet_map_coal_metadata *coal_meta,
> +			     struct sk_buff_head *list, u8 pkt_id,
> +			     bool csum_valid)
> +{

[ ... ]

> +	if (!csum_valid)
> +		goto next_pkt;
> +
> +	skbn = alloc_skb(hlen + dlen + RMNET_MAP_DEAGGR_HEADROOM, GFP_ATOMIC);
> +	if (!skbn)
> +		goto next_pkt;

[Severity: Medium]
Should these drops be counted? Packets dropped for a set bitmap bit, or
because alloc_skb() failed, are freed here without updating any counter.

The uAPI description of rx_packets in struct rtnl_link_stats64 includes
packets the host had to drop in the driver.

rmnet_vnd_rx_fixup() also still adds one per delivered skb:

	pcpu_ptr->stats.rx_pkts++;
	pcpu_ptr->stats.rx_bytes += skb->len;

With rmnet_map_gso_stamp(), one skb can now stand for up to 48 wire
packets (gso_segs). Won't rx_packets undercount on the coalesced path?

The later patch "net: qualcomm: rmnet: Add DL coalescing statistics" adds
rx_dropped and rx_alloc_fail accounting for the checksum drop and the
allocation failure. It counts those in packets, while rx_packets counts
skbs.

Even at the end of the series, a GSO skb still counts as one in
rx_packets. The rejection in rmnet_map_segment_coal_skb() when features
are off still has no counter.

> +
> +	skb_reserve(skbn, hlen + RMNET_MAP_DEAGGR_HEADROOM);
> +	skb_put_data(skbn,
> +		     coal_skb->data + coal_meta->ip_len + coal_meta->trans_len +
> +		     coal_meta->data_offset,
> +		     dlen);
> +
> +	/* Restore transport header */
> +	skb_push(skbn, coal_meta->trans_len);
> +	memcpy(skbn->data, coal_meta->trans_header, coal_meta->trans_len);
> +	skb_reset_transport_header(skbn);
> +
> +	if (coal_meta->trans_proto == IPPROTO_TCP) {
> +		struct tcphdr *th = tcp_hdr(skbn);
> +
> +		th->seq = htonl(ntohl(th->seq) + coal_meta->data_offset);
> +		/* Strip dangerous flags from non-final segments */
> +		if ((th->fin || th->psh) &&
> +		    hlen + coal_meta->data_offset + dlen < coal_skb->len) {
> +			th->fin = 0;
> +			th->psh = 0;
> +		}

[Severity: Low]
This test finds the final segment by comparing byte offsets. Can it pick
the wrong segment when the frame ends with zero-payload packets?

rmnet_map_coal_validate_bounds() accepts zero-payload NLOs. When
plen == hlen, it only clears gro. In that case the last packet that
carries data already reaches data_offset + dlen == total payload. It is
treated as final and keeps FIN/PSH, even though more packets follow.

For example, take NLO0 with 2 x 100 bytes, NLO1 with 1 x 0 bytes, and
FIN set in the shared header. Packet 2 of NLO0 keeps FIN, and the
trailing zero-length packet carries FIN again at the same sequence
number.

Would comparing the packet index against the total packet count find the
last packet more reliably?

> +	} else if (coal_meta->trans_proto == IPPROTO_UDP) {
> +		struct udphdr *uh = udp_hdr(skbn);
> +
> +		uh->len = htons(skbn->len);
> +	}
> +
> +	/* Restore IP header */
> +	skb_push(skbn, coal_meta->ip_len);
> +	memcpy(skbn->data, coal_meta->ip_header, coal_meta->ip_len);
> +	skb_reset_network_header(skbn);
> +
> +	if (coal_meta->ip_proto == 4) {
> +		struct iphdr *iph = ip_hdr(skbn);
> +
> +		iph->id = htons(ntohs(iph->id) + pkt_id);
> +		iph->tot_len = htons(skbn->len);
> +		iph->check = 0;
> +		iph->check = ip_fast_csum(iph, iph->ihl);

[Severity: Low]
rmnet_map_coal_parse_ip_hdr() checks the version, ihl and fragment bits.
It never verifies the checksum of the original IPv4 header.

Here that header is copied into each new skb and its checksum is
recomputed. A corrupted IPv4 header would therefore arrive with a valid
checksum, and ip_rcv_core() could no longer drop it. Is that intended?

The fast path and the existing CSUM_OFFLOAD path pass the original header
to ip_rcv(). The MAPv4 path checks ip_fast_csum() explicitly in
rmnet_map_ipv4_dl_csum_trailer().

This segmentation path handles multi-NLO frames, any per-packet checksum
error, IPv4 options and IPv6 extension headers.

Should ip_fast_csum() be checked on the original header before it is
copied?

[ ... ]

> +static bool rmnet_map_coal_gro_fast_path(struct sk_buff *coal_skb,
> +					 struct rmnet_map_v5_coal_header *coal_hdr,
> +					 struct rmnet_map_coal_metadata *coal_meta,
> +					 struct sk_buff_head *list,
> +					 u8 num_nlos, bool gro)
> +{
> +	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
> +
> +	if (!gro || num_nlos != 1 ||
> +	    !(coal_hdr->coal_info & MAPV5_COALINFO_CSUM_VALID_FLAG))
> +		return false;

[Severity: Low]
The commit message says:

  A single-packet frame is passed with CHECKSUM_UNNECESSARY when both
  features are enabled and with CHECKSUM_NONE when either feature is
  disabled.

Is that accurate? CHECKSUM_UNNECESSARY is only set in this function. That
also requires gro, num_nlos == 1 and MAPV5_COALINFO_CSUM_VALID_FLAG.

gro is cleared for IPv4 options (ihl != 5), IPv6 extension headers and
zero-payload NLOs.

In all other cases, a single-packet frame goes through
rmnet_map_coal_segment_loop()->__rmnet_map_segment_coal_skb(). There it
is either copied into a new skb with CHECKSUM_PARTIAL by
rmnet_map_partial_csum(), or dropped if its bitmap bit is set.

A later patch in the series adds a CHECKSUM_NONE quirk. The final
rmnet.rst only says single-packet frames become "normal non-GSO skbs".

Could the commit message for this patch describe these cases?

> +
> +	coal_meta->data_len = ntohs(coal_hdr->nl_pairs[0].pkt_len) - hlen;
> +	coal_meta->pkt_count = coal_hdr->nl_pairs[0].num_packets;
> +
> +	coal_skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	if (coal_meta->pkt_count > 1) {
> +		rmnet_map_partial_csum(coal_skb, coal_meta);
> +		rmnet_map_gso_stamp(coal_skb, coal_meta);
> +	}
> +
> +	__skb_queue_tail(list, coal_skb);
> +	return true;
> +}

[Severity: Medium]
This hands the original coalesced skb to the stack. gso_size and
gso_segs come from the NLOs. The L4 checksum is seeded from the aggregate
length in rmnet_map_partial_csum():

	u16 pkt_len = skb->len - coal_meta->ip_len;

Nothing checks the retained header against coal_skb->len: not the IPv4
tot_len or header checksum, the IPv6 payload_len, or the UDP len.
rmnet_map_coal_validate_bounds() only checks the NLO totals against
skb->len.

Is the hardware guaranteed to write aggregate lengths, and a matching
IPv4 header checksum, into this header? What happens if those fields
hold per-segment values?

ip_rcv_core(), ipv6_rcv() and __udp4_lib_rcv() would trim the skb to the
header length. The remaining segments would be lost while gso_segs still
says N.

__udp_gso_segment() would compute wrong per-segment checksums, because
it subtracts uh->len from a seed built with the aggregate length:

	check = csum16_add(csum16_sub(uh->check, uh->len), htons(newlen));

A stale IPv4 header checksum would make ip_rcv() drop the frame.

inet_gro_complete() and udp_gro_complete() normalise these fields for
software GRO. Would it make sense to check them here and fall back to
segmentation when they don't match?

[ ... ]

> +/* Expand a coalesced SKB into individual IP packets placed on the list.
> + * NLOs with checksum errors are dropped. __rmnet_map_ingress_handler will
> + * free the SKB in the error case.
> + */

[Severity: Low]
This isn't a bug, but this comment says whole NLOs are dropped.
rmnet_map_coal_segment_loop() actually drops individual packets by their
position in the flat bitmap, as the comment above
rmnet_map_data_check_coal_header() describes.

A few other pieces of text look stale after this patch.

The MAP_NEXT_HEADER_FLAG comment in include/linux/if_rmnet.h still says
the following, although header type 1 is now the coalescing header:

 *  NEXT_HEADER: 1 = packet contains V5 CSUM header 0 = no V5 CSUM header

rmnet.rst still lists MAPv5 header type 1 as "Reserved" and says the next
header is "limited to checksum header". The later patch "docs:
networking: Add documentation for the coalescing support in rmnet" fixes
this one.

If RMNET_FLAGS_INGRESS_COALESCE is set without
RMNET_FLAGS_INGRESS_MAP_CKSUMV5, rmnet_newlink() and rmnet_changelink()
report "unsupported MAP checksum flag combination". That message doesn't
mention the coalescing dependency.

The comment here, the if_rmnet.h comment and the extack text are
unchanged at the end of the series.

> +static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
> +				      u64 nlo_err_mask,
> +				      struct sk_buff_head *list,
> +				      u16 len, u16 total_pkts)
> +{
> +	bool gro_hw = coal_skb->dev->features & NETIF_F_GRO_HW;
> +	bool rxcsum = coal_skb->dev->features & NETIF_F_RXCSUM;

[ ... ]

> +	/* Device capability gates coalesced delivery. Packet format can still
> +	 * disable GSO and use the per-packet fallback below.
> +	 */
> +	if (total_pkts > 1 && (!rxcsum || !gro_hw))
> +		return -EINVAL;

[Severity: Medium]
Clearing NETIF_F_GRO_HW or NETIF_F_RXCSUM on the rmnet device doesn't
stop the modem from coalescing. rmnet has no ndo_set_features, so the
change never reaches the hardware.

Every multi-packet frame then returns -EINVAL here. Can this make
__rmnet_map_ingress_handler() free frames that each carry up to 48 valid
packets?

Documentation/networking/netdev-features.rst describes rx-gro-hw as a
request for the NIC to enable hardware GRO, not as a filter on received
traffic.

The core also clears GRO_HW on its own:

generic_xdp_install()->dev_disable_gro_hw()

net/core/dev.c:netdev_fix_features() {
    ...
		if (features & NETIF_F_GRO_HW) {
			netdev_dbg(dev, "Dropping NETIF_F_GRO_HW since no RXCSUM feature.\n");
			features &= ~NETIF_F_GRO_HW;
		}
}

So attaching an xdpgeneric program, or running "ethtool -K <dev> rx off",
would silently drop all multi-packet coalesced traffic.

rmnet_vnd_newlink() adds NETIF_F_GRO_HW and NETIF_F_RXCSUM only to
hw_features, never to the device's enabled features, so both start
disabled. Won't a port created with RMNET_FLAGS_INGRESS_COALESCE drop
multi-packet frames until the user also enables both features with
ethtool?

The gro == false path in rmnet_map_coal_segment_loop() already delivers
packets one at a time. Could that be used here instead of dropping the
frame?

The documentation patch at the end of the series only records this
behaviour ("Otherwise, multi-packet coalesced frames are rejected"), and
the drop still has no counter.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009084351.317632-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next v3 6/8] net: qualcomm: rmnet: Work around coalescing hardware quirks

[Subash Abhinov Kasiviswanathan]

On certain hardware, the frame-level CSUM_VALID indication in a MAPv5
coalescing frame cannot be trusted when the frame contains exactly one
packet and hardware completes coalescing due to a TCP FIN or PSH flag, a
packet count limit, a byte count limit or a time limit. The GRO path could
otherwise mark the packet CHECKSUM_UNNECESSARY even when its checksum is
wrong.

Force these frames to CHECKSUM_NONE before either GRO processing or
bitmap-based segmentation. This lets the network stack verify the checksum.
Packets marked by the checksum error bitmap are therefore not dropped
directly by rmnet for these frames.

The single NLO, single packet determination this fix depends on cannot be
based on the num_nlos field declared in the coalescing header, since on
certain simulation hardware configurations that field can be reported
incorrectly even though the per-NLO num_packets fields are accurate.
Derive the effective NLO count from the contiguous num_packets prefix and
stop processing at the first empty entry. This prevents stale values in
unused slots from changing checksum fixup or segmentation behavior.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
v3: no change
v2: https://lore.kernel.org/all/20261008005543.2630828-7-subash.a.kasiviswanathan@oss.qualcomm.com/
v1: https://lore.kernel.org/all/20260930051345.857443-6-subash.a.kasiviswanathan@oss.qualcomm.com/ 

 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 90 ++++++++++++++++---
 1 file changed, 80 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 5ffb811d7ef1..e8adb4006717 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -593,6 +593,68 @@ static void rmnet_map_partial_csum(struct sk_buff *skb,
 	skb->csum_start = skb->data + coal_meta->ip_len - skb->head;
 }
 
+/* On some hardware, num_nlos in the coalescing header can be reported
+ * incorrectly under certain conditions even though the per-NLO num_packets
+ * fields it is meant to summarize are correct. Recompute the NLO count from
+ * the contiguous nl_pairs[] prefix rather than trusting the declared value.
+ * The first empty NLO marks the end of the prefix. Entries after it are not
+ * processed.
+ */
+static int rmnet_map_v5_get_num_nlos(const struct rmnet_map_v5_coal_header *coal_hdr)
+{
+	int nlos;
+
+	for (nlos = 0; nlos < RMNET_MAP_V5_MAX_NLOS; nlos++)
+		if (!coal_hdr->nl_pairs[nlos].num_packets)
+			break;
+
+	if (!nlos)
+		return -EINVAL;
+
+	return nlos;
+}
+
+static void rmnet_map_v5_set_nlos(struct rmnet_map_v5_coal_header *coal_hdr,
+				  u8 nlos)
+{
+	coal_hdr->coal_info = u8_encode_bits(nlos, MAPV5_COALINFO_NUM_NLOS_FMASK) |
+				      (coal_hdr->coal_info & MAPV5_COALINFO_CSUM_VALID_FLAG);
+}
+
+/* The frame-level CSUM_VALID indication for a single NLO, single packet
+ * coalescing frame cannot be trusted when the close reason is a TCP FIN/PSH,
+ * a packet count limit, a byte count limit or a time limit.
+ */
+static bool rmnet_map_v5_csum_fixup(struct rmnet_map_v5_coal_header *coal_hdr)
+{
+	u8 close_value = u8_get_bits(coal_hdr->close_info,
+				     MAPV5_CLOSEINFO_CLOSE_VALUE_FMASK);
+	u8 close_type = u8_get_bits(coal_hdr->close_info,
+				    MAPV5_CLOSEINFO_CLOSE_TYPE_FMASK);
+	u8 num_nlos = u8_get_bits(coal_hdr->coal_info,
+				  MAPV5_COALINFO_NUM_NLOS_FMASK);
+
+	/* Only applies to single NLO, single packet frames */
+	if (num_nlos != 1 || coal_hdr->nl_pairs[0].num_packets != 1)
+		return false;
+
+	/* TCP FIN or PSH triggered the close */
+	if (close_type == RMNET_MAP_COAL_CLOSE_COAL)
+		return true;
+
+	/* Hit a hardware limit */
+	if (close_type == RMNET_MAP_COAL_CLOSE_HW) {
+		switch (close_value) {
+		case RMNET_MAP_COAL_CLOSE_HW_PKT:
+		case RMNET_MAP_COAL_CLOSE_HW_BYTE:
+		case RMNET_MAP_COAL_CLOSE_HW_TIME:
+			return true;
+		}
+	}
+
+	return false;
+}
+
 /* Carve one logical segment from a coalesced SKB and append it to the list.
  * Adjusts TCP sequence numbers, IP IDs/lengths, and checksum state.
  */
@@ -916,14 +978,13 @@ static void rmnet_map_coal_segment_loop(struct sk_buff *coal_skb,
 static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 				      u64 nlo_err_mask,
 				      struct sk_buff_head *list,
-				      u16 len, u16 total_pkts)
+				      u16 len, u16 total_pkts, u8 num_nlos)
 {
 	bool gro_hw = coal_skb->dev->features & NETIF_F_GRO_HW;
 	bool rxcsum = coal_skb->dev->features & NETIF_F_RXCSUM;
 	struct rmnet_map_v5_coal_header *coal_hdr;
 	struct rmnet_map_coal_metadata coal_meta;
 	bool gro = gro_hw;
-	u8 num_nlos;
 	u32 hlen;
 
 	memset(&coal_meta, 0, sizeof(coal_meta));
@@ -932,7 +993,7 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	skb_pull(coal_skb, sizeof(struct rmnet_map_header));
 	skb_trim(coal_skb, len);
 	coal_hdr = (struct rmnet_map_v5_coal_header *)coal_skb->data;
-	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
+	rmnet_map_v5_set_nlos(coal_hdr, num_nlos);
 	skb_pull(coal_skb, sizeof(*coal_hdr));
 
 	if (!rmnet_map_coal_parse_ip_hdr(coal_skb, &coal_meta, &gro))
@@ -959,6 +1020,12 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 		return 0;
 	}
 
+	if (rmnet_map_v5_csum_fixup(coal_hdr) && !coal_meta.zero_csum) {
+		coal_skb->ip_summed = CHECKSUM_NONE;
+		__skb_queue_tail(list, coal_skb);
+		return 0;
+	}
+
 	if (rmnet_map_coal_gro_fast_path(coal_skb, coal_hdr, &coal_meta, list,
 					 num_nlos, gro))
 		return 0;
@@ -991,13 +1058,14 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
  */
 static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
 					    u64 *nlo_err_mask,
+					    u8 *num_nlos,
 					    u16 *num_pkts)
 {
 	struct rmnet_map_header *maph = (struct rmnet_map_header *)skb->data;
 	struct rmnet_map_v5_coal_header *coal_hdr;
-	u8 num_nlos;
 	u16 pkts = 0;
 	u64 mask = 0;
+	int nlos;
 	int i;
 
 	/* coal header is counted in pkt_len */
@@ -1005,17 +1073,18 @@ static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
 		return -EINVAL;
 
 	coal_hdr = (struct rmnet_map_v5_coal_header *)(skb->data + sizeof(*maph));
-	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
-
-	if (num_nlos == 0 || num_nlos > RMNET_MAP_V5_MAX_NLOS)
+	nlos = rmnet_map_v5_get_num_nlos(coal_hdr);
+	if (nlos < 0)
 		return -EINVAL;
 
+	*num_nlos = nlos;
+
 	for (i = 0; i < RMNET_MAP_V5_MAX_NLOS; i++) {
 		u8 err = coal_hdr->nl_pairs[i].csum_error_bitmap;
 		u8 pkt = coal_hdr->nl_pairs[i].num_packets;
 
 		mask |= ((u64)err) << (8 * i);
-		if (i < num_nlos) {
+		if (i < *num_nlos) {
 			pkts += pkt;
 			if (pkts > RMNET_MAP_V5_MAX_PACKETS)
 				return -EINVAL;
@@ -1034,6 +1103,7 @@ int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
 	struct rmnet_priv *priv = netdev_priv(skb->dev);
 	u64 nlo_err_mask;
 	u16 num_pkts;
+	u8 num_nlos;
 	int rc;
 
 	switch (rmnet_map_get_next_hdr_type(skb)) {
@@ -1042,7 +1112,7 @@ int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
 			return -EINVAL;
 
 		rc = rmnet_map_data_check_coal_header(skb, &nlo_err_mask,
-						      &num_pkts);
+						      &num_nlos, &num_pkts);
 		if (rc)
 			return rc;
 
@@ -1051,7 +1121,7 @@ int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
 			return -ENOMEM;
 
 		rc = rmnet_map_segment_coal_skb(skb, nlo_err_mask, list, len,
-						num_pkts);
+						num_pkts, num_nlos);
 		if (rc)
 			return rc;
 
-- 
2.34.1

Re: [PATCH net-next v3 6/8] net: qualcomm: rmnet: Work around coalescing hardware quirks

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch fixes a checksum-trust defect added by the previous
  patch in the same series, "net: qualcomm: rmnet: Add DL packet…
- [Low] Several comments in
  drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c no longer match…

--

net: qualcomm: rmnet: Work around coalescing hardware quirks

On some hardware the frame-level CSUM_VALID flag can't be trusted for a
MAPv5 coalescing frame that holds one NLO with one packet. This applies
when the frame was closed by a TCP FIN/PSH or by the packet count, byte
count or time limit. This patch forces those frames to CHECKSUM_NONE. It
also derives the NLO count from the contiguous num_packets prefix rather
than the num_nlos field in the coalescing header.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 5ffb811d7ef1a..e8adb40067175 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

[ ... ]

> @@ -959,6 +1020,12 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
>  		return 0;
>  	}
>  
> +	if (rmnet_map_v5_csum_fixup(coal_hdr) && !coal_meta.zero_csum) {
> +		coal_skb->ip_summed = CHECKSUM_NONE;
> +		__skb_queue_tail(list, coal_skb);
> +		return 0;
> +	}
> +
>  	if (rmnet_map_coal_gro_fast_path(coal_skb, coal_hdr, &coal_meta, list,
>  					 num_nlos, gro))
>  		return 0;

[Severity: Low]
This workaround fixes behaviour added by the previous patch in this
series, "net: qualcomm: rmnet: Add DL packet coalescing support". Would it
be better to fold the quirk handling into that patch?

At that commit, rmnet_map_coal_gro_fast_path() trusts the frame-level
CSUM_VALID flag and never checks close_info:

rmnet_map_coal_gro_fast_path() {
	if (!gro || num_nlos != 1 ||
	    !(coal_hdr->coal_info & MAPV5_COALINFO_CSUM_VALID_FLAG))
		return false;
	...
	coal_skb->ip_summed = CHECKSUM_UNNECESSARY;
	...
}

Assume RXCSUM and GRO_HW are both enabled, and a frame with one NLO and
one packet was closed by FIN/PSH or by the HW packet, byte or time limit.
That frame goes through this path:

rmnet_map_process_next_hdr_packet()
  -> rmnet_map_segment_coal_skb()
    -> rmnet_map_coal_gro_fast_path()

There it is marked CHECKSUM_UNNECESSARY even if its checksum is wrong. The
commit message of that patch also says "A single-packet frame is passed
with CHECKSUM_UNNECESSARY when both features are enabled".

This can only happen with the new opt-in RMNET_FLAGS_INGRESS_COALESCE
flag, and the problem is gone by the end of the series. Even so, a bisect
that stops on the coalescing patch, or a backport that takes only that
patch, would accept corrupted payloads on the affected hardware. Both
patches are in the same unmerged series, so a Fixes: tag isn't the right
tool here.

[ ... ]

> @@ -1005,17 +1073,18 @@ static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
>  		return -EINVAL;
>  
>  	coal_hdr = (struct rmnet_map_v5_coal_header *)(skb->data + sizeof(*maph));
> -	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
> -
> -	if (num_nlos == 0 || num_nlos > RMNET_MAP_V5_MAX_NLOS)
> +	nlos = rmnet_map_v5_get_num_nlos(coal_hdr);
> +	if (nlos < 0)
>  		return -EINVAL;
>  
> +	*num_nlos = nlos;
> +
>  	for (i = 0; i < RMNET_MAP_V5_MAX_NLOS; i++) {
>  		u8 err = coal_hdr->nl_pairs[i].csum_error_bitmap;
>  		u8 pkt = coal_hdr->nl_pairs[i].num_packets;
>  
>  		mask |= ((u64)err) << (8 * i);

[Severity: Low]
After this change, several comments in rmnet_map_data.c no longer match
the code. They are still there at the end of the series.

The "Checks performed" list above rmnet_map_data_check_coal_header() still
says:

 *  - num_nlos is in [1, RMNET_MAP_V5_MAX_NLOS].

This function no longer reads or range-checks the NUM_NLOS field in the
header, so a value of 0 or 7 there is now accepted. The only check left is
that the derived prefix count is non-zero.

The new comment on rmnet_map_v5_get_num_nlos() says:

 * The first empty NLO marks the end of the prefix. Entries after it are not
 * processed.

The commit message also says "stop processing at the first empty entry".
But this loop still ORs csum_error_bitmap from all RMNET_MAP_V5_MAX_NLOS
slots into nlo_err_mask. The comment on this function requires that
"regardless of how many NLOs are actually in use". The documentation
added in this series also says bitmap bytes after the active prefix must
not be ignored.

The comment above rmnet_map_segment_coal_skb() also still says:

 * NLOs with checksum errors are dropped. __rmnet_map_ingress_handler will

The new rmnet_map_v5_csum_fixup() branch queues the whole coal_skb with
CHECKSUM_NONE. It returns before rmnet_map_coal_segment_loop(), so
packets flagged in csum_error_bitmap are delivered rather than dropped.
The commit message notes this.

Could these comments be updated to match the new behaviour?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009084351.317632-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next v3 7/8] net: qualcomm: rmnet: Add DL coalescing statistics

[Subash Abhinov Kasiviswanathan]

Track downlink coalescing activity with standard network stack RX
statistics and rmnet specific ethtool diagnostics.

Expose these standard RX statistics through netdev_stat_ops:

- HW-GRO packets and the number of wire packets represented by them
- RX allocation failures
- RX drops for reconstructed packets discarded by rmnet

Keep these rmnet specific ethtool statistics for diagnostics that have no
standard representation:

- Coalescing header, packet count and bounds errors
- Invalid IP and transport headers
- Segment reconstruction
- Hardware close reasons

Store the per-CPU counters as u64_stats_t values and update them with
u64_stats_inc() and u64_stats_add() under u64_stats_sync protection. Copy
them into plain snapshot structures with u64_stats_copy() before
aggregation. This avoids torn counter reads during reporting.

Checksum error diagnostics distinguish hardware reported errors from
packets actually dropped. IPv4 UDP packets with a zero checksum are
accepted and are not counted as drops.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
v3: 
  - Fix build warning due to fortify detecting memcpy() read beyond the
    declared ethtool_stats field boundary in struct rmnet_priv_stats as
    reported by netdev build bot.
  - Fix build warning due to rmnet_get_ethtool_stats() exceeding the
    configured 1280 byte stack frame limit due to the statistics
    snapshot as reported by kernel test robot.
v2: https://lore.kernel.org/all/20261008005543.2630828-8-subash.a.kasiviswanathan@oss.qualcomm.com/
v1: https://lore.kernel.org/all/20260930051345.857443-7-subash.a.kasiviswanathan@oss.qualcomm.com/

 .../ethernet/qualcomm/rmnet/rmnet_config.h    | 104 +++++-
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 338 ++++++++++++++++--
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   | 130 ++++++-
 3 files changed, 516 insertions(+), 56 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
index 5adda0323dda..917d31f6eded 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
@@ -63,31 +63,105 @@ struct rmnet_vnd_stats {
 	u32 tx_drops;
 };
 
+struct rmnet_priv_stats {
+	/* Network stack RX statistics for all rmnet receive paths */
+	u64 rx_hw_gro_packets;
+	u64 rx_hw_gro_wire_packets;
+	u64 rx_alloc_fail;
+	u64 rx_dropped;
+
+	/* Custom diagnostics for the coalescing receive path */
+	struct_group(ethtool_stats, /* ethtool statistics */
+		u64 csum_ok;
+		u64 csum_ip4_header_bad;
+		u64 csum_valid_unset;
+		u64 csum_validation_failed;
+		u64 csum_err_bad_buffer;
+		u64 csum_err_invalid_ip_version;
+		u64 csum_err_invalid_transport;
+		u64 csum_fragmented_pkt;
+		u64 csum_skipped;
+		u64 csum_sw;
+		u64 csum_hw;
+		/* Coalescing-only counters. Some are subsets of the standard counters. */
+		u64 coal_alloc_fail;
+		u64 coal_rx;
+		u64 coal_pkts;
+		u64 coal_hdr_err;
+		u64 coal_hdr_pkt_err;
+		u64 coal_bounds_err;
+		u64 coal_csum_err;
+		u64 coal_csum_drop;
+		u64 coal_reconstruct;
+		u64 coal_ip_invalid;
+		u64 coal_trans_invalid;
+		/* close-reason sub-counters */
+		u64 coal_close_non_coal;
+		u64 coal_close_ip_miss;
+		u64 coal_close_trans_miss;
+		u64 coal_close_hw_nl;
+		u64 coal_close_hw_pkt;
+		u64 coal_close_hw_byte;
+		u64 coal_close_hw_time;
+		u64 coal_close_hw_evict;
+		u64 coal_close_coal;
+	);
+};
+
+struct rmnet_pcpu_priv_stats {
+	/* Network stack RX statistics for all rmnet receive paths */
+	u64_stats_t rx_hw_gro_packets;
+	u64_stats_t rx_hw_gro_wire_packets;
+	u64_stats_t rx_alloc_fail;
+	u64_stats_t rx_dropped;
+
+	/* Custom diagnostics for the coalescing receive path */
+	u64_stats_t csum_ok;
+	u64_stats_t csum_ip4_header_bad;
+	u64_stats_t csum_valid_unset;
+	u64_stats_t csum_validation_failed;
+	u64_stats_t csum_err_bad_buffer;
+	u64_stats_t csum_err_invalid_ip_version;
+	u64_stats_t csum_err_invalid_transport;
+	u64_stats_t csum_fragmented_pkt;
+	u64_stats_t csum_skipped;
+	u64_stats_t csum_sw;
+	u64_stats_t csum_hw;
+	/* Coalescing-only counters. Some are subsets of the standard counters. */
+	u64_stats_t coal_alloc_fail;
+	u64_stats_t coal_rx;
+	u64_stats_t coal_pkts;
+	u64_stats_t coal_hdr_err;
+	u64_stats_t coal_hdr_pkt_err;
+	u64_stats_t coal_bounds_err;
+	u64_stats_t coal_csum_err;
+	u64_stats_t coal_csum_drop;
+	u64_stats_t coal_reconstruct;
+	u64_stats_t coal_ip_invalid;
+	u64_stats_t coal_trans_invalid;
+	/* close-reason sub-counters */
+	u64_stats_t coal_close_non_coal;
+	u64_stats_t coal_close_ip_miss;
+	u64_stats_t coal_close_trans_miss;
+	u64_stats_t coal_close_hw_nl;
+	u64_stats_t coal_close_hw_pkt;
+	u64_stats_t coal_close_hw_byte;
+	u64_stats_t coal_close_hw_time;
+	u64_stats_t coal_close_hw_evict;
+	u64_stats_t coal_close_coal;
+};
+
 struct rmnet_pcpu_stats {
 	struct rmnet_vnd_stats stats;
+	struct rmnet_pcpu_priv_stats priv_stats;
 	struct u64_stats_sync syncp;
 };
 
-struct rmnet_priv_stats {
-	u64 csum_ok;
-	u64 csum_ip4_header_bad;
-	u64 csum_valid_unset;
-	u64 csum_validation_failed;
-	u64 csum_err_bad_buffer;
-	u64 csum_err_invalid_ip_version;
-	u64 csum_err_invalid_transport;
-	u64 csum_fragmented_pkt;
-	u64 csum_skipped;
-	u64 csum_sw;
-	u64 csum_hw;
-};
-
 struct rmnet_priv {
 	u8 mux_id;
 	struct net_device *real_dev;
 	struct rmnet_pcpu_stats __percpu *pcpu_stats;
 	struct gro_cells gro_cells;
-	struct rmnet_priv_stats stats;
 };
 
 struct rmnet_port *rmnet_get_port_rcu(const struct net_device *real_dev);
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index e8adb4006717..74d64b7066e2 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -53,34 +53,45 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 {
 	struct iphdr *ip4h = (struct iphdr *)skb->data;
 	void *txporthdr = skb->data + ip4h->ihl * 4;
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	__sum16 *csum_field, pseudo_csum;
 	__sum16 ip_payload_csum;
 
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
 	/* Computing the checksum over just the IPv4 header--including its
 	 * checksum field--should yield 0.  If it doesn't, the IP header
 	 * is bad, so return an error and let the IP layer drop it.
 	 */
 	if (ip_fast_csum(ip4h, ip4h->ihl)) {
-		priv->stats.csum_ip4_header_bad++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_ip4_header_bad);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EINVAL;
 	}
 
 	/* We don't support checksum offload on IPv4 fragments */
 	if (ip_is_fragment(ip4h)) {
-		priv->stats.csum_fragmented_pkt++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_fragmented_pkt);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EOPNOTSUPP;
 	}
 
 	/* Checksum offload is only supported for UDP and TCP protocols */
 	csum_field = rmnet_map_get_csum_field(ip4h->protocol, txporthdr);
 	if (!csum_field) {
-		priv->stats.csum_err_invalid_transport++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_err_invalid_transport);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EPROTONOSUPPORT;
 	}
 
 	/* RFC 768: UDP checksum is optional for IPv4, and is 0 if unused */
 	if (!*csum_field && ip4h->protocol == IPPROTO_UDP) {
-		priv->stats.csum_skipped++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_skipped);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return 0;
 	}
 
@@ -113,11 +124,15 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 
 	/* The cast is required to ensure only the low 16 bits are examined */
 	if (ip_payload_csum != (__sum16)~pseudo_csum) {
-		priv->stats.csum_validation_failed++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_validation_failed);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EINVAL;
 	}
 
-	priv->stats.csum_ok++;
+	u64_stats_update_begin(&pcpu_ptr->syncp);
+	u64_stats_inc(&pcpu_ptr->priv_stats.csum_ok);
+	u64_stats_update_end(&pcpu_ptr->syncp);
 	return 0;
 }
 
@@ -129,16 +144,21 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 {
 	struct ipv6hdr *ip6h = (struct ipv6hdr *)skb->data;
 	void *txporthdr = skb->data + sizeof(*ip6h);
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	__sum16 *csum_field, pseudo_csum;
 	__sum16 ip6_payload_csum;
 	__be16 ip_header_csum;
 
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
 	/* Checksum offload is only supported for UDP and TCP protocols;
 	 * the packet cannot include any IPv6 extension headers
 	 */
 	csum_field = rmnet_map_get_csum_field(ip6h->nexthdr, txporthdr);
 	if (!csum_field) {
-		priv->stats.csum_err_invalid_transport++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_err_invalid_transport);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EPROTONOSUPPORT;
 	}
 
@@ -164,11 +184,15 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	 * examined.
 	 */
 	if (ip6_payload_csum != (__sum16)~pseudo_csum) {
-		priv->stats.csum_validation_failed++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_validation_failed);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EINVAL;
 	}
 
-	priv->stats.csum_ok++;
+	u64_stats_update_begin(&pcpu_ptr->syncp);
+	u64_stats_inc(&pcpu_ptr->priv_stats.csum_ok);
+	u64_stats_update_end(&pcpu_ptr->syncp);
 	return 0;
 }
 #else
@@ -263,6 +287,9 @@ static void rmnet_map_v5_checksum_uplink_packet(struct sk_buff *skb,
 {
 	struct rmnet_priv *priv = netdev_priv(orig_dev);
 	struct rmnet_map_v5_csum_header *ul_header;
+	struct rmnet_pcpu_stats *pcpu_ptr;
+
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
 
 	ul_header = skb_push(skb, sizeof(*ul_header));
 	memset(ul_header, 0, sizeof(*ul_header));
@@ -287,7 +314,9 @@ static void rmnet_map_v5_checksum_uplink_packet(struct sk_buff *skb,
 			proto = ((struct ipv6hdr *)iph)->nexthdr;
 			trans = iph + ip_len;
 		} else {
-			priv->stats.csum_err_invalid_ip_version++;
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.csum_err_invalid_ip_version);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			goto sw_csum;
 		}
 
@@ -296,13 +325,17 @@ static void rmnet_map_v5_checksum_uplink_packet(struct sk_buff *skb,
 			skb->ip_summed = CHECKSUM_NONE;
 			/* Ask for checksum offloading */
 			ul_header->csum_info |= MAPV5_CSUMINFO_VALID_FLAG;
-			priv->stats.csum_hw++;
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.csum_hw);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			return;
 		}
 	}
 
 sw_csum:
-	priv->stats.csum_sw++;
+	u64_stats_update_begin(&pcpu_ptr->syncp);
+	u64_stats_inc(&pcpu_ptr->priv_stats.csum_sw);
+	u64_stats_update_end(&pcpu_ptr->syncp);
 }
 
 /* Adds MAP header to front of skb->data
@@ -434,16 +467,23 @@ int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len)
 {
 	struct rmnet_priv *priv = netdev_priv(skb->dev);
 	struct rmnet_map_dl_csum_trailer *csum_trailer;
+	struct rmnet_pcpu_stats *pcpu_ptr;
+
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
 
 	if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) {
-		priv->stats.csum_sw++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_sw);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EOPNOTSUPP;
 	}
 
 	csum_trailer = (struct rmnet_map_dl_csum_trailer *)(skb->data + len);
 
 	if (!(csum_trailer->flags & MAP_CSUM_DL_VALID_FLAG)) {
-		priv->stats.csum_valid_unset++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_valid_unset);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EINVAL;
 	}
 
@@ -453,7 +493,9 @@ int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len)
 	if (IS_ENABLED(CONFIG_IPV6) && skb->protocol == htons(ETH_P_IPV6))
 		return rmnet_map_ipv6_dl_csum_trailer(skb, csum_trailer, priv);
 
-	priv->stats.csum_err_invalid_ip_version++;
+	u64_stats_update_begin(&pcpu_ptr->syncp);
+	u64_stats_inc(&pcpu_ptr->priv_stats.csum_err_invalid_ip_version);
+	u64_stats_update_end(&pcpu_ptr->syncp);
 
 	return -EPROTONOSUPPORT;
 }
@@ -463,8 +505,11 @@ static void rmnet_map_v4_checksum_uplink_packet(struct sk_buff *skb,
 {
 	struct rmnet_priv *priv = netdev_priv(orig_dev);
 	struct rmnet_map_ul_csum_header *ul_header;
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	void *iphdr;
 
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
 	ul_header = (struct rmnet_map_ul_csum_header *)
 		    skb_push(skb, sizeof(struct rmnet_map_ul_csum_header));
 
@@ -480,22 +525,30 @@ static void rmnet_map_v4_checksum_uplink_packet(struct sk_buff *skb,
 
 	if (skb->protocol == htons(ETH_P_IP)) {
 		rmnet_map_ipv4_ul_csum_header(iphdr, ul_header, skb);
-		priv->stats.csum_hw++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_hw);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return;
 	}
 
 	if (IS_ENABLED(CONFIG_IPV6) && skb->protocol == htons(ETH_P_IPV6)) {
 		rmnet_map_ipv6_ul_csum_header(iphdr, ul_header, skb);
-		priv->stats.csum_hw++;
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.csum_hw);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return;
 	}
 
-	priv->stats.csum_err_invalid_ip_version++;
+	u64_stats_update_begin(&pcpu_ptr->syncp);
+	u64_stats_inc(&pcpu_ptr->priv_stats.csum_err_invalid_ip_version);
+	u64_stats_update_end(&pcpu_ptr->syncp);
 
 sw_csum:
 	memset(ul_header, 0, sizeof(*ul_header));
 
-	priv->stats.csum_sw++;
+	u64_stats_update_begin(&pcpu_ptr->syncp);
+	u64_stats_inc(&pcpu_ptr->priv_stats.csum_sw);
+	u64_stats_update_end(&pcpu_ptr->syncp);
 }
 
 /* Generates UL checksum meta info header for IPv4 and IPv6 over TCP and UDP
@@ -665,9 +718,13 @@ __rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 			     bool csum_valid)
 {
 	u32 dlen = coal_meta->data_len * coal_meta->pkt_count;
+	struct rmnet_priv *priv = netdev_priv(coal_skb->dev);
 	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	struct sk_buff *skbn;
 
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
 	/* RFC 768: UDP checksum is optional for IPv4, and is 0 if unused.
 	 * Such packets are never actually bad, regardless of what the
 	 * checksum bitmap says.
@@ -675,12 +732,29 @@ __rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	if (!csum_valid && coal_meta->zero_csum)
 		csum_valid = true;
 
-	if (!csum_valid)
+	if (!csum_valid) {
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		/* rx_dropped is standard accounting. coal_csum_drop is its
+		 * coalescing-specific checksum error subset.
+		 */
+		u64_stats_add(&pcpu_ptr->priv_stats.rx_dropped, coal_meta->pkt_count);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_csum_drop);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		goto next_pkt;
+	}
 
 	skbn = alloc_skb(hlen + dlen + RMNET_MAP_DEAGGR_HEADROOM, GFP_ATOMIC);
-	if (!skbn)
+	if (!skbn) {
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		/* The coal counter is the coalescing subset of the standard
+		 * allocation failure and RX drop counters.
+		 */
+		u64_stats_add(&pcpu_ptr->priv_stats.rx_alloc_fail, coal_meta->pkt_count);
+		u64_stats_add(&pcpu_ptr->priv_stats.rx_dropped, coal_meta->pkt_count);
+		u64_stats_add(&pcpu_ptr->priv_stats.coal_alloc_fail, coal_meta->pkt_count);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		goto next_pkt;
+	}
 
 	skb_reserve(skbn, hlen + RMNET_MAP_DEAGGR_HEADROOM);
 	skb_put_data(skbn,
@@ -729,6 +803,9 @@ __rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	rmnet_map_partial_csum(skbn, coal_meta);
 
 	skbn->dev = coal_skb->dev;
+	u64_stats_update_begin(&pcpu_ptr->syncp);
+	u64_stats_inc(&pcpu_ptr->priv_stats.coal_reconstruct);
+	u64_stats_update_end(&pcpu_ptr->syncp);
 
 	if (coal_meta->pkt_count > 1)
 		rmnet_map_gso_stamp(skbn, coal_meta);
@@ -747,14 +824,22 @@ static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,
 					struct rmnet_map_coal_metadata *meta,
 					bool *gro)
 {
+	struct rmnet_priv *priv = netdev_priv(coal_skb->dev);
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	struct ipv6hdr *ip6h;
 	struct iphdr *iph;
 	__be16 frag_off;
 	u8 protocol;
 	int ret;
 
-	if (coal_skb->len < sizeof(*iph))
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
+	if (coal_skb->len < sizeof(*iph)) {
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_ip_invalid);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return false;
+	}
 
 	iph = (struct iphdr *)coal_skb->data;
 
@@ -763,35 +848,58 @@ static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,
 		meta->ip_len = iph->ihl * 4;
 		meta->trans_proto = iph->protocol;
 		meta->ip_header = iph;
-		if (meta->ip_len < sizeof(*iph) || coal_skb->len < meta->ip_len)
+		if (meta->ip_len < sizeof(*iph) || coal_skb->len < meta->ip_len) {
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_ip_invalid);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			return false;
+		}
 
-		if (ip_is_fragment(iph))
+		if (ip_is_fragment(iph)) {
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_ip_invalid);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			return false;
+		}
 
 		if (iph->ihl != 5)
 			*gro = false;
 	} else if (iph->version == 6) {
-		if (coal_skb->len < sizeof(*ip6h))
+		if (coal_skb->len < sizeof(*ip6h)) {
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_ip_invalid);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			return false;
+		}
 
 		ip6h = (struct ipv6hdr *)iph;
 		protocol = ip6h->nexthdr;
 		meta->ip_proto = 6;
 		ret = ipv6_skip_exthdr(coal_skb, sizeof(*ip6h), &protocol,
 				       &frag_off);
-		if (ret < 0 || frag_off || ret > U16_MAX)
+		if (ret < 0 || frag_off || ret > U16_MAX) {
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_ip_invalid);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			return false;
+		}
 
 		meta->ip_len = (u16)ret;
 		meta->trans_proto = protocol;
 		meta->ip_header = ip6h;
-		if (coal_skb->len < meta->ip_len)
+		if (coal_skb->len < meta->ip_len) {
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_ip_invalid);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			return false;
+		}
 
 		if (meta->ip_len > sizeof(*ip6h))
 			*gro = false;
 	} else {
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_ip_invalid);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return false;
 	}
 
@@ -804,29 +912,49 @@ static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,
 static bool rmnet_map_coal_parse_trans_hdr(struct sk_buff *coal_skb,
 					   struct rmnet_map_coal_metadata *meta)
 {
+	struct rmnet_priv *priv = netdev_priv(coal_skb->dev);
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	struct udphdr *uh;
 	struct tcphdr *th;
 	u32 avail;
 	u8 *base;
 
-	if (meta->ip_len > coal_skb->len)
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
+	if (meta->ip_len > coal_skb->len) {
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_trans_invalid);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return false;
+	}
 
 	base = (u8 *)meta->ip_header + meta->ip_len;
 	avail = coal_skb->len - meta->ip_len;
 
 	if (meta->trans_proto == IPPROTO_TCP) {
-		if (avail < sizeof(*th))
+		if (avail < sizeof(*th)) {
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_trans_invalid);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			return false;
+		}
 
 		th = (struct tcphdr *)base;
 		meta->trans_len = th->doff * 4;
 		meta->trans_header = th;
-		if (meta->trans_len < sizeof(*th) || avail < meta->trans_len)
+		if (meta->trans_len < sizeof(*th) || avail < meta->trans_len) {
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_trans_invalid);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			return false;
+		}
 	} else if (meta->trans_proto == IPPROTO_UDP) {
-		if (avail < sizeof(*uh))
+		if (avail < sizeof(*uh)) {
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_trans_invalid);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			return false;
+		}
 
 		uh = (struct udphdr *)base;
 		meta->trans_len = sizeof(*uh);
@@ -834,6 +962,9 @@ static bool rmnet_map_coal_parse_trans_hdr(struct sk_buff *coal_skb,
 		if (meta->ip_proto == 4 && !uh->check)
 			meta->zero_csum = true;
 	} else {
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_trans_invalid);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return false;
 	}
 
@@ -919,12 +1050,16 @@ static void rmnet_map_coal_segment_loop(struct sk_buff *coal_skb,
 					struct sk_buff_head *list,
 					u64 nlo_err_mask, bool gro, u8 num_nlos)
 {
+	struct rmnet_priv *priv = netdev_priv(coal_skb->dev);
 	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	u8 pkt, total_pkt = 0;
 	bool csum_err;
 	u16 pkt_len;
 	u8 nlo;
 
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
 	for (nlo = 0; nlo < num_nlos; nlo++) {
 		pkt_len = ntohs(coal_hdr->nl_pairs[nlo].pkt_len);
 		pkt_len -= hlen;
@@ -939,6 +1074,12 @@ static void rmnet_map_coal_segment_loop(struct sk_buff *coal_skb,
 		     pkt++, total_pkt++, nlo_err_mask >>= 1) {
 			csum_err = nlo_err_mask & 1;
 
+			if (csum_err) {
+				u64_stats_update_begin(&pcpu_ptr->syncp);
+				u64_stats_inc(&pcpu_ptr->priv_stats.coal_csum_err);
+				u64_stats_update_end(&pcpu_ptr->syncp);
+			}
+
 			if (!gro) {
 				coal_meta->pkt_count = 1;
 				__rmnet_map_segment_coal_skb(coal_skb, coal_meta,
@@ -982,11 +1123,15 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 {
 	bool gro_hw = coal_skb->dev->features & NETIF_F_GRO_HW;
 	bool rxcsum = coal_skb->dev->features & NETIF_F_RXCSUM;
+	struct rmnet_priv *priv = netdev_priv(coal_skb->dev);
 	struct rmnet_map_v5_coal_header *coal_hdr;
 	struct rmnet_map_coal_metadata coal_meta;
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	bool gro = gro_hw;
 	u32 hlen;
 
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
 	memset(&coal_meta, 0, sizeof(coal_meta));
 
 	/* Drop any MAP frame padding. The coal header is counted in len */
@@ -1005,8 +1150,12 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	hlen = coal_meta.ip_len + coal_meta.trans_len;
 
 	if (!rmnet_map_coal_validate_bounds(coal_skb, coal_hdr, num_nlos, hlen,
-					    &gro))
+					    &gro)) {
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_bounds_err);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EINVAL;
+	}
 
 	/* Device capability gates coalesced delivery. Packet format can still
 	 * disable GSO and use the per-packet fallback below.
@@ -1014,6 +1163,16 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	if (total_pkts > 1 && (!rxcsum || !gro_hw))
 		return -EINVAL;
 
+	if (gro && total_pkts > 1) {
+		/* Standard counters cover delivered HW-GRO skbs. coal_rx and
+		 * coal_pkts cover coalescing input regardless of HW-GRO.
+		 */
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.rx_hw_gro_packets);
+		u64_stats_add(&pcpu_ptr->priv_stats.rx_hw_gro_wire_packets, total_pkts);
+		u64_stats_update_end(&pcpu_ptr->syncp);
+	}
+
 	if (total_pkts == 1 && (!rxcsum || !gro_hw)) {
 		coal_skb->ip_summed = CHECKSUM_NONE;
 		__skb_queue_tail(list, coal_skb);
@@ -1036,6 +1195,67 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	return 0;
 }
 
+/* Log the hardware close-reason counter for a coalescing header. */
+static void rmnet_map_data_log_close_stats(struct rmnet_pcpu_stats *pcpu_ptr,
+					   u8 type, u8 code)
+{
+	switch (type) {
+	case RMNET_MAP_COAL_CLOSE_NON_COAL:
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_close_non_coal);
+		u64_stats_update_end(&pcpu_ptr->syncp);
+		break;
+	case RMNET_MAP_COAL_CLOSE_IP_MISS:
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_close_ip_miss);
+		u64_stats_update_end(&pcpu_ptr->syncp);
+		break;
+	case RMNET_MAP_COAL_CLOSE_TRANS_MISS:
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_close_trans_miss);
+		u64_stats_update_end(&pcpu_ptr->syncp);
+		break;
+	case RMNET_MAP_COAL_CLOSE_HW:
+		switch (code) {
+		case RMNET_MAP_COAL_CLOSE_HW_NL:
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_close_hw_nl);
+			u64_stats_update_end(&pcpu_ptr->syncp);
+			break;
+		case RMNET_MAP_COAL_CLOSE_HW_PKT:
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_close_hw_pkt);
+			u64_stats_update_end(&pcpu_ptr->syncp);
+			break;
+		case RMNET_MAP_COAL_CLOSE_HW_BYTE:
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_close_hw_byte);
+			u64_stats_update_end(&pcpu_ptr->syncp);
+			break;
+		case RMNET_MAP_COAL_CLOSE_HW_TIME:
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_close_hw_time);
+			u64_stats_update_end(&pcpu_ptr->syncp);
+			break;
+		case RMNET_MAP_COAL_CLOSE_HW_EVICT:
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.coal_close_hw_evict);
+			u64_stats_update_end(&pcpu_ptr->syncp);
+			break;
+		default:
+			break;
+		}
+		break;
+	case RMNET_MAP_COAL_CLOSE_COAL:
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_close_coal);
+		u64_stats_update_end(&pcpu_ptr->syncp);
+		break;
+	default:
+		break;
+	}
+}
+
 /* Validate the coalescing header and build the checksum error mask.
  *
  * Checks performed:
@@ -1062,23 +1282,34 @@ static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
 					    u16 *num_pkts)
 {
 	struct rmnet_map_header *maph = (struct rmnet_map_header *)skb->data;
+	struct rmnet_priv *priv = netdev_priv(skb->dev);
 	struct rmnet_map_v5_coal_header *coal_hdr;
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	u16 pkts = 0;
 	u64 mask = 0;
 	int nlos;
 	int i;
 
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
 	/* coal header is counted in pkt_len */
-	if (ntohs(maph->pkt_len) < sizeof(*coal_hdr))
+	if (ntohs(maph->pkt_len) < sizeof(*coal_hdr)) {
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_hdr_err);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EINVAL;
+	}
 
 	coal_hdr = (struct rmnet_map_v5_coal_header *)(skb->data + sizeof(*maph));
 	nlos = rmnet_map_v5_get_num_nlos(coal_hdr);
-	if (nlos < 0)
+	if (nlos < 0) {
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_hdr_err);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		return -EINVAL;
+	}
 
 	*num_nlos = nlos;
-
 	for (i = 0; i < RMNET_MAP_V5_MAX_NLOS; i++) {
 		u8 err = coal_hdr->nl_pairs[i].csum_error_bitmap;
 		u8 pkt = coal_hdr->nl_pairs[i].num_packets;
@@ -1086,11 +1317,27 @@ static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
 		mask |= ((u64)err) << (8 * i);
 		if (i < *num_nlos) {
 			pkts += pkt;
-			if (pkts > RMNET_MAP_V5_MAX_PACKETS)
+			if (pkts > RMNET_MAP_V5_MAX_PACKETS) {
+				u64_stats_update_begin(&pcpu_ptr->syncp);
+				u64_stats_inc(&pcpu_ptr->priv_stats.coal_hdr_pkt_err);
+				u64_stats_update_end(&pcpu_ptr->syncp);
 				return -EINVAL;
+			}
 		}
 	}
 
+	/* coal_pkts counts packets reported by the hardware, independent of
+	 * whether the frame is later delivered through HW-GRO.
+	 */
+	u64_stats_update_begin(&pcpu_ptr->syncp);
+	u64_stats_add(&pcpu_ptr->priv_stats.coal_pkts, pkts);
+	u64_stats_update_end(&pcpu_ptr->syncp);
+	rmnet_map_data_log_close_stats(pcpu_ptr,
+				       u8_get_bits(coal_hdr->close_info,
+						   MAPV5_CLOSEINFO_CLOSE_TYPE_FMASK),
+				       u8_get_bits(coal_hdr->close_info,
+						   MAPV5_CLOSEINFO_CLOSE_VALUE_FMASK));
+
 	*nlo_err_mask = mask;
 	*num_pkts = pkts;
 	return 0;
@@ -1101,16 +1348,23 @@ int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
 				      u16 len, u32 data_format)
 {
 	struct rmnet_priv *priv = netdev_priv(skb->dev);
+	struct rmnet_pcpu_stats *pcpu_ptr;
 	u64 nlo_err_mask;
 	u16 num_pkts;
 	u8 num_nlos;
 	int rc;
 
+	pcpu_ptr = this_cpu_ptr(priv->pcpu_stats);
+
 	switch (rmnet_map_get_next_hdr_type(skb)) {
 	case RMNET_MAP_HEADER_TYPE_COALESCING:
 		if (!(data_format & RMNET_FLAGS_INGRESS_COALESCE))
 			return -EINVAL;
 
+		/* coal_rx counts coalescing input, not delivered HW-GRO skbs. */
+		u64_stats_update_begin(&pcpu_ptr->syncp);
+		u64_stats_inc(&pcpu_ptr->priv_stats.coal_rx);
+		u64_stats_update_end(&pcpu_ptr->syncp);
 		rc = rmnet_map_data_check_coal_header(skb, &nlo_err_mask,
 						      &num_nlos, &num_pkts);
 		if (rc)
@@ -1135,12 +1389,18 @@ int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
 
 	case RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD:
 		if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) {
-			priv->stats.csum_sw++;
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.csum_sw);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 		} else if (rmnet_map_get_csum_valid(skb)) {
-			priv->stats.csum_ok++;
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.csum_ok);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 		} else {
-			priv->stats.csum_valid_unset++;
+			u64_stats_update_begin(&pcpu_ptr->syncp);
+			u64_stats_inc(&pcpu_ptr->priv_stats.csum_valid_unset);
+			u64_stats_update_end(&pcpu_ptr->syncp);
 		}
 
 		skb_pull(skb, sizeof(struct rmnet_map_header) +
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index b8542d2f03b2..d76af4788243 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -7,6 +7,7 @@
 #include <linux/capability.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
+#include <net/netdev_queues.h>
 #include <linux/if_arp.h>
 #include <linux/netlink.h>
 #include <net/pkt_sched.h>
@@ -141,8 +142,10 @@ static void rmnet_vnd_uninit(struct net_device *dev)
 static void rmnet_get_stats64(struct net_device *dev,
 			      struct rtnl_link_stats64 *s)
 {
+	struct rmnet_priv_stats total_priv_stats = { };
 	struct rmnet_priv *priv = netdev_priv(dev);
 	struct rmnet_vnd_stats total_stats = { };
+	struct rmnet_priv_stats priv_snapshot;
 	struct rmnet_pcpu_stats *pcpu_ptr;
 	struct rmnet_vnd_stats snapshot;
 	unsigned int cpu, start;
@@ -153,6 +156,8 @@ static void rmnet_get_stats64(struct net_device *dev,
 		do {
 			start = u64_stats_fetch_begin(&pcpu_ptr->syncp);
 			snapshot = pcpu_ptr->stats;	/* struct assignment */
+			u64_stats_copy(&priv_snapshot, &pcpu_ptr->priv_stats,
+				       sizeof(priv_snapshot));
 		} while (u64_stats_fetch_retry(&pcpu_ptr->syncp, start));
 
 		total_stats.rx_pkts += snapshot.rx_pkts;
@@ -160,6 +165,7 @@ static void rmnet_get_stats64(struct net_device *dev,
 		total_stats.tx_pkts += snapshot.tx_pkts;
 		total_stats.tx_bytes += snapshot.tx_bytes;
 		total_stats.tx_drops += snapshot.tx_drops;
+		total_priv_stats.rx_dropped += priv_snapshot.rx_dropped;
 	}
 
 	s->rx_packets = total_stats.rx_pkts;
@@ -167,8 +173,45 @@ static void rmnet_get_stats64(struct net_device *dev,
 	s->tx_packets = total_stats.tx_pkts;
 	s->tx_bytes = total_stats.tx_bytes;
 	s->tx_dropped = total_stats.tx_drops;
+	s->rx_dropped = total_priv_stats.rx_dropped;
 }
 
+/* Report standard network stack RX statistics. These are distinct from the
+ * rmnet-specific coalescing diagnostics exposed through ethtool.
+ */
+static void rmnet_get_queue_stats_rx(struct net_device *dev, int idx,
+				     struct netdev_queue_stats_rx *stats)
+{
+	struct rmnet_priv *priv = netdev_priv(dev);
+	struct rmnet_priv_stats total_stats = { };
+	struct rmnet_pcpu_stats *pcpu_ptr;
+	struct rmnet_priv_stats snapshot;
+	unsigned int cpu, start;
+
+	for_each_possible_cpu(cpu) {
+		pcpu_ptr = per_cpu_ptr(priv->pcpu_stats, cpu);
+
+		do {
+			start = u64_stats_fetch_begin(&pcpu_ptr->syncp);
+			u64_stats_copy(&snapshot, &pcpu_ptr->priv_stats,
+				       sizeof(snapshot));
+		} while (u64_stats_fetch_retry(&pcpu_ptr->syncp, start));
+
+		total_stats.rx_hw_gro_packets += snapshot.rx_hw_gro_packets;
+		total_stats.rx_hw_gro_wire_packets +=
+			snapshot.rx_hw_gro_wire_packets;
+		total_stats.rx_alloc_fail += snapshot.rx_alloc_fail;
+	}
+
+	stats->hw_gro_packets = total_stats.rx_hw_gro_packets;
+	stats->hw_gro_wire_packets = total_stats.rx_hw_gro_wire_packets;
+	stats->alloc_fail = total_stats.rx_alloc_fail;
+}
+
+static const struct netdev_stat_ops rmnet_stat_ops = {
+	.get_queue_stats_rx = rmnet_get_queue_stats_rx,
+};
+
 static const struct net_device_ops rmnet_vnd_ops = {
 	.ndo_start_xmit = rmnet_vnd_start_xmit,
 	.ndo_change_mtu = rmnet_vnd_change_mtu,
@@ -192,8 +235,34 @@ static const char rmnet_gstrings_stats[][ETH_GSTRING_LEN] = {
 	"Checksum skipped",
 	"Checksum computed in software",
 	"Checksum computed in hardware",
+	/* DL coalescing */
+	"Coal alloc packet drops",
+	"Coal frames received",
+	"Packets in coal frames",
+	"Coal header errors",
+	"Coal hdr pkt count errors",
+	"Coal bounds errors",
+	"Coal checksum errors",
+	"Coal packets csum err drops",
+	"Coal segments reconstructed",
+	"Coal invalid IP packet",
+	"Coal invalid transport",
+	/* close reasons */
+	"Coal closed: non-coal",
+	"Coal closed: IP miss",
+	"Coal closed: transport miss",
+	"Coal closed: hw NL limit",
+	"Coal closed: hw pkt limit",
+	"Coal closed: hw byte limit",
+	"Coal closed: hw time limit",
+	"Coal closed: hw evict",
+	"Coal closed: FIN/PSH",
 };
 
+static_assert(ARRAY_SIZE(rmnet_gstrings_stats) ==
+	      (sizeof(struct rmnet_priv_stats) -
+	       offsetof(struct rmnet_priv_stats, csum_ok)) / sizeof(u64));
+
 static void rmnet_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
 {
 	switch (stringset) {
@@ -218,12 +287,68 @@ static void rmnet_get_ethtool_stats(struct net_device *dev,
 				    struct ethtool_stats *stats, u64 *data)
 {
 	struct rmnet_priv *priv = netdev_priv(dev);
-	struct rmnet_priv_stats *st = &priv->stats;
+	struct rmnet_priv_stats total_stats = { };
+	struct rmnet_pcpu_stats *pcpu_ptr;
+	struct rmnet_priv_stats *snapshot;
+	unsigned int cpu, start;
 
 	if (!data)
 		return;
 
-	memcpy(data, st, ARRAY_SIZE(rmnet_gstrings_stats) * sizeof(u64));
+	snapshot = kzalloc_obj(*snapshot);
+	if (!snapshot)
+		return;
+
+	for_each_possible_cpu(cpu) {
+		pcpu_ptr = per_cpu_ptr(priv->pcpu_stats, cpu);
+
+		do {
+			start = u64_stats_fetch_begin(&pcpu_ptr->syncp);
+			u64_stats_copy(snapshot, &pcpu_ptr->priv_stats,
+				       sizeof(*snapshot));
+		} while (u64_stats_fetch_retry(&pcpu_ptr->syncp, start));
+
+		total_stats.csum_ok += snapshot->csum_ok;
+		total_stats.csum_ip4_header_bad += snapshot->csum_ip4_header_bad;
+		total_stats.csum_valid_unset += snapshot->csum_valid_unset;
+		total_stats.csum_validation_failed += snapshot->csum_validation_failed;
+		total_stats.csum_err_bad_buffer += snapshot->csum_err_bad_buffer;
+		total_stats.csum_err_invalid_ip_version +=
+			snapshot->csum_err_invalid_ip_version;
+		total_stats.csum_err_invalid_transport +=
+			snapshot->csum_err_invalid_transport;
+		total_stats.csum_fragmented_pkt += snapshot->csum_fragmented_pkt;
+		total_stats.csum_skipped += snapshot->csum_skipped;
+		total_stats.csum_sw += snapshot->csum_sw;
+		total_stats.csum_hw += snapshot->csum_hw;
+		/* Custom rmnet coalescing diagnostics follow. */
+		total_stats.coal_alloc_fail += snapshot->coal_alloc_fail;
+		total_stats.coal_rx += snapshot->coal_rx;
+		total_stats.coal_pkts += snapshot->coal_pkts;
+		total_stats.coal_hdr_err += snapshot->coal_hdr_err;
+		total_stats.coal_hdr_pkt_err += snapshot->coal_hdr_pkt_err;
+		total_stats.coal_bounds_err += snapshot->coal_bounds_err;
+		total_stats.coal_csum_err += snapshot->coal_csum_err;
+		total_stats.coal_csum_drop += snapshot->coal_csum_drop;
+		total_stats.coal_reconstruct += snapshot->coal_reconstruct;
+		total_stats.coal_ip_invalid += snapshot->coal_ip_invalid;
+		total_stats.coal_trans_invalid += snapshot->coal_trans_invalid;
+		total_stats.coal_close_non_coal += snapshot->coal_close_non_coal;
+		total_stats.coal_close_ip_miss += snapshot->coal_close_ip_miss;
+		total_stats.coal_close_trans_miss += snapshot->coal_close_trans_miss;
+		total_stats.coal_close_hw_nl += snapshot->coal_close_hw_nl;
+		total_stats.coal_close_hw_pkt += snapshot->coal_close_hw_pkt;
+		total_stats.coal_close_hw_byte += snapshot->coal_close_hw_byte;
+		total_stats.coal_close_hw_time += snapshot->coal_close_hw_time;
+		total_stats.coal_close_hw_evict += snapshot->coal_close_hw_evict;
+		total_stats.coal_close_coal += snapshot->coal_close_coal;
+	}
+
+	kfree(snapshot);
+
+	/* The first four fields are reported through netdev queue statistics. */
+	memcpy(data, &total_stats.ethtool_stats,
+	       sizeof(total_stats.ethtool_stats));
 }
 
 static int rmnet_get_coalesce(struct net_device *dev,
@@ -332,6 +457,7 @@ int rmnet_vnd_newlink(u8 id, struct net_device *rmnet_dev,
 	rmnet_dev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	rmnet_dev->hw_features |= NETIF_F_SG;
 	rmnet_dev->hw_features |= NETIF_F_GRO_HW;
+	rmnet_dev->stat_ops = &rmnet_stat_ops;
 
 	priv->real_dev = real_dev;
 
-- 
2.34.1

Re: [PATCH net-next v3 7/8] net: qualcomm: rmnet: Add DL coalescing statistics

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 5 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] rmnet's new netdev_stat_ops does not follow the qstats
  contract.
- [Medium] In rmnet_map_segment_coal_skb() (rmnet_map_data.c:1166-1174),
  the standard HW-GRO counters are updated once per input MAPv5…
- [Medium] The new ethtool -S string 'Coal alloc packet drops'
  (coal_alloc_fail) duplicates a standard uAPI counter.
- [Medium] The comments and commit message misdescribe how far the new
  standard counters reach.
- [Medium] The standard qstats rx-alloc-fail counter is fed a packet
  count instead of a count of allocation failures.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  rmnet_vnd_init() (rmnet_vnd.c:121) allocates struct rmnet_pcpu_stats…

--

net: qualcomm: rmnet: Add DL coalescing statistics

This patch moves the rmnet private checksum counters into the per-CPU
u64_stats protected structure and adds DL coalescing counters. The
HW-GRO and allocation failure counters are exported through
netdev_stat_ops queue stats and rx_dropped through ndo_get_stats64. The
remaining counters are exported through ethtool -S.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
> index 5adda0323dda7..917d31f6eded7 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
> @@ -63,31 +63,105 @@ struct rmnet_vnd_stats {
>  	u32 tx_drops;
>  };
>  
> +struct rmnet_priv_stats {
> +	/* Network stack RX statistics for all rmnet receive paths */
> +	u64 rx_hw_gro_packets;
> +	u64 rx_hw_gro_wire_packets;
> +	u64 rx_alloc_fail;
> +	u64 rx_dropped;

[Severity: Medium]
Is "all rmnet receive paths" accurate here? rx_dropped and rx_alloc_fail
are only written in __rmnet_map_segment_coal_skb().

Other RX drops on the same rmnet device leave both counters unchanged.
For example, rmnet_map_process_next_hdr_packet() has:

		if (skb_unclone(skb, GFP_ATOMIC))
			return -ENOMEM;

The coalescing header, packet count, bounds and IP/transport validation
failures also return an error. In each case __rmnet_map_ingress_handler()
then does:

		if (rmnet_map_process_next_hdr_packet(skb, &list, len, data_format))
			goto free_skb;

so the whole frame is freed and the standard counters do not change. The
alloc_skb() failure in rmnet_map_deaggregate() happens before the rmnet
device is known, so that one is harder to attribute. The skb_unclone()
and validation paths already have skb->dev set to the rmnet device.

There is also a mismatch with the commit message, which says:

  Expose these standard RX statistics through netdev_stat_ops:
  ...
  - RX drops for reconstructed packets discarded by rmnet

and with rmnet_get_ethtool_stats(), which says:

	/* The first four fields are reported through netdev queue statistics. */

In the code, rmnet_get_queue_stats_rx() reports three fields, and
rx_dropped is reported only through rmnet_get_stats64().

Could the comments and the commit message be updated to match what the
code does?

[ ... ]

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index e8adb40067175..74d64b7066e2b 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

[ ... ]

> @@ -675,12 +732,29 @@ __rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,

[ ... ]

>  	skbn = alloc_skb(hlen + dlen + RMNET_MAP_DEAGGR_HEADROOM, GFP_ATOMIC);
> -	if (!skbn)
> +	if (!skbn) {
> +		u64_stats_update_begin(&pcpu_ptr->syncp);
> +		/* The coal counter is the coalescing subset of the standard
> +		 * allocation failure and RX drop counters.
> +		 */
> +		u64_stats_add(&pcpu_ptr->priv_stats.rx_alloc_fail, coal_meta->pkt_count);

[Severity: Medium]
Should rx_alloc_fail be incremented by coal_meta->pkt_count here? The
whole segment uses a single alloc_skb() call, and netdev.yaml defines
rx-alloc-fail as:

  Number of times skb or buffer allocation failed on the Rx datapath.

As written, one failed allocation for a multi-packet segment shows up as
up to RMNET_MAP_V5_MAX_PACKETS failures in stats->alloc_fail, which
rmnet_get_queue_stats_rx() reports.

Would u64_stats_inc() be the right update for rx_alloc_fail, with the
packet count kept for rx_dropped and coal_alloc_fail?

> +		u64_stats_add(&pcpu_ptr->priv_stats.rx_dropped, coal_meta->pkt_count);
> +		u64_stats_add(&pcpu_ptr->priv_stats.coal_alloc_fail, coal_meta->pkt_count);
> +		u64_stats_update_end(&pcpu_ptr->syncp);
>  		goto next_pkt;
> +	}

[ ... ]

> @@ -1014,6 +1163,16 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
>  	if (total_pkts > 1 && (!rxcsum || !gro_hw))
>  		return -EINVAL;
>  
> +	if (gro && total_pkts > 1) {
> +		/* Standard counters cover delivered HW-GRO skbs. coal_rx and
> +		 * coal_pkts cover coalescing input regardless of HW-GRO.
> +		 */
> +		u64_stats_update_begin(&pcpu_ptr->syncp);
> +		u64_stats_inc(&pcpu_ptr->priv_stats.rx_hw_gro_packets);
> +		u64_stats_add(&pcpu_ptr->priv_stats.rx_hw_gro_wire_packets, total_pkts);
> +		u64_stats_update_end(&pcpu_ptr->syncp);
> +	}

[Severity: Medium]
These counters go up once per input coalescing frame, before the driver
knows how the frame will be delivered. The comment says they cover
delivered HW-GRO skbs. However, only rmnet_map_coal_gro_fast_path()
delivers a single aggregate, and only when num_nlos == 1 and
MAPV5_COALINFO_CSUM_VALID_FLAG is set.

In the other cases, rmnet_map_coal_segment_loop() flushes at every NLO
boundary. It also splits around each packet that has a checksum error
bit and drops that packet. __rmnet_map_segment_coal_skb() only
GSO-stamps a segment when pkt_count > 1.

Some examples:

- Three NLOs with one packet each are counted as 1 HW-GRO packet and 3
  wire packets, but no GSO skb is delivered.

- One NLO of 10 packets with CSUM_VALID clear and two error bits is
  counted as 1 and 10. Up to 3 GSO skbs are delivered, and the 2 dropped
  packets are also counted in rx_dropped.

- Segments that hit an alloc_skb() failure in
  __rmnet_map_segment_coal_skb() have already been counted as HW-GRO.

Would it be more accurate to update rx_hw_gro_packets and
rx_hw_gro_wire_packets where a GSO-stamped skb is actually queued, i.e.
in the fast path and in __rmnet_map_segment_coal_skb()?

[ ... ]

> @@ -1101,16 +1348,23 @@ int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,

[ ... ]

>  	switch (rmnet_map_get_next_hdr_type(skb)) {
>  	case RMNET_MAP_HEADER_TYPE_COALESCING:
>  		if (!(data_format & RMNET_FLAGS_INGRESS_COALESCE))
>  			return -EINVAL;
>  
> +		/* coal_rx counts coalescing input, not delivered HW-GRO skbs. */
> +		u64_stats_update_begin(&pcpu_ptr->syncp);
> +		u64_stats_inc(&pcpu_ptr->priv_stats.coal_rx);
> +		u64_stats_update_end(&pcpu_ptr->syncp);

[Severity: Low]
This issue is pre-existing and was not introduced by this patch.
rmnet_vnd_init() allocates the per-CPU stats with plain alloc_percpu()
and never calls u64_stats_init() on any CPU's syncp:

	priv->pcpu_stats = alloc_percpu(struct rmnet_pcpu_stats);

On 32-bit SMP kernels with CONFIG_DEBUG_LOCK_ALLOC,
u64_stats_update_begin() calls write_seqcount_begin() on a zeroed
dep_map that has no key. lockdep then prints "INFO: trying to register
non-static key" and turns itself off.

The existing writers in rmnet_vnd_rx_fixup() and rmnet_vnd_tx_fixup()
already hit this. This patch adds many more writers on the same syncp
in the RX coalescing, RX checksum and TX checksum paths. Torn-read
protection still works, since a zeroed seqcount is valid. Only lockdep
is affected.

Since this code is being reworked anyway, would it make sense to call
u64_stats_init() for each possible CPU, or to use
netdev_alloc_pcpu_stats()?

[ ... ]

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> index b8542d2f03b23..d76af4788243b 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c

[ ... ]

> @@ -167,8 +173,45 @@ static void rmnet_get_stats64(struct net_device *dev,

[ ... ]

> +static void rmnet_get_queue_stats_rx(struct net_device *dev, int idx,
> +				     struct netdev_queue_stats_rx *stats)
> +{

[ ... ]

> +	stats->hw_gro_packets = total_stats.rx_hw_gro_packets;
> +	stats->hw_gro_wire_packets = total_stats.rx_hw_gro_wire_packets;
> +	stats->alloc_fail = total_stats.rx_alloc_fail;
> +}
> +
> +static const struct netdev_stat_ops rmnet_stat_ops = {
> +	.get_queue_stats_rx = rmnet_get_queue_stats_rx,
> +};

[Severity: Medium]
rmnet_get_queue_stats_rx() never uses idx, so every RX queue reports the
device-wide totals. netdev_nl_stats_by_queue() calls it once for each
index below real_num_rx_queues:

	while (ops->get_queue_stats_rx && i < netdev->real_num_rx_queues) {

rmnet_link_ops has no get_num_rx_queues, so the queue count comes from
IFLA_NUM_RX_QUEUES (for example "ip link add ... numrxqueues 4 type
rmnet"). In that case each queue reports the same totals, and the sum
over all queues is N times the real value.

Separately, rmnet_stat_ops does not set .get_base_stats, and
netdev_nl_stats_by_netdev() returns early in that case:

	if (!netdev->stat_ops->get_base_stats)
		return 0;

As a result, does the default netdev-scope qstats-get dump report
nothing for rmnet? Since these counters are not tracked per queue,
would reporting them from .get_base_stats fit the qstats model better?

[ ... ]

> @@ -192,8 +235,34 @@ static const char rmnet_gstrings_stats[][ETH_GSTRING_LEN] = {
>  	"Checksum skipped",
>  	"Checksum computed in software",
>  	"Checksum computed in hardware",
> +	/* DL coalescing */
> +	"Coal alloc packet drops",

[Severity: Medium]
This counter looks like it will always equal the standard alloc_fail.
rx_alloc_fail is written in only one place, in
__rmnet_map_segment_coal_skb(), right next to coal_alloc_fail:

		u64_stats_add(&pcpu_ptr->priv_stats.rx_alloc_fail, coal_meta->pkt_count);
		u64_stats_add(&pcpu_ptr->priv_stats.rx_dropped, coal_meta->pkt_count);
		u64_stats_add(&pcpu_ptr->priv_stats.coal_alloc_fail, coal_meta->pkt_count);

rmnet_get_queue_stats_rx() already exports rx_alloc_fail as qstats
alloc_fail. Is a separate ethtool -S entry needed for the same value?

The commit message says the ethtool stats are kept for "diagnostics that
have no standard representation". Its list does not include
coal_alloc_fail, or coal_rx, coal_pkts, coal_csum_err and
coal_csum_drop.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009084351.317632-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next v3 8/8] docs: networking: Add documentation for the coalescing support in rmnet

[Subash Abhinov Kasiviswanathan]

Add information about the MAPv5 coalescing header covering the layout
and the information from the fields in the header.

Document the IFLA_RMNET_FLAGS coalescing rules. Ingress coalescing
requires MAPv5 checksum offload, and MAPv4 and MAPv5 checksum
configurations cannot be enabled together in either direction. A direction
may leave checksum offload disabled.

Document that the frame-level CSUM valid indication is distinct from the
per-packet CSUM error bitmap. Multi-packet coalesced frames require RX
checksum offload and rx-gro-hw. Single-packet frames are delivered as
normal non-GSO skbs when those features are disabled.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
v3: no change
v2: https://lore.kernel.org/all/20261008005543.2630828-9-subash.a.kasiviswanathan@oss.qualcomm.com/
v1: https://lore.kernel.org/all/20260930051345.857443-8-subash.a.kasiviswanathan@oss.qualcomm.com/

 .../cellular/qualcomm/rmnet.rst               | 153 +++++++++++++++++-
 1 file changed, 145 insertions(+), 8 deletions(-)

diff --git a/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst b/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
index 5aedbabb7382..52f92d2fba31 100644
--- a/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
+++ b/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
@@ -125,8 +125,8 @@ Command (1)/ Data (0) bit value is to indicate if the packet is a MAP command
 or data packet. Command packet is used for transport level flow control. Data
 packets are standard IP packets.
 
-Next header is used to indicate the presence of another header, currently is
-limited to checksum header.
+Next header is used to indicate the presence of another header, currently
+limited to the checksum and coalescing headers.
 
 Padding is the number of bytes to be appended to the payload to
 ensure 4 byte alignment.
@@ -150,11 +150,11 @@ Header Type is to indicate the type of header, this usually is set to CHECKSUM
 
 Header types
 
-= ===============
+= ======================
 0 Reserved
-1 Reserved
+1 coalescing header
 2 checksum header
-= ===============
+= ======================
 
 Checksum Valid is to indicate whether the header checksum is valid. Value of 1
 implies that checksum is calculated on this packet and is valid, value of 0
@@ -162,8 +162,109 @@ indicates that the calculated packet checksum is invalid.
 
 Reserved bits must be zero when sent and ignored when received.
 
-e. MAP packet v1/v5 (command specific)
---------------------------------------
+e. Coalescing header v5
+------------------------
+
+Hardware can coalesce multiple same-flow IP packets into a single MAP frame
+to reduce per-packet overhead at high data rates.  Packets are grouped into
+NLOs, with all packets in each NLO having the same length.  The coalescing
+header (header type 1) describes the coalesced content.
+
+Packet format::
+
+  Bit         0 - 6         7          8           9-11         12-15
+  Function  Header Type  Next Header  CSUM valid  Num NLOs    (reserved)
+
+  Bit        16-19        20-23
+  Function  Close value  Close type
+
+  Bit        24-27        28-31
+  Function  (reserved)    VEID
+
+  Bit           32 - 47        48 - 55            56 - 63
+  Function   Packet length  CSUM error bitmap  Num packets (NLO 0)
+
+  ... (up to 6 NLO entries total, same 32-bit format per entry)
+
+Header Type is set to 1 (coalescing).
+
+The MAP header ``pkt_len`` includes the coalescing header, coalesced packet
+data, and any MAP padding.
+
+Num NLOs (Number-Length Objects) is the count of active NLO entries
+(1 – 6).  Each NLO describes a group of consecutive coalesced packets
+that all share the same IP packet length.
+
+Num NLOs identifies the active prefix of the six NLO slots.  The header
+always contains all six slots and remains 28 bytes long regardless of Num
+NLOs.  A slot with ``num_packets == 0`` ends the active NLO prefix.  The
+full coalescing header is included in the MAP header's ``pkt_len``.
+
+Some hardware may report Num NLOs incorrectly.  For compatibility, receivers
+should derive the active prefix from ``num_packets`` when needed.  This
+requires unused slots to have ``num_packets == 0``.
+
+CSUM valid (bit 8) is a frame-level indication that the hardware checksum is
+valid for all packets in the frame. It is distinct from the per-packet CSUM
+error bitmap in each NLO entry.
+
+For a single-NLO, single-packet frame, CSUM valid must not be trusted when
+the close reason is a FIN/PSH close, a packet limit, a byte limit, or a time
+limit.  In these cases, the rmnet driver treats the packet checksum as
+unverified and lets the network stack validate it instead of relying on the
+coalescing checksum indications.
+IPv4 UDP packets with a zero checksum remain valid because that checksum is
+optional.
+
+Close type and close value encode the hardware reason that coalescing
+was terminated for this frame:
+
+Close type values:
+
+= ==============================
+0 non-coalesced (single packet)
+1 IP flow miss
+2 transport flow miss
+3 hardware limit (see value)
+4 coalescing closed (FIN/PSH)
+= ==============================
+
+Close value (used when close type is 3):
+
+= ==================
+0 NL limit reached
+1 packet limit
+2 byte limit
+3 time limit
+4 eviction
+= ==================
+
+VEID is the virtual endpoint ID of the originating flow.
+
+Each NLO entry::
+
+  Bit         0 - 15        16 - 23            24 - 31
+  Function  Pkt length   CSUM error bitmap   Num packets
+
+Pkt length is the full IP packet length, including the IP header, transport
+header, and payload, for every packet in this NLO group. The IP and transport
+headers are present once in the coalesced frame and are not repeated for each
+packet.
+
+CSUM error bitmap is one 48-bit stream formed by concatenating the
+``csum_error_bitmap`` bytes from all six NLO slots in slot order. Packet 0
+corresponds to bit 0 of slot 0's bitmap byte, and bits are consumed from
+least significant bit to most significant bit. The bit stream is indexed by
+the packet's absolute position in the frame and does not restart at an NLO
+boundary. If an NLO contains more than eight packets, its error bits
+continue into the bitmap byte of the following slot. Bitmap bytes in slots
+after the active NLO prefix may therefore contain continuation bits and must
+not be ignored.
+
+Num packets is the count of coalesced packets described by this NLO.
+
+f. MAP packet v1/v5 (command specific)
+---------------------------------------
 
 Packet format::
 
@@ -187,7 +288,7 @@ Command types
 3 is for error during processing of commands
 = ==========================================
 
-f. Aggregation
+g. Aggregation
 --------------
 
 Aggregation is multiple MAP packets (can be data or command) delivered to
@@ -208,3 +309,39 @@ rmnet userspace configuration is done through netlink using iproute2
 https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/
 
 The driver uses rtnl_link_ops for communication.
+
+The data format flags controlling the ingress and egress processing
+pipeline are set via the ``IFLA_RMNET_FLAGS`` attribute
+(``struct ifla_rmnet_flags``).
+
+Relevant ingress flags:
+
+``RMNET_FLAGS_INGRESS_DEAGGREGATION``
+  Enable MAP frame de-aggregation.
+
+``RMNET_FLAGS_INGRESS_MAP_CKSUMV4``
+  Enable MAPv4 downlink checksum offload.
+
+``RMNET_FLAGS_INGRESS_MAP_CKSUMV5``
+  Enable MAPv5 downlink checksum offload (header type 2).
+
+``RMNET_FLAGS_INGRESS_COALESCE``
+  Enable MAPv5 downlink hardware coalescing (header type 1).
+  This flag requires ``RMNET_FLAGS_INGRESS_MAP_CKSUMV5``. MAPv4 and
+  MAPv5 checksum flags cannot be enabled together in either direction.
+  A direction may leave checksum offload disabled. Invalid combinations
+  are rejected.
+  When RX checksum offload and ``rx-gro-hw`` are enabled, valid
+  multi-packet coalesced frames can be delivered as batched GSO SKBs.
+  Otherwise, multi-packet coalesced frames are rejected. A coalesced frame
+  containing one packet is delivered as a normal non-GSO skb. IP options,
+  IPv6 extension headers, and zero-payload packets can also prevent GSO
+  processing for a frame.
+
+Relevant egress flags:
+
+``RMNET_FLAGS_EGRESS_MAP_CKSUMV4``
+  Enable MAPv4 uplink checksum offload.
+
+``RMNET_FLAGS_EGRESS_MAP_CKSUMV5``
+  Enable MAPv5 uplink checksum offload.
-- 
2.34.1

Re: [PATCH net-next v3 8/8] docs: networking: Add documentation for the coalescing support in rmnet

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new rmnet.rst text never says that rmnet itself drops packets
  that the CSUM error bitmap flags, and it states the single-packet…
- [Low] The MAPv4/MAPv5 checksum mutual-exclusion rule is documented only
  under RMNET_FLAGS_INGRESS_COALESCE, and its wording is ambiguous:…
- [Low] The comment for the rmnet_map_header flags field in
  include/linux/if_rmnet.h still says 'NEXT_HEADER: 1 = packet contains…
- [Low] The comment above rmnet_map_segment_coal_skb()
  (rmnet_map_data.c:1116) says 'NLOs with checksum errors are dropped.'…
- [Low] The new section 'e.

--

docs: networking: Add documentation for the coalescing support in rmnet

This commit documents the MAPv5 coalescing header layout and field
semantics in rmnet.rst. It also documents the IFLA_RMNET_FLAGS ingress and
egress flags, the rules for combining coalescing with checksum offload,
and when coalesced frames are delivered as GSO skbs.

> diff --git a/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst b/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
> index 5aedbabb73827..52f92d2fba311 100644
> --- a/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
> +++ b/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
> @@ -125,8 +125,8 @@ Command (1)/ Data (0) bit value is to indicate if the packet is a MAP command
>  or data packet. Command packet is used for transport level flow control. Data
>  packets are standard IP packets.
>  
> -Next header is used to indicate the presence of another header, currently is
> -limited to checksum header.
> +Next header is used to indicate the presence of another header, currently
> +limited to the checksum and coalescing headers.

[Severity: Low]
This text now disagrees with the flags comment in include/linux/if_rmnet.h.
The series did not update that comment:

/* rmnet_map_header flags field:
 ...
 *  NEXT_HEADER: 1 = packet contains V5 CSUM header 0 = no V5 CSUM header
 */
#define MAP_NEXT_HEADER_FLAG		BIT(6)

After this series, MAP_NEXT_HEADER_FLAG can also indicate a coalescing
header. rmnet_map_validate_packet_len() handles the length of that type
differently:

		if (hdr_type == RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
			packet_len += sizeof(*next_hdr);
		else if (hdr_type != RMNET_MAP_HEADER_TYPE_COALESCING)
			return 0;

Should the comment in if_rmnet.h also mention the coalescing header?

[ ... ]

> @@ -162,8 +162,109 @@ indicates that the calculated packet checksum is invalid.
>  
>  Reserved bits must be zero when sent and ignored when received.
>  
> -e. MAP packet v1/v5 (command specific)
> ---------------------------------------
> +e. Coalescing header v5
> +------------------------
> +
> +Hardware can coalesce multiple same-flow IP packets into a single MAP frame
> +to reduce per-packet overhead at high data rates.  Packets are grouped into
> +NLOs, with all packets in each NLO having the same length.  The coalescing
> +header (header type 1) describes the coalesced content.

[Severity: Low]
Sections a through d each say their fields are in big endian format. This
new section gives no byte order.

The NLO Pkt length field is big endian on the wire:

include/linux/if_rmnet.h:
struct rmnet_map_v5_nl_pair {
	__be16 pkt_len;
	u8  csum_error_bitmap;
	u8  num_packets;
} __aligned(1);

Could this section add a sentence such as "Coalescing header fields are in
big endian format."?

[ ... ]

> +CSUM error bitmap is one 48-bit stream formed by concatenating the
> +``csum_error_bitmap`` bytes from all six NLO slots in slot order. Packet 0
> +corresponds to bit 0 of slot 0's bitmap byte, and bits are consumed from
> +least significant bit to most significant bit. The bit stream is indexed by
> +the packet's absolute position in the frame and does not restart at an NLO
> +boundary. If an NLO contains more than eight packets, its error bits
> +continue into the bitmap byte of the following slot. Bitmap bytes in slots
> +after the active NLO prefix may therefore contain continuation bits and must
> +not be ignored.

[Severity: Low]
This description matches rmnet_map_coal_segment_loop(). The comment above
rmnet_map_segment_coal_skb() in rmnet_map_data.c says something different:

/* Expand a coalesced SKB into individual IP packets placed on the list.
 * NLOs with checksum errors are dropped. __rmnet_map_ingress_handler will
 * free the SKB in the error case.
 */

rmnet_map_coal_segment_loop() checks the flat bitmap once per packet:

			csum_err = nlo_err_mask & 1;

It flushes the good packets collected so far, drops only the errored
packet, and carries on within the same NLO.

Should that comment say that individual packets are dropped, not whole
NLOs?

[Severity: Low]
This section explains how the bitmap is encoded, but not what rmnet does
with the packets it flags. __rmnet_map_segment_coal_skb() drops a flagged
packet and counts it:

	if (!csum_valid && coal_meta->zero_csum)
		csum_valid = true;

	if (!csum_valid) {
		...
		u64_stats_add(&pcpu_ptr->priv_stats.rx_dropped, coal_meta->pkt_count);
		u64_stats_inc(&pcpu_ptr->priv_stats.coal_csum_drop);
		...
		goto next_pkt;
	}

Could the doc say that these packets are dropped and counted in
rx_dropped?

The zero checksum exemption appears only in the single-packet quirk
paragraph ("IPv4 UDP packets with a zero checksum remain valid ..."). In
the code, zero_csum overrides the bitmap for every rebuilt segment, not
only in that case.

The RMNET_FLAGS_INGRESS_COALESCE entry further down says "A coalesced
frame containing one packet is delivered as a normal non-GSO skb." Does
that still hold when RX checksum offload and rx-gro-hw are both enabled?

Take a single-NLO, single-packet frame where:

  - CSUM valid is clear
  - the close reason is not on the quirk list (for example close type 0,
    1 or 2)
  - bit 0 of the bitmap is set
  - the packet is not zero-checksum IPv4 UDP

The path looks like this:

rmnet_map_segment_coal_skb()
  rmnet_map_v5_csum_fixup() returns false
  rmnet_map_coal_gro_fast_path() returns false
  rmnet_map_coal_segment_loop()
    __rmnet_map_segment_coal_skb(..., false)   <- packet dropped

The list stays empty, rmnet_map_process_next_hdr_packet() frees the skb,
and nothing is delivered.

[ ... ]

> @@ -208,3 +309,39 @@ rmnet userspace configuration is done through netlink using iproute2
>  https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/
>  
>  The driver uses rtnl_link_ops for communication.
> +
> +The data format flags controlling the ingress and egress processing
> +pipeline are set via the ``IFLA_RMNET_FLAGS`` attribute
> +(``struct ifla_rmnet_flags``).
> +
> +Relevant ingress flags:
> +
> +``RMNET_FLAGS_INGRESS_DEAGGREGATION``
> +  Enable MAP frame de-aggregation.
> +
> +``RMNET_FLAGS_INGRESS_MAP_CKSUMV4``
> +  Enable MAPv4 downlink checksum offload.
> +
> +``RMNET_FLAGS_INGRESS_MAP_CKSUMV5``
> +  Enable MAPv5 downlink checksum offload (header type 2).
> +
> +``RMNET_FLAGS_INGRESS_COALESCE``
> +  Enable MAPv5 downlink hardware coalescing (header type 1).
> +  This flag requires ``RMNET_FLAGS_INGRESS_MAP_CKSUMV5``. MAPv4 and
> +  MAPv5 checksum flags cannot be enabled together in either direction.
> +  A direction may leave checksum offload disabled. Invalid combinations
> +  are rejected.

[Severity: Low]
"cannot be enabled together in either direction" reads like a rule
applied to each direction separately. rmnet_config_data_format_valid()
applies it to the whole device, whether or not
RMNET_FLAGS_INGRESS_COALESCE is set:

	u32 v4_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV4 |
		      RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
	u32 v5_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV5 |
		      RMNET_FLAGS_EGRESS_MAP_CKSUMV5;

	if ((data_format & v4_mask) && (data_format & v5_mask))
		return false;

This means rmnet_newlink() and rmnet_changelink() reject
RMNET_FLAGS_INGRESS_MAP_CKSUMV5 plus RMNET_FLAGS_EGRESS_MAP_CKSUMV4 with
-EINVAL, even with no coalescing.

The rule applies to the checksum flags by themselves. Could it go in the
RMNET_FLAGS_INGRESS/EGRESS_MAP_CKSUMV4/V5 entries, worded to say that only
one checksum version may be enabled across both directions?

The comment above rmnet_config_data_format_valid() ("selected
independently for ingress and egress, but only one version may be enabled
across the device") is also ambiguous on this point.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009084351.317632-1-subash.a.kasiviswanathan%40oss.qualcomm.com