Messages in this thread | | | Date | Wed, 4 Nov 2020 23:27:00 +0200 | From | Vladimir Oltean <> | Subject | Re: [RFC 6/9] staging: dpaa2-switch: add .ndo_start_xmit() callback |
| |
On Wed, Nov 04, 2020 at 06:57:17PM +0200, Ioana Ciornei wrote: > From: Ioana Ciornei <ioana.ciornei@nxp.com> > > 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@nxp.com> > --- > @@ -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.
> + } > > /* 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? 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.
> + 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? And what is the reason for no non-linear skb's? Too much code to copy from dpaa2-eth?
> + goto err_free_skb; > + } > + > + err = dpaa2_switch_build_single_fd(ethsw, skb, &fd); > + if (unlikely(err)) { > + netdev_err(net_dev, "ethsw_build_*_fd() %d\n", err); > + goto err_free_skb; > + } > + > + do { > + err = dpaa2_io_service_enqueue_qd(NULL, > + port_priv->tx_qdid, > + 8, 0, &fd); > + retries--; > + } while (err == -EBUSY && retries); > + > + if (unlikely(err < 0)) { > + dpaa2_switch_free_fd(ethsw, &fd); > + goto err_exit; > + } > + > + return NETDEV_TX_OK; > + > +err_free_skb: > + dev_kfree_skb(skb); > +err_exit: > + return NETDEV_TX_OK; > +} > + > 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?
> + > +#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.
| |