From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 51C0EC43381 for ; Wed, 27 Mar 2019 00:31:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 10D062075E for ; Wed, 27 Mar 2019 00:31:54 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="S0f2SlEO" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732335AbfC0Abx (ORCPT ); Tue, 26 Mar 2019 20:31:53 -0400 Received: from mail-wm1-f65.google.com ([209.85.128.65]:36165 "EHLO mail-wm1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731847AbfC0Abx (ORCPT ); Tue, 26 Mar 2019 20:31:53 -0400 Received: by mail-wm1-f65.google.com with SMTP id h18so14892213wml.1 for ; Tue, 26 Mar 2019 17:31:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=/EmPjQogR81QrEiSr4JkILYCsntqz/RzB8nZ9hR8F+k=; b=S0f2SlEO+puOnq5SP8ZMPWWe86eab7zNXFTK39yQ2vG/fXfHM9VCCnDwkMRUHxBuJj eJrPmW0sL+5HWJTXTsCoNdqleZnnZzcF5DrKcSPf41Fj7Sp9PzeoYcZG2waR1fOiAfcy bLe9XlxNHPntPAv7IlIgAQIUwdOA5/lrEO+EyycKmjNqT0DudisCSk8MCZsD5g5+ERZn mzj+1vy/bx3XzGPsMYx3432XTe70pgzkj+M4BcxIywgwbuD8M/dJ71us8l92xjNh7i4e NT0O3zi2gGvlPAuSIArLmvtqDasz/lFnokaLVE1MwS6U2F+8jbSWeZz6t1GIs1VHqTi0 clog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=/EmPjQogR81QrEiSr4JkILYCsntqz/RzB8nZ9hR8F+k=; b=mT1t0iHEINQBIXK0XRV+YtdL3SsrbCPkHeWGT00joySkpmCoM1qquoZn2LytUnMpbc bqNAKIuos8nNjzNt28YciiYjgG9SeIEbDabqSZmmG1mKBFwti+4Y3H3lvkcdkSRNydYP JivypHpAslxhSr8mS9wMcS0Yl5/QjxQuZ4joc7ddYlm7BcEhv1aO6K02mVKsBa0KddF6 LwH28VHvFCD8P22nex68Xo5VyOynRlqmSLLMLKLKxFjl4ZVh401KQPQMeQImgrq8Dj/f Pyk6yGVOv6pmhjD2izh3WUqmlf3cdcYzL2tEdQGmCZPJHWNoo4Oh/nO89PIXEJdzSzTo SHzg== X-Gm-Message-State: APjAAAXXnldsN4UhuPuWwld26md8hyvcxmIiyXsu8NzhW3E1YkfcrwaE ieU7j/KSC5YSNXFikD0JgGA= X-Google-Smtp-Source: APXvYqzfVf6QfUkR0LSENx16MEvwMSV1RYycTT8sEwJVb7OP3Rm9TdDUdLltBdQofMD8FUHnXnBCcA== X-Received: by 2002:a1c:5581:: with SMTP id j123mr17651489wmb.10.1553646710615; Tue, 26 Mar 2019 17:31:50 -0700 (PDT) Received: from [192.168.1.2] ([188.26.228.227]) by smtp.gmail.com with ESMTPSA id j66sm36293156wrj.51.2019.03.26.17.31.49 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 26 Mar 2019 17:31:50 -0700 (PDT) Subject: Re: [RFC PATCH net-next 03/13] net: dsa: Create a more convenient function for installing port VLANs To: Florian Fainelli , davem@davemloft.net, netdev@vger.kernel.org Cc: andrew@lunn.ch, vivien.didelot@gmail.com, linus.walleij@linaro.org References: <20190324032346.32394-1-olteanv@gmail.com> <20190324032346.32394-4-olteanv@gmail.com> <49500e18-f2d5-4c9f-2ce0-b6fa12cbc8d6@gmail.com> From: Vladimir Oltean Message-ID: <32bbb7cd-bc0e-0669-312d-99cdc845604b@gmail.com> Date: Wed, 27 Mar 2019 02:31:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <49500e18-f2d5-4c9f-2ce0-b6fa12cbc8d6@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > > 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, >> > >