On Wed, Nov 04, 2020 at 06:57:17PM +0200, Ioana Ciornei wrote:
> From: Ioana Ciornei <ioana.cior...@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.cior...@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.
