Re: [PATCH net-next v3 06/12] net: dsa: allow switches to work without tagging

[Alexander Duyck <alexander.duyck@gmail.com>]

On 08/24/2014 11:44 AM, Florian Fainelli wrote:
> In case switch port tagging is disabled (voluntarily, or the switch just
> does not support it), allow us to continue using the master net_device
> operations instead of the tag-specific ones.
>
> dsa_protocol_is_tagged() is updated to check that the switch driver
> tag_protocol field is not 0 to avoid overriding skb->protocol in the
> eth_type_trans().
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v3:
> - update logic with the helpers introduced in the firs patch set
> - renamed dummy to notag
>
>  include/net/dsa.h |  2 +-
>  net/dsa/slave.c   | 25 ++++++++++++++++++++++++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 89b44e8d2dc3..6620d4722efa 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -206,7 +206,7 @@ static inline void *ds_to_priv(struct dsa_switch *ds)
>   */
>  static inline bool dsa_protocol_is_tagged(struct dsa_switch_tree *dst)
>  {
> -	return dst->tag_protocol != ETH_P_EDSA;
> +	return (dst->tag_protocol != 0) && (dst->tag_protocol != ETH_P_EDSA);
>  }
>  
>  static inline __be16 dsa_tag_protocol(struct dsa_switch_tree *dst)
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index b4b6d457bfd0..35cd8b7a56b8 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -332,6 +332,28 @@ static const struct net_device_ops trailer_netdev_ops = {
>  };
>  #endif
>  
> +static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb,
> +				  struct net_device *dev)
> +{
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +
> +	skb->dev = p->parent->dst->master_netdev;
> +	dev_queue_xmit(skb);
> +
> +	return NETDEV_TX_OK;
> +}
> +
> +static const struct net_device_ops notag_netdev_ops = {
> +	.ndo_init		= dsa_slave_init,
> +	.ndo_open		= dsa_slave_open,
> +	.ndo_stop		= dsa_slave_close,
> +	.ndo_start_xmit		= dsa_slave_xmit,
> +	.ndo_change_rx_flags	= dsa_slave_change_rx_flags,
> +	.ndo_set_rx_mode	= dsa_slave_set_rx_mode,
> +	.ndo_set_mac_address	= dsa_slave_set_mac_address,
> +	.ndo_do_ioctl		= dsa_slave_ioctl,
> +};
> +
>  static void dsa_slave_adjust_link(struct net_device *dev)
>  {
>  	struct dsa_slave_priv *p = netdev_priv(dev);
> @@ -432,7 +454,8 @@ dsa_slave_create(struct dsa_switch *ds, struct device *parent,
>  		break;
>  #endif
>  	default:
> -		BUG();
> +		slave_dev->netdev_ops = &notag_netdev_ops;
> +		break;
>  	}
>  
>  	SET_NETDEV_DEV(slave_dev, parent);

Actually the more I look at this the more I think it might make sense to
just have a pair of function pointers, maybe even a header ops structure
that is maintained in the dsa_switch_tree for xmit and rcv.  Then
instead of having to define 4 slaves each with only one function
different between them you could have just one function that calls the
xmit function pointer contained in the dsa_switch_tree structure.

Thanks,

Alex