diff mbox series

[v2,net] net: microchip: sparx5: correctly free skb in xmit

Message ID 20221129152635.15362-1-casper.casan@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v2,net] net: microchip: sparx5: correctly free skb in xmit | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 1 blamed authors not CCed: bjarni.jonasson@microchip.com; 3 maintainers not CCed: linux-arm-kernel@lists.infradead.org bjarni.jonasson@microchip.com zhengyongjun3@huawei.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes fail Problems with Fixes tag: 1
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 64 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Casper Andersson Nov. 29, 2022, 3:26 p.m. UTC 
consume_skb on transmitted, kfree_skb on dropped, do not free on
TX_BUSY.

Previously the xmit function could return -EBUSY without freeing, which
supposedly is interpreted as a drop. And was using kfree on successfully
transmitted packets.

sparx5_fdma_xmit and sparx5_inject returns error code, where -EBUSY
indicates TX_BUSY and any other error code indicates dropped.

Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support")

Signed-off-by: Casper Andersson <casper.casan@gmail.com>
---
 .../ethernet/microchip/sparx5/sparx5_fdma.c   |  2 +-
 .../ethernet/microchip/sparx5/sparx5_packet.c | 41 +++++++++++--------
 2 files changed, 25 insertions(+), 18 deletions(-)

Comments

Paolo Abeni Dec. 1, 2022, 12:52 p.m. UTC | #1 
On Tue, 2022-11-29 at 16:26 +0100, Casper Andersson wrote:
> consume_skb on transmitted, kfree_skb on dropped, do not free on
> TX_BUSY.
> 
> Previously the xmit function could return -EBUSY without freeing, which
> supposedly is interpreted as a drop. And was using kfree on successfully
> transmitted packets.
> 
> sparx5_fdma_xmit and sparx5_inject returns error code, where -EBUSY
> indicates TX_BUSY and any other error code indicates dropped.
> 
> Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support")
> 
> Signed-off-by: Casper Andersson <casper.casan@gmail.com>

Minor nit: please remote the empty line between the Fixes and SoB tags,
thanks!

Paolo
Horatiu Vultur Dec. 1, 2022, 2:38 p.m. UTC | #2 
The 11/29/2022 16:26, Casper Andersson wrote:
> 
> consume_skb on transmitted, kfree_skb on dropped, do not free on
> TX_BUSY.
> 
> Previously the xmit function could return -EBUSY without freeing, which
> supposedly is interpreted as a drop. And was using kfree on successfully
> transmitted packets.
> 
> sparx5_fdma_xmit and sparx5_inject returns error code, where -EBUSY
> indicates TX_BUSY and any other error code indicates dropped.
> 
> Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support")
> 
> Signed-off-by: Casper Andersson <casper.casan@gmail.com>

Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> ---
>  .../ethernet/microchip/sparx5/sparx5_fdma.c   |  2 +-
>  .../ethernet/microchip/sparx5/sparx5_packet.c | 41 +++++++++++--------
>  2 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> index 66360c8c5a38..141897dfe388 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> @@ -317,7 +317,7 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
>         next_dcb_hw = sparx5_fdma_next_dcb(tx, tx->curr_entry);
>         db_hw = &next_dcb_hw->db[0];
>         if (!(db_hw->status & FDMA_DCB_STATUS_DONE))
> -               tx->dropped++;
> +               return -EINVAL;
>         db = list_first_entry(&tx->db_list, struct sparx5_db, list);
>         list_move_tail(&db->list, &tx->db_list);
>         next_dcb_hw->nextptr = FDMA_DCB_INVALID_DATA;
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> index 83c16ca5b30f..6db6ac6a3bbc 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> @@ -234,9 +234,8 @@ netdev_tx_t sparx5_port_xmit_impl(struct sk_buff *skb, struct net_device *dev)
>         sparx5_set_port_ifh(ifh, port->portno);
> 
>         if (sparx5->ptp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> -               ret = sparx5_ptp_txtstamp_request(port, skb);
> -               if (ret)
> -                       return ret;
> +               if (sparx5_ptp_txtstamp_request(port, skb) < 0)
> +                       return NETDEV_TX_BUSY;
> 
>                 sparx5_set_port_ifh_rew_op(ifh, SPARX5_SKB_CB(skb)->rew_op);
>                 sparx5_set_port_ifh_pdu_type(ifh, SPARX5_SKB_CB(skb)->pdu_type);
> @@ -250,23 +249,31 @@ netdev_tx_t sparx5_port_xmit_impl(struct sk_buff *skb, struct net_device *dev)
>         else
>                 ret = sparx5_inject(sparx5, ifh, skb, dev);
> 
> -       if (ret == NETDEV_TX_OK) {
> -               stats->tx_bytes += skb->len;
> -               stats->tx_packets++;
> +       if (ret == -EBUSY)
> +               goto busy;
> +       if (ret < 0)
> +               goto drop;
> 
> -               if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> -                   SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> -                       return ret;
> +       stats->tx_bytes += skb->len;
> +       stats->tx_packets++;
> +       sparx5->tx.packets++;
> 
> -               dev_kfree_skb_any(skb);
> -       } else {
> -               stats->tx_dropped++;
> +       if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> +           SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> +               return NETDEV_TX_OK;
> 
> -               if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> -                   SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> -                       sparx5_ptp_txtstamp_release(port, skb);
> -       }
> -       return ret;
> +       dev_consume_skb_any(skb);
> +       return NETDEV_TX_OK;
> +drop:
> +       stats->tx_dropped++;
> +       sparx5->tx.dropped++;
> +       dev_kfree_skb_any(skb);
> +       return NETDEV_TX_OK;
> +busy:
> +       if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> +           SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> +               sparx5_ptp_txtstamp_release(port, skb);
> +       return NETDEV_TX_BUSY;
>  }
> 
>  static enum hrtimer_restart sparx5_injection_timeout(struct hrtimer *tmr)
> --
> 2.34.1
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index 66360c8c5a38..141897dfe388 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -317,7 +317,7 @@  int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
 	next_dcb_hw = sparx5_fdma_next_dcb(tx, tx->curr_entry);
 	db_hw = &next_dcb_hw->db[0];
 	if (!(db_hw->status & FDMA_DCB_STATUS_DONE))
