Re: [PATCH net] net: microchip: sparx5: correctly free skb in xmit

[Casper Andersson <casper.casan@gmail.com>]

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