Re: [RFC 6/9] staging: dpaa2-switch: add .ndo_start_xmit() callback

[Ioana Ciornei]

On Wed, Nov 04, 2020 at 11:27:00PM +0200, Vladimir Oltean wrote:
> On Wed, Nov 04, 2020 at 06:57:17PM +0200, Ioana Ciornei wrote:
> > From: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> > 
> > Implement the .ndo_start_xmit() callback for the switch port interfaces.
> > For each of the switch ports, gather the corresponding queue
> > destination ID (QDID) necessary for Tx enqueueing.
> > 
> > We'll reserve 64 bytes for software annotations, where we keep a skb
> > backpointer used on the Tx confirmation side for releasing the allocated
> > memory. At the moment, we only support linear skbs.
> > 
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> > ---
> > @@ -554,8 +560,10 @@ static int dpaa2_switch_port_open(struct net_device *netdev)
> >  	struct ethsw_core *ethsw = port_priv->ethsw_data;
> >  	int err;
> >  
> > -	/* No need to allow Tx as control interface is disabled */
> > -	netif_tx_stop_all_queues(netdev);
> > +	if (!dpaa2_switch_has_ctrl_if(port_priv->ethsw_data)) {
> > +		/* No need to allow Tx as control interface is disabled */
> > +		netif_tx_stop_all_queues(netdev);
> 
> Personal taste probably, but you could remove the braces here.

Usually checkpatch complains about this kind of thing but not this time.
Maybe it takes into account the comment as well..

I'll remove the braces.

> 
> > +	}
> >  
> >  	/* Explicitly set carrier off, otherwise
> >  	 * netif_carrier_ok() will return true and cause 'ip link show'
> > @@ -610,15 +618,6 @@ static int dpaa2_switch_port_stop(struct net_device *netdev)
> >  	return 0;
> >  }
> >  
> > +static netdev_tx_t dpaa2_switch_port_tx(struct sk_buff *skb,
> > +					struct net_device *net_dev)
> > +{
> > +	struct ethsw_port_priv *port_priv = netdev_priv(net_dev);
> > +	struct ethsw_core *ethsw = port_priv->ethsw_data;
> > +	int retries = DPAA2_SWITCH_SWP_BUSY_RETRIES;
> > +	struct dpaa2_fd fd;
> > +	int err;
> > +
> > +	if (!dpaa2_switch_has_ctrl_if(ethsw))
> > +		goto err_free_skb;
> > +
> > +	if (unlikely(skb_headroom(skb) < DPAA2_SWITCH_NEEDED_HEADROOM)) {
> > +		struct sk_buff *ns;
> > +
> > +		ns = skb_realloc_headroom(skb, DPAA2_SWITCH_NEEDED_HEADROOM);
> 
> Looks like this passion for skb_realloc_headroom runs in the company?

Not really, ocelot and sja1105 are safe :)

> Few other drivers use it, and Claudiu just had a bug with that in gianfar.
> Luckily what saves you from the same bug is the skb_unshare from right below.
> Maybe you could use skb_cow_head and simplify this a bit?
> 
> > +		if (unlikely(!ns)) {
> > +			netdev_err(net_dev, "Error reallocating skb headroom\n");
> > +			goto err_free_skb;
> > +		}
> > +		dev_kfree_skb(skb);
> 
> Please use dev_consume_skb_any here, as it's not error path. Or, if you
> use skb_cow_head, only the skb data will be reallocated, not the skb
> structure itself, so there will be no consume_skb in that case at all,
> another reason to simplify.

Ok, I can try that.

How dpaa2-eth deals now with this is to just create a S/G FD when the
headroom is less than what's necessary, so no skb_realloc_headroom() or
skb_cow_head(). But I agree that it's best to make it as simple as
possible.

> 
> > +		skb = ns;
> > +	}
> > +
> > +	/* We'll be holding a back-reference to the skb until Tx confirmation */
> > +	skb = skb_unshare(skb, GFP_ATOMIC);
> > +	if (unlikely(!skb)) {
> > +		/* skb_unshare() has already freed the skb */
> > +		netdev_err(net_dev, "Error copying the socket buffer\n");
> > +		goto err_exit;
> > +	}
> > +
> > +	if (skb_is_nonlinear(skb)) {
> > +		netdev_err(net_dev, "No support for non-linear SKBs!\n");
> 
> Rate-limit maybe?

Yep, that probably should be rate-limited.

> And what is the reason for no non-linear skb's? Too much code to copy
> from dpaa2-eth?

Once this is out of staging, dpaa2-eth and dpaa2-switch could share
the Tx/Rx code path so, as you said, I just didn't want to duplicate
everything if it's not specifically needed.

> > diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.h b/drivers/staging/fsl-dpaa2/ethsw/ethsw.h
> > index bd24be2c6308..b267c04e2008 100644
> > --- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.h
> > +++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.h
> > @@ -66,6 +66,19 @@
> >   */
> >  #define DPAA2_SWITCH_SWP_BUSY_RETRIES		1000
> >  
> > +/* Hardware annotation buffer size */
> > +#define DPAA2_SWITCH_HWA_SIZE		64
> > +/* Software annotation buffer size */
> > +#define DPAA2_SWITCH_SWA_SIZE		64
> > +
> > +#define DPAA2_SWITCH_TX_BUF_ALIGN	64
> 
> Could you align all of these to the "1000" from DPAA2_SWITCH_SWP_BUSY_RETRIES?
> 

Sure.

> > +
> > +#define DPAA2_SWITCH_TX_DATA_OFFSET \
> > +	(DPAA2_SWITCH_HWA_SIZE + DPAA2_SWITCH_SWA_SIZE)
> > +
> > +#define DPAA2_SWITCH_NEEDED_HEADROOM \
> > +	(DPAA2_SWITCH_TX_DATA_OFFSET + DPAA2_SWITCH_TX_BUF_ALIGN)
> > +
> 
> Ironically, you create a definition for NEEDED_HEADROOM but you do not
> set dev->needed_headroom.