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=-0.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,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 AC89EC43381 for ; Tue, 19 Feb 2019 17:38:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 715AA2147C for ; Tue, 19 Feb 2019 17:38:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="s/HM8tz6" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727585AbfBSRic (ORCPT ); Tue, 19 Feb 2019 12:38:32 -0500 Received: from mail-qk1-f194.google.com ([209.85.222.194]:42490 "EHLO mail-qk1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725613AbfBSRib (ORCPT ); Tue, 19 Feb 2019 12:38:31 -0500 Received: by mail-qk1-f194.google.com with SMTP id y140so162598qkb.9 for ; Tue, 19 Feb 2019 09:38:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:message-id:from:to:cc:subject:in-reply-to:references :mime-version:content-disposition:content-transfer-encoding; bh=CwPcphlHF9arr0Gc5odt6fipSI+QG39KaTXQYnL/7yM=; b=s/HM8tz6PNYeiaJoj+zWU6y40ogzZQd5HwXO5fzCI+k5nBzxTKusy6vIyDsDU4p4Az uuEjsI2Lz1Y1viA5Am+d6I2NJrvkOQQHkDjqwWnvLAKpGKra9DT+xwtEd+zivs5Db+Yv To+3EymUzTBw1Fj4muAViuT5wL/yb+8RAR//OMCVeJQ6/NZcCVvv760XDzNEiEQiYxb3 U09ZEdmPdX1c6LYTFytbfLAltOHygLiBD/DA/0QORpg/BHWRFa35PSWDp6rwQl5Fv0MB PO6iEEf22TgBjKV9K6hZbcqu4ROXypH4QZOpIinaD3bHNR3hiEETkyBKPhbBGovQMg1X bHXQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:message-id:from:to:cc:subject:in-reply-to :references:mime-version:content-disposition :content-transfer-encoding; bh=CwPcphlHF9arr0Gc5odt6fipSI+QG39KaTXQYnL/7yM=; b=YceSXdHauCDM8u9O8oGI3uD+7NbA9vpChytXkXMgBs3oiZ+EIVGZlaT9Mv5NucrUDW 6Z+fA2G//z1KXTqrAkmMas2bj8msc1VpqG35ctfWGWd9rgWnL8hQO834zD4/2HiZ3hqm pDeQ2Nr8DJ7/YKYXx3YAth3BTbNCczJ9SU/Cfu0sFlJGlqwenqwEfL9ABO3K/3V7B90X wjDNu2q7gQg4aghex9ldnim6NuNsQruaxQ7h+KouTPScLIufUmjP0swMo1TzGsGRVaRh WmL7aNPOA6PvccV6INW3SDyWu5uL1kiFPpLGJoamFHCoFe/OVJMv7tmoijrrx4j5W92Z ph4A== X-Gm-Message-State: AHQUAuamoh++wnGPSVv5quiIq6rbdnnwFq9L4KkOBiL+n7K4xiHpqQgb CUtbaAmfZay0QBiUf5Pc/Yc= X-Google-Smtp-Source: AHgI3IZPjwxKa92mv1rv6vz45ZkqJ/8ncU3C7RfPkT3DQ9bliqtSwD2+LMOUInGd1tSeqSbDi9Smxg== X-Received: by 2002:a37:9a85:: with SMTP id c127mr21200002qke.273.1550597910009; Tue, 19 Feb 2019 09:38:30 -0800 (PST) Received: from localhost (modemcable249.105-163-184.mc.videotron.ca. [184.163.105.249]) by smtp.gmail.com with ESMTPSA id r44sm6124191qtb.53.2019.02.19.09.38.29 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 19 Feb 2019 09:38:29 -0800 (PST) Date: Tue, 19 Feb 2019 12:38:28 -0500 Message-ID: <20190219123828.GD10959@t480s.localdomain> From: Vivien Didelot To: Russell King - ARM Linux admin Cc: Andrew Lunn , Florian Fainelli , Heiner Kallweit , "David S. Miller" , netdev@vger.kernel.org Subject: Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags In-Reply-To: <20190219171414.cvaiw7u2xnd5zk3g@shell.armlinux.org.uk> References: <20190217163114.yomawlljyxlqy3ob@shell.armlinux.org.uk> <20190219111612.GF27578@t480s.localdomain> <20190219162435.f5zl5harbarwy6bj@shell.armlinux.org.uk> <20190219120000.GB4140@t480s.localdomain> <20190219171414.cvaiw7u2xnd5zk3g@shell.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hi Russell, On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin wrote: > > > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds) > > > > > +{ > > > > > + struct mv88e6xxx_chip *chip = ds->priv; > > > > > + unsigned long support = 0; > > > > > + > > > > > + if (chip->info->ops->port_set_egress_floods) > > > > > + support |= BR_FLOOD | BR_MCAST_FLOOD; > > > > > + > > > > > + return support; > > > > > +} > > > > > > > > I think that it isn't necessary to propagate the notion of bridge flags down > > > > to the DSA drivers. It might be just enough to add: > > > > > > > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc) > > > > > > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core, > > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think? > > > > > > There are two other flags that I haven't covered which the bridge code > > > expects to be offloaded, and those are the broadcast flood flag and > > > the learning flag. > > > > I see. What does the bridge code do if these flags are set? Does it expect > > the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it > > program this entry into the bridged ports? > > The bridge code defaults to all four flags set. See new_nbp() in > net/bridge/br_if.c: > > p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD; > > bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to > userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's > no man page documentation for that flag. > > According to br_flood() in net/bridge/br_forward.c, it controls > whether broadcast frames are flooded to all ports or not. Changing > this flag is merely handled just like the multicast/unicast flooding > flags - a call is made to set the offloaded flags, and if it isn't > returned as being supported, a warning is printed. No attempt is > made to create or change a forwarding entry for the broadcast MAC > address. OK, thanks for the details. The programming of the broadcast MAC address must be handled in the core then, I will move this from mv88e6xxx up to the DSA layer later, but that's totally orthogonal here. > > bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING: > > learning on or learning off > Controls whether a given port will learn MAC addresses from > received traffic or not. If learning if off, the bridge will end > up flooding any traffic for which it has no FDB entry. By > default this flag is on. > > > In the latter case we have almost nothing to do. In the former case, we can > > make the core call dsa_port_mdb_add on setup and when a VLAN is added. > > > > mv88e6xxx tries to be smart and is already doing that and I'm really not a fan. > > > > If tomorrow there's a switch capable of simply toggling a bit to do that, > > we can add a new ops and skip the port_mdb_add call in the core. > > > > > I know that the Marvell switches don't have a bit to control the > > > broadcast flooding, that appears to be controlled via a static entry > > > in the ATU which would have to be modified as the broadcast flood flag > > > is manipulated. I don't know how that is handled in other bridges. > > > > > > Do we want to include the broadcast flood in the above prototype? > > > If we go for this, how do we detect which options a switch supports? > > > > If the necessary dsa_switch_ops routine is correctly prototyped, having it > > implemented by a driver or not should be enough to inform the core that the > > related feature(s) is/are supported by the switch. > > > > I'll try to give a bit more context on why I'd prefer this approach, hoping > > it makes sense: a switch driver does not need to understand bridge flags > > per-se, the core should give enough abstraction to this layer (and any other > > net-specifics). The core just needs to know if a driver can program this or > > that. More importantly, it can easily become messy to maintain switch-cases > > of arbitrary flags in all drivers and the core. > > So, should we go the other way and have: > > int (*port_learning)(struct dsa_switch *ds, int port, bool enable); > int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable); > int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable); > int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable); > > rather than trying to combine uc/mc into one? It would mean that we'd > be performing more bus reads/writes, but I guess that doesn't matter > for these configuration parameters. I like this very much. As long as these flags can be programmed in switch devices, these ops totally make sense. Thanks, Vivien