From mboxrd@z Thu Jan 1 00:00:00 1970 From: roopa Subject: Re: [PATCH net-next 04/18] switchdev: convert STP update to swdev attr set Date: Mon, 30 Mar 2015 06:47:10 -0700 Message-ID: <551953DE.90702@cumulusnetworks.com> References: <1427704836-8776-1-git-send-email-sfeldma@gmail.com> <1427704836-8776-5-git-send-email-sfeldma@gmail.com> <20150330115417.GC2045@nanopsycho.orion> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: sfeldma@gmail.com, netdev@vger.kernel.org, linux@roeck-us.net, f.fainelli@gmail.com To: Jiri Pirko Return-path: Received: from mail-pd0-f171.google.com ([209.85.192.171]:34311 "EHLO mail-pd0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752601AbbC3NrL (ORCPT ); Mon, 30 Mar 2015 09:47:11 -0400 Received: by pdbni2 with SMTP id ni2so176325413pdb.1 for ; Mon, 30 Mar 2015 06:47:11 -0700 (PDT) In-Reply-To: <20150330115417.GC2045@nanopsycho.orion> Sender: netdev-owner@vger.kernel.org List-ID: 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 >> >> STP update is just a writable port attribute, so convert swdev_port_stp_update >> to an attr set. >> >> Signed-off-by: Scott Feldman >> --- >> 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_*