diff mbox series

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

Message ID 20190324032346.32394-4-olteanv@gmail.com
State RFC
Delegated to: David Miller
Headers show
Series NXP SJA1105 DSA driver | expand

Commit Message

Vladimir Oltean March 24, 2019, 3:23 a.m. UTC 
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>
---
 net/dsa/dsa_priv.h |  2 ++
 net/dsa/port.c     | 24 ++++++++++++++++++++++++
 net/dsa/slave.c    | 16 ++--------------
 3 files changed, 28 insertions(+), 14 deletions(-)

Comments

Florian Fainelli March 25, 2019, 5:06 p.m. UTC | #1 
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,
>
Vladimir Oltean March 27, 2019, 12:31 a.m. UTC | #2 
On 3/25/19 7:06 PM, Florian Fainelli wrote:
> 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?
> 

The reason why I made it this way was mainly to make the 
switchdev_obj_port_vlan struct an implementation detail. That being so I 
wouldn't need to keep and continuously modify this struct during the 3 
times I'm calling the function from within the 05/13 patch ("net: dsa: 
Optional VLAN-based port separation for switches without tagging"). I 
did try to make use of the .vid_begin/.vid_end feature and batch some of 
the function calls, but doing so would restrict possible values that the 
rx_vid and tx_vid functions may return - dsa_port_setup_8021q_tagging() 
would need to ensure that they are contiguous prior to batching them 
into a single vlan object. By the way, I don't think anybody is making 
any good use of this feature, it's just creating useless boilerplate as 
of now.
The other thing is that if I were to wrap around dsa_port_vlan_add() and 
get rid of the switchdev_obj_port_vlan and just pass the vid as u16, I'd 
have to do the same wrapping for the dsa_port_vlan_del() function too. 
Plus I'd have to keep 3 'if' conditions just to decide whether to call 
*_add or *_del.

> diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
> index 221299b264f5..1b11b245e2d6 100644
> --- a/net/dsa/tag_8021q.c
> +++ b/net/dsa/tag_8021q.c
> @@ -120,8 +120,10 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>  			/* The Rx VID is a regular VLAN on all others */
>  			flags = BRIDGE_VLAN_INFO_UNTAGGED;
>  
> -		err = dsa_port_trans_vlan_apply(other_dp, rx_vid, flags,
> -						enabled);
> +		if (enabled)
> +			err = __dsa_port_vlan_add(other_dp, rx_vid, flags);
> +		else
> +			err = __dsa_port_vlan_del(other_dp, rx_vid);
>  		if (err) {
>  			dev_err(ds->dev, "Failed to apply Rx VID %d to port %d: %d\n",
>  				rx_vid, port, err);
> @@ -129,14 +131,20 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>  		}
>  	}
>  	/* Finally apply the Tx VID on this port and on the CPU port */
> -	err = dsa_port_trans_vlan_apply(dp, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED,
> -					enabled);
> +	if (enabled)
> +		err = __dsa_port_vlan_add(dp, tx_vid,
> +					  BRIDGE_VLAN_INFO_UNTAGGED);
> +	else
> +		err = __dsa_port_vlan_del(dp, tx_vid);
>  	if (err) {
>  		dev_err(ds->dev, "Failed to apply Tx VID %d on port %d: %d\n",
>  			tx_vid, port, err);
>  		return err;
>  	}
> -	err = dsa_port_trans_vlan_apply(upstream_dp, tx_vid, 0, enabled);
> +	if (enabled)
> +		err = __dsa_port_vlan_add(upstream_dp, tx_vid, 0);
> +	else
> +		err = __dsa_port_vlan_del(upstream_dp, tx_vid);
>  	if (err) {
>  		dev_err(ds->dev, "Failed to apply Tx VID %d on port %d: %d\n",
>  			tx_vid, upstream, err);

How does something like the above look like?

Thanks,
-Vladimir

>> ---
>>   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,
>>
> 
>
diff mbox series

Patch

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,