-		tx->dropped++;
+		return -EINVAL;
 	db = list_first_entry(&tx->db_list, struct sparx5_db, list);
 	list_move_tail(&db->list, &tx->db_list);
 	next_dcb_hw->nextptr = FDMA_DCB_INVALID_DATA;
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
index 83c16ca5b30f..6db6ac6a3bbc 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
@@ -234,9 +234,8 @@  netdev_tx_t sparx5_port_xmit_impl(struct sk_buff *skb, struct net_device *dev)
 	sparx5_set_port_ifh(ifh, port->portno);
 
 	if (sparx5->ptp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
-		ret = sparx5_ptp_txtstamp_request(port, skb);
-		if (ret)
-			return ret;
+		if (sparx5_ptp_txtstamp_request(port, skb) < 0)
+			return NETDEV_TX_BUSY;
 
 		sparx5_set_port_ifh_rew_op(ifh, SPARX5_SKB_CB(skb)->rew_op);
 		sparx5_set_port_ifh_pdu_type(ifh, SPARX5_SKB_CB(skb)->pdu_type);
@@ -250,23 +249,31 @@  netdev_tx_t sparx5_port_xmit_impl(struct sk_buff *skb, struct net_device *dev)
 	else
 		ret = sparx5_inject(sparx5, ifh, skb, dev);
 
-	if (ret == NETDEV_TX_OK) {
-		stats->tx_bytes += skb->len;
-		stats->tx_packets++;
+	if (ret == -EBUSY)
+		goto busy;
+	if (ret < 0)
+		goto drop;
 
-		if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
-		    SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
-			return ret;
+	stats->tx_bytes += skb->len;
+	stats->tx_packets++;
+	sparx5->tx.packets++;
 
-		dev_kfree_skb_any(skb);
-	} else {
-		stats->tx_dropped++;
+	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
+	    SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
+		return NETDEV_TX_OK;
 
-		if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
-		    SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
-			sparx5_ptp_txtstamp_release(port, skb);
-	}
-	return ret;
+	dev_consume_skb_any(skb);
+	return NETDEV_TX_OK;
+drop:
+	stats->tx_dropped++;
+	sparx5->tx.dropped++;
+	dev_kfree_skb_any(skb);
+	return NETDEV_TX_OK;
+busy:
+	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
+	    SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
+		sparx5_ptp_txtstamp_release(port, skb);
+	return NETDEV_TX_BUSY;
 }
 
 static enum hrtimer_restart sparx5_injection_timeout(struct hrtimer *tmr)