Re: [RFC PATCH net-next 03/13] net: dsa: Create a more convenient function for installing port VLANs

[Florian Fainelli <f.fainelli@gmail.com>]

On 3/23/19 8:23 PM, Vladimir Oltean wrote:
> This refactors the two-phase transaction from dsa_slave_vlan_rx_add_vid
> and also makes that code available for other functions from within DSA.
> The newly exposed function either adds or deletes the specified VLAN
> entry based on a boolean argument.
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

The name of the function does not make it particularly clear that
passing false results in deleting the VLAN. Can you just wrap this under
a different function name that is only doing the two-step adding of
VLANs and keep using dsa_port_vlan_del() explicitly when you want to
remove a VLAN?

> ---
>  net/dsa/dsa_priv.h |  2 ++
>  net/dsa/port.c     | 24 ++++++++++++++++++++++++
>  net/dsa/slave.c    | 16 ++--------------
>  3 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 093b7d145eb1..8048ced3708f 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -164,6 +164,8 @@ int dsa_port_pre_bridge_flags(const struct dsa_port *dp, unsigned long flags,
>  			      struct switchdev_trans *trans);
>  int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
>  			  struct switchdev_trans *trans);
> +int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags,
> +			      bool enabled);
>  int dsa_port_vlan_add(struct dsa_port *dp,
>  		      const struct switchdev_obj_port_vlan *vlan,
>  		      struct switchdev_trans *trans);
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index a86fe3be1261..9c7358f98004 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -326,6 +326,30 @@ int dsa_port_vlan_del(struct dsa_port *dp,
>  	return 0;
>  }
>  
> +int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags,
> +			      bool enabled)
> +{
> +	struct switchdev_obj_port_vlan vlan = {
> +		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
> +		.flags = flags,
> +		.vid_begin = vid,
> +		.vid_end = vid,
> +	};
> +	struct switchdev_trans trans;
> +	int err;
> +
> +	if (!enabled)
> +		return dsa_port_vlan_del(dp, &vlan);
> +
> +	trans.ph_prepare = true;
> +	err = dsa_port_vlan_add(dp, &vlan, &trans);
> +	if (err == -EOPNOTSUPP)
> +		return 0;
> +
> +	trans.ph_prepare = false;
> +	return dsa_port_vlan_add(dp, &vlan, &trans);
> +}
> +
>  static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
>  {
>  	struct device_node *phy_dn;
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 093eef6f2599..3191ef74f6a1 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -987,13 +987,6 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>  				     u16 vid)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> -	struct switchdev_obj_port_vlan vlan = {
> -		.vid_begin = vid,
> -		.vid_end = vid,
> -		/* This API only allows programming tagged, non-PVID VIDs */
> -		.flags = 0,
> -	};
> -	struct switchdev_trans trans;
>  	struct bridge_vlan_info info;
>  	int ret;
>  
> @@ -1010,13 +1003,8 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>  			return -EBUSY;
>  	}
>  
> -	trans.ph_prepare = true;
> -	ret = dsa_port_vlan_add(dp, &vlan, &trans);
> -	if (ret == -EOPNOTSUPP)
> -		return 0;
> -
> -	trans.ph_prepare = false;
> -	return dsa_port_vlan_add(dp, &vlan, &trans);
> +	/* This API only allows programming tagged, non-PVID VIDs */
> +	return dsa_port_trans_vlan_apply(dp, vid, 0, true);
>  }
>  
>  static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
> 


-- 
Florian