* [PATCH net] net: microchip: sparx5: correctly free skb in xmit
@ 2022-11-16 9:57 Casper Andersson
2022-11-16 11:41 ` Horatiu Vultur
0 siblings, 1 reply; 4+ messages in thread
From: Casper Andersson @ 2022-11-16 9:57 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Lars Povlsen, Steen Hegelund, Daniel Machon, UNGLinuxDriver,
Richard Cochran, netdev, Horatiu Vultur
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.
https://lore.kernel.org/netdev/20220920072948.33c25dd2@kernel.org/t/#mdb821eb507a207dd5e27683239ffa7ec7199421a
Fixes: 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA functionality")
Signed-off-by: Casper Andersson <casper.casan@gmail.com>
---
I am not entirely sure about the following construct which is present in
both sparx5 and lan966x drivers and returns before consuming the skb. Is
there any reason it does not free the skb?
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;
.../ethernet/microchip/sparx5/sparx5_fdma.c | 2 +-
.../ethernet/microchip/sparx5/sparx5_main.h | 2 +-
.../ethernet/microchip/sparx5/sparx5_packet.c | 47 ++++++++++---------
3 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index 66360c8c5a38..302e7ff55585 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -306,7 +306,7 @@ static struct sparx5_tx_dcb_hw *sparx5_fdma_next_dcb(struct sparx5_tx *tx,
return next_dcb;
}
-int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
+netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
{
struct sparx5_tx_dcb_hw *next_dcb_hw;
struct sparx5_tx *tx = &sparx5->tx;
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index 7a83222caa73..34b8d11f76df 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -312,7 +312,7 @@ void sparx5_port_inj_timer_setup(struct sparx5_port *port);
/* sparx5_fdma.c */
int sparx5_fdma_start(struct sparx5 *sparx5);
int sparx5_fdma_stop(struct sparx5 *sparx5);
-int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
+netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
irqreturn_t sparx5_fdma_handler(int irq, void *args);
/* sparx5_mactable.c */
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
index 83c16ca5b30f..6fc1c1e410f6 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
@@ -159,10 +159,10 @@ static void sparx5_xtr_grp(struct sparx5 *sparx5, u8 grp, bool byte_swap)
netif_rx(skb);
}
-static int sparx5_inject(struct sparx5 *sparx5,
- u32 *ifh,
- struct sk_buff *skb,
- struct net_device *ndev)
+static netdev_tx_t sparx5_inject(struct sparx5 *sparx5,
+ u32 *ifh,
+ struct sk_buff *skb,
+ struct net_device *ndev)
{
int grp = INJ_QUEUE;
u32 val, w, count;
@@ -172,7 +172,7 @@ static int sparx5_inject(struct sparx5 *sparx5,
if (!(QS_INJ_STATUS_FIFO_RDY_GET(val) & BIT(grp))) {
pr_err_ratelimited("Injection: Queue not ready: 0x%lx\n",
QS_INJ_STATUS_FIFO_RDY_GET(val));
- return -EBUSY;
+ return NETDEV_TX_BUSY;
}
/* Indicate SOF */
@@ -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))
+ goto drop;
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,27 @@ 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 == NETDEV_TX_BUSY)
+ goto busy;
- 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++;
- 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++;
+ 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: microchip: sparx5: correctly free skb in xmit
2022-11-16 9:57 [PATCH net] net: microchip: sparx5: correctly free skb in xmit Casper Andersson
@ 2022-11-16 11:41 ` Horatiu Vultur
2022-11-16 12:34 ` Casper Andersson
0 siblings, 1 reply; 4+ messages in thread
From: Horatiu Vultur @ 2022-11-16 11:41 UTC (permalink / raw)
To: Casper Andersson
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Lars Povlsen, Steen Hegelund, Daniel Machon, UNGLinuxDriver,
Richard Cochran, netdev
The 11/16/2022 10:57, Casper Andersson wrote:
Hi Casper,
>
> 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.
> https://lore.kernel.org/netdev/20220920072948.33c25dd2@kernel.org/t/#mdb821eb507a207dd5e27683239ffa7ec7199421a
>
> Fixes: 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA functionality")
Actually the fix shouldn't be:
70dfe25cd866 ("net: sparx5: Update extraction/injection for timestamping") ?
Because this commit introduced the function sparx5_ptp_txtstamp_request,
and it didn't free the skb on error path.
> Signed-off-by: Casper Andersson <casper.casan@gmail.com>
> ---
> I am not entirely sure about the following construct which is present in
> both sparx5 and lan966x drivers and returns before consuming the skb. Is
> there any reason it does not free the skb?
Yes, this case will happen when the HW is configured to timestamp frames
and return the timestamp back to the SW. And in that case the frame is freed
once the timestamp is received. Have a look in the function
'sparx5_ptp_irq_handler'
>
> 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;
>
>
>
>
> .../ethernet/microchip/sparx5/sparx5_fdma.c | 2 +-
> .../ethernet/microchip/sparx5/sparx5_main.h | 2 +-
> .../ethernet/microchip/sparx5/sparx5_packet.c | 47 ++++++++++---------
> 3 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> index 66360c8c5a38..302e7ff55585 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> @@ -306,7 +306,7 @@ static struct sparx5_tx_dcb_hw *sparx5_fdma_next_dcb(struct sparx5_tx *tx,
> return next_dcb;
> }
>
> -int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
> +netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
> {
> struct sparx5_tx_dcb_hw *next_dcb_hw;
> struct sparx5_tx *tx = &sparx5->tx;
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> index 7a83222caa73..34b8d11f76df 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> @@ -312,7 +312,7 @@ void sparx5_port_inj_timer_setup(struct sparx5_port *port);
> /* sparx5_fdma.c */
> int sparx5_fdma_start(struct sparx5 *sparx5);
> int sparx5_fdma_stop(struct sparx5 *sparx5);
> -int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
> +netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
> irqreturn_t sparx5_fdma_handler(int irq, void *args);
>
> /* sparx5_mactable.c */
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> index 83c16ca5b30f..6fc1c1e410f6 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> @@ -159,10 +159,10 @@ static void sparx5_xtr_grp(struct sparx5 *sparx5, u8 grp, bool byte_swap)
> netif_rx(skb);
> }
>
> -static int sparx5_inject(struct sparx5 *sparx5,
> - u32 *ifh,
> - struct sk_buff *skb,
> - struct net_device *ndev)
> +static netdev_tx_t sparx5_inject(struct sparx5 *sparx5,
> + u32 *ifh,
> + struct sk_buff *skb,
> + struct net_device *ndev)
> {
> int grp = INJ_QUEUE;
> u32 val, w, count;
> @@ -172,7 +172,7 @@ static int sparx5_inject(struct sparx5 *sparx5,
> if (!(QS_INJ_STATUS_FIFO_RDY_GET(val) & BIT(grp))) {
> pr_err_ratelimited("Injection: Queue not ready: 0x%lx\n",
> QS_INJ_STATUS_FIFO_RDY_GET(val));
> - return -EBUSY;
> + return NETDEV_TX_BUSY;
> }
>
> /* Indicate SOF */
> @@ -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))
> + goto drop;
Is it not possible to return busy also here?
The reason why I ask this is because of the following case.
Imagine you need to send a frame and timestamp it, but the HW
currrently don't have enough slots to store the TX timestamp. In that
case is better to return BUSY because maybe in short time there will be
slots. Than just dropping the frame, because if the frame is dropped,
then there is no chance to get the TX timestamp, and who needs this will
stop working.
>
> 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,27 @@ 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 == NETDEV_TX_BUSY)
> + goto busy;
>
> - 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++;
>
> - 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++;
> + 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
>
--
/Horatiu
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: microchip: sparx5: correctly free skb in xmit
2022-11-16 11:41 ` Horatiu Vultur
@ 2022-11-16 12:34 ` Casper Andersson
2022-11-17 12:22 ` Horatiu Vultur
0 siblings, 1 reply; 4+ messages in thread
From: Casper Andersson @ 2022-11-16 12:34 UTC (permalink / raw)
To: Horatiu Vultur
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Lars Povlsen, Steen Hegelund, Daniel Machon, UNGLinuxDriver,
Richard Cochran, netdev
Hi Horatiu,
On 2022-11-16 12:41, Horatiu Vultur wrote:
> The 11/16/2022 10:57, Casper Andersson wrote:
>
> Hi Casper,
>
> >
> > 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.
> > https://lore.kernel.org/netdev/20220920072948.33c25dd2@kernel.org/t/#mdb821eb507a207dd5e27683239ffa7ec7199421a
> >
> > Fixes: 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA functionality")
>
> Actually the fix shouldn't be:
> 70dfe25cd866 ("net: sparx5: Update extraction/injection for timestamping") ?
> Because this commit introduced the function sparx5_ptp_txtstamp_request,
> and it didn't free the skb on error path.
>
As I understood Jakub's comment (linked above) part of the issue was
that sparx5_inject (and sparx5_ptp_txtstamp_request) returns -EBUSY
instead of NETDEV_TX_BUSY (which then gets returned further). So maybe
it should be separate patches for the two cases.
While on the topic, I noticed another possible issue with
sparx5_fdma_xmit where it increments dropped, but keeps going with
transmit. I assume it should return and indicate dropped here.
if (!(db_hw->status & FDMA_DCB_STATUS_DONE))
tx->dropped++;
db = list_first_entry(&tx->db_list, struct sparx5_db, list);
...
> > Signed-off-by: Casper Andersson <casper.casan@gmail.com>
> > ---
> > I am not entirely sure about the following construct which is present in
> > both sparx5 and lan966x drivers and returns before consuming the skb. Is
> > there any reason it does not free the skb?
>
> Yes, this case will happen when the HW is configured to timestamp frames
> and return the timestamp back to the SW. And in that case the frame is freed
> once the timestamp is received. Have a look in the function
> 'sparx5_ptp_irq_handler'
Alright, that makes sense.
>
> >
> > 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;
> >
> >
> >
> >
> > .../ethernet/microchip/sparx5/sparx5_fdma.c | 2 +-
> > .../ethernet/microchip/sparx5/sparx5_main.h | 2 +-
> > .../ethernet/microchip/sparx5/sparx5_packet.c | 47 ++++++++++---------
> > 3 files changed, 27 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > index 66360c8c5a38..302e7ff55585 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > @@ -306,7 +306,7 @@ static struct sparx5_tx_dcb_hw *sparx5_fdma_next_dcb(struct sparx5_tx *tx,
> > return next_dcb;
> > }
> >
> > -int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
> > +netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
> > {
> > struct sparx5_tx_dcb_hw *next_dcb_hw;
> > struct sparx5_tx *tx = &sparx5->tx;
> > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> > index 7a83222caa73..34b8d11f76df 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> > @@ -312,7 +312,7 @@ void sparx5_port_inj_timer_setup(struct sparx5_port *port);
> > /* sparx5_fdma.c */
> > int sparx5_fdma_start(struct sparx5 *sparx5);
> > int sparx5_fdma_stop(struct sparx5 *sparx5);
> > -int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
> > +netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
> > irqreturn_t sparx5_fdma_handler(int irq, void *args);
> >
> > /* sparx5_mactable.c */
> > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> > index 83c16ca5b30f..6fc1c1e410f6 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> > @@ -159,10 +159,10 @@ static void sparx5_xtr_grp(struct sparx5 *sparx5, u8 grp, bool byte_swap)
> > netif_rx(skb);
> > }
> >
> > -static int sparx5_inject(struct sparx5 *sparx5,
> > - u32 *ifh,
> > - struct sk_buff *skb,
> > - struct net_device *ndev)
> > +static netdev_tx_t sparx5_inject(struct sparx5 *sparx5,
> > + u32 *ifh,
> > + struct sk_buff *skb,
> > + struct net_device *ndev)
> > {
> > int grp = INJ_QUEUE;
> > u32 val, w, count;
> > @@ -172,7 +172,7 @@ static int sparx5_inject(struct sparx5 *sparx5,
> > if (!(QS_INJ_STATUS_FIFO_RDY_GET(val) & BIT(grp))) {
> > pr_err_ratelimited("Injection: Queue not ready: 0x%lx\n",
> > QS_INJ_STATUS_FIFO_RDY_GET(val));
> > - return -EBUSY;
> > + return NETDEV_TX_BUSY;
> > }
> >
> > /* Indicate SOF */
> > @@ -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))
> > + goto drop;
>
> Is it not possible to return busy also here?
> The reason why I ask this is because of the following case.
> Imagine you need to send a frame and timestamp it, but the HW
> currrently don't have enough slots to store the TX timestamp. In that
> case is better to return BUSY because maybe in short time there will be
> slots. Than just dropping the frame, because if the frame is dropped,
> then there is no chance to get the TX timestamp, and who needs this will
> stop working.
Yes, we can do that. Thanks for clarifying, I hadn't quite understood
the PTP parts of this.
>
> >
> > 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,27 @@ 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 == NETDEV_TX_BUSY)
> > + goto busy;
> >
> > - 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++;
> >
> > - 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++;
> > + 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
> >
>
> --
> /Horatiu
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: microchip: sparx5: correctly free skb in xmit
2022-11-16 12:34 ` Casper Andersson
@ 2022-11-17 12:22 ` Horatiu Vultur
0 siblings, 0 replies; 4+ messages in thread
From: Horatiu Vultur @ 2022-11-17 12:22 UTC (permalink / raw)
To: Casper Andersson
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Lars Povlsen, Steen Hegelund, Daniel Machon, UNGLinuxDriver,
Richard Cochran, netdev
The 11/16/2022 13:34, Casper Andersson wrote:
>
> Hi Horatiu,
>
> On 2022-11-16 12:41, Horatiu Vultur wrote:
> > The 11/16/2022 10:57, Casper Andersson wrote:
> >
> > Hi Casper,
> >
> > >
> > > 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.
> > > https://lore.kernel.org/netdev/20220920072948.33c25dd2@kernel.org/t/#mdb821eb507a207dd5e27683239ffa7ec7199421a
> > >
> > > Fixes: 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA functionality")
> >
> > Actually the fix shouldn't be:
> > 70dfe25cd866 ("net: sparx5: Update extraction/injection for timestamping") ?
> > Because this commit introduced the function sparx5_ptp_txtstamp_request,
> > and it didn't free the skb on error path.
> >
>
> As I understood Jakub's comment (linked above) part of the issue was
> that sparx5_inject (and sparx5_ptp_txtstamp_request) returns -EBUSY
> instead of NETDEV_TX_BUSY (which then gets returned further). So maybe
> it should be separate patches for the two cases.
Ah.. I think now I understand it. It is up to you how to take this.
>
> While on the topic, I noticed another possible issue with
> sparx5_fdma_xmit where it increments dropped, but keeps going with
> transmit. I assume it should return and indicate dropped here.
>
> if (!(db_hw->status & FDMA_DCB_STATUS_DONE))
> tx->dropped++;
> db = list_first_entry(&tx->db_list, struct sparx5_db, list);
You are correct, that looks wrong also to me.
It should return and drop the frame.
> ...
>
> > > Signed-off-by: Casper Andersson <casper.casan@gmail.com>
> > > ---
> > > I am not entirely sure about the following construct which is present in
> > > both sparx5 and lan966x drivers and returns before consuming the skb. Is
> > > there any reason it does not free the skb?
> >
> > Yes, this case will happen when the HW is configured to timestamp frames
> > and return the timestamp back to the SW. And in that case the frame is freed
> > once the timestamp is received. Have a look in the function
> > 'sparx5_ptp_irq_handler'
>
> Alright, that makes sense.
>
> >
> > >
> > > 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;
> > >
> > >
> > >
> > >
> > > .../ethernet/microchip/sparx5/sparx5_fdma.c | 2 +-
> > > .../ethernet/microchip/sparx5/sparx5_main.h | 2 +-
> > > .../ethernet/microchip/sparx5/sparx5_packet.c | 47 ++++++++++---------
> > > 3 files changed, 27 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > > index 66360c8c5a38..302e7ff55585 100644
> > > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > > @@ -306,7 +306,7 @@ static struct sparx5_tx_dcb_hw *sparx5_fdma_next_dcb(struct sparx5_tx *tx,
> > > return next_dcb;
> > > }
> > >
> > > -int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
> > > +netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
> > > {
> > > struct sparx5_tx_dcb_hw *next_dcb_hw;
> > > struct sparx5_tx *tx = &sparx5->tx;
> > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> > > index 7a83222caa73..34b8d11f76df 100644
> > > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> > > @@ -312,7 +312,7 @@ void sparx5_port_inj_timer_setup(struct sparx5_port *port);
> > > /* sparx5_fdma.c */
> > > int sparx5_fdma_start(struct sparx5 *sparx5);
> > > int sparx5_fdma_stop(struct sparx5 *sparx5);
> > > -int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
> > > +netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
> > > irqreturn_t sparx5_fdma_handler(int irq, void *args);
> > >
> > > /* sparx5_mactable.c */
> > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> > > index 83c16ca5b30f..6fc1c1e410f6 100644
> > > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> > > @@ -159,10 +159,10 @@ static void sparx5_xtr_grp(struct sparx5 *sparx5, u8 grp, bool byte_swap)
> > > netif_rx(skb);
> > > }
> > >
> > > -static int sparx5_inject(struct sparx5 *sparx5,
> > > - u32 *ifh,
> > > - struct sk_buff *skb,
> > > - struct net_device *ndev)
> > > +static netdev_tx_t sparx5_inject(struct sparx5 *sparx5,
> > > + u32 *ifh,
> > > + struct sk_buff *skb,
> > > + struct net_device *ndev)
> > > {
> > > int grp = INJ_QUEUE;
> > > u32 val, w, count;
> > > @@ -172,7 +172,7 @@ static int sparx5_inject(struct sparx5 *sparx5,
> > > if (!(QS_INJ_STATUS_FIFO_RDY_GET(val) & BIT(grp))) {
> > > pr_err_ratelimited("Injection: Queue not ready: 0x%lx\n",
> > > QS_INJ_STATUS_FIFO_RDY_GET(val));
> > > - return -EBUSY;
> > > + return NETDEV_TX_BUSY;
> > > }
> > >
> > > /* Indicate SOF */
> > > @@ -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))
> > > + goto drop;
> >
> > Is it not possible to return busy also here?
> > The reason why I ask this is because of the following case.
> > Imagine you need to send a frame and timestamp it, but the HW
> > currrently don't have enough slots to store the TX timestamp. In that
> > case is better to return BUSY because maybe in short time there will be
> > slots. Than just dropping the frame, because if the frame is dropped,
> > then there is no chance to get the TX timestamp, and who needs this will
> > stop working.
>
> Yes, we can do that. Thanks for clarifying, I hadn't quite understood
> the PTP parts of this.
>
> >
> > >
> > > 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,27 @@ 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 == NETDEV_TX_BUSY)
> > > + goto busy;
> > >
> > > - 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++;
> > >
> > > - 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++;
> > > + 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
> > >
> >
> > --
> > /Horatiu
--
/Horatiu
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-17 12:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-16 9:57 [PATCH net] net: microchip: sparx5: correctly free skb in xmit Casper Andersson
2022-11-16 11:41 ` Horatiu Vultur
2022-11-16 12:34 ` Casper Andersson
2022-11-17 12:22 ` Horatiu Vultur
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox