From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [patch net-next RFC 2/3] switchdev: allow caller to explicitly use deferred attr_set version Date: Thu, 8 Oct 2015 10:26:31 +0200 Message-ID: <20151008082631.GB2186@nanopsycho.orion> References: <1444242652-17260-1-git-send-email-jiri@resnulli.us> <1444242652-17260-3-git-send-email-jiri@resnulli.us> <20151008053933.GA2186@nanopsycho.orion> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Netdev , "David S. Miller" , Ido Schimmel , eladr@mellanox.com, Florian Fainelli , Guenter Roeck , Vivien Didelot , "andrew@lunn.ch" , john fastabend , David Laight To: Scott Feldman Return-path: Received: from mail-wi0-f176.google.com ([209.85.212.176]:33948 "EHLO mail-wi0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753524AbbJHI0e (ORCPT ); Thu, 8 Oct 2015 04:26:34 -0400 Received: by wicfx3 with SMTP id fx3so17050627wic.1 for ; Thu, 08 Oct 2015 01:26:33 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: Thu, Oct 08, 2015 at 08:03:35AM CEST, sfeldma@gmail.com wrote: >On Wed, Oct 7, 2015 at 10:39 PM, Jiri Pirko wrote: >> Thu, Oct 08, 2015 at 06:27:07AM CEST, sfeldma@gmail.com wrote: >>>On Wed, Oct 7, 2015 at 11:30 AM, Jiri Pirko wrote: >>>> From: Jiri Pirko >>>> >>>> Caller should know if he can call attr_set directly (when holding RTNL) >>>> or if he has to use deferred version of this function. >>>> >>>> This also allows drivers to sleep inside attr_set and report operation >>>> status back to switchdev core. Switchdev core then warns if status is >>>> not ok, instead of silent errors happening in drivers. >>>> >>>> Signed-off-by: Jiri Pirko >>>> --- >>>> include/net/switchdev.h | 2 + >>>> net/bridge/br_stp.c | 4 +- >>>> net/switchdev/switchdev.c | 113 +++++++++++++++++++++++++--------------------- >>>> 3 files changed, 65 insertions(+), 54 deletions(-) >>>> >>>> diff --git a/include/net/switchdev.h b/include/net/switchdev.h >>>> index 89266a3..320be44 100644 >>>> --- a/include/net/switchdev.h >>>> +++ b/include/net/switchdev.h >>>> @@ -168,6 +168,8 @@ int switchdev_port_attr_get(struct net_device *dev, >>>> struct switchdev_attr *attr); >>>> int switchdev_port_attr_set(struct net_device *dev, >>>> struct switchdev_attr *attr); >>>> +int switchdev_port_attr_set_deferred(struct net_device *dev, >>>> + struct switchdev_attr *attr); >>> >>>Rather than adding another op, use attr->flags and define: >>> >>>#define SWITCHDEV_F_DEFERRED BIT(x) >>> >>>So we get: >>> >>>void br_set_state(struct net_bridge_port *p, unsigned int state) >>>{ >>> struct switchdev_attr attr = { >>> .id = SWITCHDEV_ATTR_ID_PORT_STP_STATE, >>>+ .flags = SWITCHDEV_F_DEFERRED, >>> .u.stp_state = state, >>> }; >>> int err; >>> >>> p->state = state; >>> err = switchdev_port_attr_set(p->dev, &attr); >>> if (err && err != -EOPNOTSUPP) >>> br_warn(p->br, "error setting offload STP state on >>>port %u(%s)\n", >>> (unsigned int) p->port_no, >>>p->dev->name); >>>} >>> >>>(And add obj->flags to do the same). >> >> That's what I wanted to avoid. Also because the obj is const and for >> call from work, this flag would have to be removed. > >What did you want to avoid? Having this as a flag. I don't like it too much. But that is cosmetics. Other than that, does the patchset make sense? Do you see some possible issues? Thanks. Jiri