Re: [PATCH net-next 04/18] switchdev: convert STP update to swdev attr set

[roopa <roopa@cumulusnetworks.com>]

On 3/30/15, 4:54 AM, Jiri Pirko wrote:
> Mon, Mar 30, 2015 at 10:40:22AM CEST, sfeldma@gmail.com wrote:
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> STP update is just a writable port attribute, so convert swdev_port_stp_update
>> to an attr set.
>>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> ---
>> drivers/net/ethernet/rocker/rocker.c |   16 +++++++++++++---
>> include/net/switchdev.h              |   13 ++-----------
>> net/bridge/br_stp.c                  |    6 +++++-
>> net/dsa/slave.c                      |   19 ++++++++++++++++++-
>> net/switchdev/switchdev.c            |   28 ----------------------------
>> 5 files changed, 38 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
>> index 4bcc555..224f91d 100644
>> --- a/drivers/net/ethernet/rocker/rocker.c
>> +++ b/drivers/net/ethernet/rocker/rocker.c
>> @@ -4237,11 +4237,21 @@ static int rocker_port_attr_get(struct net_device *dev, struct swdev_attr *attr)
>> 	return 0;
>> }
>>
>> -static int rocker_port_swdev_port_stp_update(struct net_device *dev, u8 state)
>> +static int rocker_port_attr_set(struct net_device *dev, struct swdev_attr *attr)
>> {
>> 	struct rocker_port *rocker_port = netdev_priv(dev);
>> +	int err = 0;
>> +
>> +	switch (attr->attr) {
>> +	case SWDEV_ATTR_PORT_STP_STATE:
>> +		err = rocker_port_stp_update(rocker_port, attr->stp_state);
>> +		break;
>> +	default:
>> +		err = -EOPNOTSUPP;
>> +		break;
>> +	}
>>
>> -	return rocker_port_stp_update(rocker_port, state);
>> +	return err;
>> }
>>
>> static int rocker_port_swdev_fib_ipv4_add(struct net_device *dev,
>> @@ -4271,7 +4281,7 @@ static int rocker_port_swdev_fib_ipv4_del(struct net_device *dev,
>>
>> static const struct swdev_ops rocker_port_swdev_ops = {
>> 	.swdev_port_attr_get		= rocker_port_attr_get,
>> -	.swdev_port_stp_update		= rocker_port_swdev_port_stp_update,
>> +	.swdev_port_attr_set		= rocker_port_attr_set,
>> 	.swdev_fib_ipv4_add		= rocker_port_swdev_fib_ipv4_add,
>> 	.swdev_fib_ipv4_del		= rocker_port_swdev_fib_ipv4_del,
>> };
>> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> index fd36b5c..99050b0 100644
>> --- a/include/net/switchdev.h
>> +++ b/include/net/switchdev.h
>> @@ -17,6 +17,7 @@
>> enum swdev_attr_id {
>> 	SWDEV_ATTR_UNDEFINED,
>> 	SWDEV_ATTR_PORT_PARENT_ID,
>> +	SWDEV_ATTR_PORT_STP_STATE,
>> };
>>
>> #define SWDEV_ATTR_F_NO_RECURSE		BIT(0)
>> @@ -27,6 +28,7 @@ struct swdev_attr {
>> 	u32 flags;
>> 	union {
>> 		struct netdev_phys_item_id ppid;	/* PORT_PARENT_ID */
>> +		u8 stp_state;				/* PORT_STP_STATE */
>> 	};
>> };
>>
>> @@ -39,9 +41,6 @@ struct fib_info;
>>   *
>>   * @swdev_port_attr_set: Set a port attribute (see swdev_attr).
>>   *
>> - * @swdev_port_stp_update: Called to notify switch device port of bridge
>> - *   port STP state change.
>> - *
>>   * @swdev_fib_ipv4_add: Called to add/modify IPv4 route to switch device.
>>   *
>>   * @swdev_fib_ipv4_del: Called to delete IPv4 route from switch device.
>> @@ -51,7 +50,6 @@ struct swdev_ops {
>> 				       struct swdev_attr *attr);
>> 	int	(*swdev_port_attr_set)(struct net_device *dev,
>> 				       struct swdev_attr *attr);
>> -	int	(*swdev_port_stp_update)(struct net_device *dev, u8 state);
>> 	int	(*swdev_fib_ipv4_add)(struct net_device *dev, __be32 dst,
>> 				      int dst_len, struct fib_info *fi,
>> 				      u8 tos, u8 type, u32 nlflags,
>> @@ -86,7 +84,6 @@ netdev_switch_notifier_info_to_dev(const struct netdev_switch_notifier_info *inf
>>
>> int swdev_port_attr_get(struct net_device *dev, struct swdev_attr *attr);
>> int swdev_port_attr_set(struct net_device *dev, struct swdev_attr *attr);
>> -int netdev_switch_port_stp_update(struct net_device *dev, u8 state);
>> int register_netdev_switch_notifier(struct notifier_block *nb);
>> int unregister_netdev_switch_notifier(struct notifier_block *nb);
>> int call_netdev_switch_notifiers(unsigned long val, struct net_device *dev,
>> @@ -119,12 +116,6 @@ static inline int swdev_port_attr_set(struct net_device *dev,
>> 	return -EOPNOTSUPP;
>> }
>>
>> -static inline int netdev_switch_port_stp_update(struct net_device *dev,
>> -						u8 state)
>> -{
>> -	return -EOPNOTSUPP;
>> -}
>> -
>> static inline int register_netdev_switch_notifier(struct notifier_block *nb)
>> {
>> 	return 0;
>> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
>> index fb3ebe6..8b2ef23 100644
>> --- a/net/bridge/br_stp.c
>> +++ b/net/bridge/br_stp.c
>> @@ -39,10 +39,14 @@ void br_log_state(const struct net_bridge_port *p)
>>
>> void br_set_state(struct net_bridge_port *p, unsigned int state)
>> {
>> +	struct swdev_attr attr = {
>> +		.attr = SWDEV_ATTR_PORT_STP_STATE,
>> +		.stp_state = state,
>> +	};
>> 	int err;
>>
>> 	p->state = state;
>> -	err = netdev_switch_port_stp_update(p->dev, state);
>> +	err = swdev_port_attr_set(p->dev, &attr);
> Seems nicer to me to provide a wrapper for callers so they don't call
> directly swdev_port_attr_set and have no knowledge of swdev_attr.
+1. I had a similar patch long back. swdev_port_attr_set can take a 
attrname and void argument.
the caller only needs to know the attrname in that case.

I had used netlink attribute names at that time to not have another set 
of duplicate attribute names in the swdev layer. But then the api has to 
be called swdev_bridge_port_attr_set to match the bridge attribute 
policy name.
but, i have no objections to having  the new attribute names 
SWDEV_ATTR_PORT_*