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=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable 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 629E7C433E0 for ; Mon, 21 Dec 2020 00:27:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 352A422CA1 for ; Mon, 21 Dec 2020 00:27:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726807AbgLUA0t (ORCPT ); Sun, 20 Dec 2020 19:26:49 -0500 Received: from vps0.lunn.ch ([185.16.172.187]:35102 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725272AbgLUA0s (ORCPT ); Sun, 20 Dec 2020 19:26:48 -0500 Received: from andrew by vps0.lunn.ch with local (Exim 4.94) (envelope-from ) id 1kr91Q-00D37t-Qr; Mon, 21 Dec 2020 01:25:56 +0100 Date: Mon, 21 Dec 2020 01:25:56 +0100 From: Andrew Lunn To: Steen Hegelund Cc: "David S. Miller" , Jakub Kicinski , Russell King , Lars Povlsen , Bjarni Jonasson , Microchip Linux Driver Support , Alexandre Belloni , Madalin Bucur , Nicolas Ferre , Mark Einon , Masahiro Yamada , Arnd Bergmann , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH v2 5/8] net: sparx5: add switching, vlan and mactable support Message-ID: <20201221002556.GC3107610@lunn.ch> References: <20201217075134.919699-1-steen.hegelund@microchip.com> <20201217075134.919699-6-steen.hegelund@microchip.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201217075134.919699-6-steen.hegelund@microchip.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c > + > +static inline int sparx5_mact_get_status(struct sparx5 *sparx5) > +{ > + return spx5_rd(sparx5, LRN_COMMON_ACCESS_CTRL); > +} > + > +static inline int sparx5_mact_wait_for_completion(struct sparx5 *sparx5) > +{ > + u32 val; > + > + return readx_poll_timeout(sparx5_mact_get_status, > + sparx5, val, > + LRN_COMMON_ACCESS_CTRL_MAC_TABLE_ACCESS_SHOT_GET(val) == 0, > + TABLE_UPDATE_SLEEP_US, TABLE_UPDATE_TIMEOUT_US); > +} No inline functions in C files please. > +void sparx5_mact_init(struct sparx5 *sparx5) > +{ > + mutex_init(&sparx5->lock); > + > + mutex_lock(&sparx5->lock); > + > + /* Flush MAC table */ > + spx5_wr(LRN_COMMON_ACCESS_CTRL_CPU_ACCESS_CMD_SET(MAC_CMD_CLEAR_ALL) | > + LRN_COMMON_ACCESS_CTRL_MAC_TABLE_ACCESS_SHOT_SET(1), > + sparx5, LRN_COMMON_ACCESS_CTRL); > + > + if (sparx5_mact_wait_for_completion(sparx5) != 0) > + dev_warn(sparx5->dev, "MAC flush error\n"); > + > + mutex_unlock(&sparx5->lock); It always seems odd to me, when you initialise a mutex, and then immediately take it. Who are you locking against? I'm not saying it is wrong though, especially if you have code in spx5_wr() and spx5_rd() which check the lock is actually taken. I've found a number of locking bugs in mv88e6xxx by having such checks. > + > + sparx5_set_ageing(sparx5, 10 * MSEC_PER_SEC); /* 10 sec */ BR_DEFAULT_AGEING_TIME is 300 seconds. Is this the same thing? > +static int sparx5_port_bridge_join(struct sparx5_port *port, > + struct net_device *bridge) > +{ > + struct sparx5 *sparx5 = port->sparx5; > + > + if (bitmap_empty(sparx5->bridge_mask, SPX5_PORTS)) > + /* First bridged port */ > + sparx5->hw_bridge_dev = bridge; > + else > + if (sparx5->hw_bridge_dev != bridge) > + /* This is adding the port to a second bridge, this is > + * unsupported > + */ > + return -ENODEV; Just checking my understanding. You have a 64 port switch, which only supports a single bridge? -EOPNOTSUPP seems like a better return code. > + > + set_bit(port->portno, sparx5->bridge_mask); > + > + /* Port enters in bridge mode therefor don't need to copy to CPU > + * frames for multicast in case the bridge is not requesting them > + */ > + __dev_mc_unsync(port->ndev, sparx5_mc_unsync); Did you copy that from the mellanox driver? I think in DSA we take the opposite approach. Multicast/broadcast goes to the CPU until the CPU says it does not want it. > +static void sparx5_port_bridge_leave(struct sparx5_port *port, > + struct net_device *bridge) > +{ > + struct sparx5 *sparx5 = port->sparx5; > + > + clear_bit(port->portno, sparx5->bridge_mask); > + if (bitmap_empty(sparx5->bridge_mask, SPX5_PORTS)) > + sparx5->hw_bridge_dev = NULL; > + > + /* Clear bridge vlan settings before updating the port settings */ > + port->vlan_aware = 0; > + port->pvid = NULL_VID; > + port->vid = NULL_VID; > + > + /* Port enters in host more therefore restore mc list */ s/more/mode > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_vlan.c > @@ -0,0 +1,223 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Microchip Sparx5 Switch driver > + * > + * Copyright (c) 2020 Microchip Technology Inc. and its subsidiaries. > + */ > + > +#include "sparx5_main.h" > + > +static int sparx5_vlant_set_mask(struct sparx5 *sparx5, u16 vid) Is the t in vlant typ0? > +int sparx5_vlan_vid_add(struct sparx5_port *port, u16 vid, bool pvid, > + bool untagged) > +{ > + struct sparx5 *sparx5 = port->sparx5; > + int ret; > + > + /* Make the port a member of the VLAN */ > + set_bit(port->portno, sparx5->vlan_mask[vid]); > + ret = sparx5_vlant_set_mask(sparx5, vid); > + if (ret) > + return ret; > + > + /* Default ingress vlan classification */ > + if (pvid) > + port->pvid = vid; > + > + /* Untagged egress vlan clasification */ classification > + if (untagged && port->vid != vid) { > + if (port->vid) { > + netdev_err(port->ndev, > + "Port already has a native VLAN: %d\n", > + port->vid); > + return -EBUSY; > + } > + port->vid = vid; > + } > + > + sparx5_vlan_port_apply(sparx5, port); > + > + return 0; > +} > +void sparx5_update_fwd(struct sparx5 *sparx5) > +{ > + u32 mask[3]; > + DECLARE_BITMAP(workmask, SPX5_PORTS); > + int port; > + > + /* Divide up fwd mask in 32 bit words */ > + bitmap_to_arr32(mask, sparx5->bridge_fwd_mask, SPX5_PORTS); > + > + /* Update flood masks */ > + for (port = PGID_UC_FLOOD; port <= PGID_BCAST; port++) { > + spx5_wr(mask[0], sparx5, ANA_AC_PGID_CFG(port)); > + spx5_wr(mask[1], sparx5, ANA_AC_PGID_CFG1(port)); > + spx5_wr(mask[2], sparx5, ANA_AC_PGID_CFG2(port)); > + } > + > + /* Update SRC masks */ > + for (port = 0; port < SPX5_PORTS; port++) { > + if (test_bit(port, sparx5->bridge_fwd_mask)) { > + /* Allow to send to all bridged but self */ > + bitmap_copy(workmask, sparx5->bridge_fwd_mask, SPX5_PORTS); > + clear_bit(port, workmask); > + bitmap_to_arr32(mask, workmask, SPX5_PORTS); > + spx5_wr(mask[0], sparx5, ANA_AC_SRC_CFG(port)); > + spx5_wr(mask[1], sparx5, ANA_AC_SRC_CFG1(port)); > + spx5_wr(mask[2], sparx5, ANA_AC_SRC_CFG2(port)); > + } else { > + spx5_wr(0, sparx5, ANA_AC_SRC_CFG(port)); > + spx5_wr(0, sparx5, ANA_AC_SRC_CFG1(port)); > + spx5_wr(0, sparx5, ANA_AC_SRC_CFG2(port)); > + } Humm, interesting. This seems to control what other ports a port can send to. That is one of the basic features you need for supporting multiple bridges. So i assume your problems is you cannot partition the MAC table? Andrew 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=-8.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 8AECCC433DB for ; Mon, 21 Dec 2020 00:28:07 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 15A4222C9C for ; Mon, 21 Dec 2020 00:28:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 15A4222C9C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lunn.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=MbOflNNJW6vB8j9cpbP+l58LTk1INxtHLtMy+V32tJY=; b=HFQEpYAlQhoZHxG7J5GkI8o6e kRX4E2l0ARrHBOCsUqo0LrZhc1HYQFSC/ZPRTPWff/K0d0rOrHxdDRpvKg7I6mwTmIRP2pF1Nl9oY x5kDxvrLz2wPjwc98o9YHSKuwSPInzGyWswj/0Tm1MkkZ9x50E0jo1OaYIrywy56yyeXMTbAmN8Fp mnvGokGUYGRj7/zihH2lQ6CbRUfGfkS2KUzM8Rxri4aeSkbWDGEpepZgMkErzY/h4F61I2Bktt483 a6A3vZxgzamueOU6jQAMGokWpX4h1PXm/5R3suzaHmpii+5/jJ7t6XIi3OpcEITDswIbAXNkPa/Zf krAjBdbiQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kr91j-0000Rb-GJ; Mon, 21 Dec 2020 00:26:15 +0000 Received: from vps0.lunn.ch ([185.16.172.187]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kr91g-0000RF-0t for linux-arm-kernel@lists.infradead.org; Mon, 21 Dec 2020 00:26:12 +0000 Received: from andrew by vps0.lunn.ch with local (Exim 4.94) (envelope-from ) id 1kr91Q-00D37t-Qr; Mon, 21 Dec 2020 01:25:56 +0100 Date: Mon, 21 Dec 2020 01:25:56 +0100 From: Andrew Lunn To: Steen Hegelund Subject: Re: [RFC PATCH v2 5/8] net: sparx5: add switching, vlan and mactable support Message-ID: <20201221002556.GC3107610@lunn.ch> References: <20201217075134.919699-1-steen.hegelund@microchip.com> <20201217075134.919699-6-steen.hegelund@microchip.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201217075134.919699-6-steen.hegelund@microchip.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201220_192612_191456_1FDA335F X-CRM114-Status: GOOD ( 24.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Bjarni Jonasson , Alexandre Belloni , linux-kernel@vger.kernel.org, Arnd Bergmann , Madalin Bucur , netdev@vger.kernel.org, Masahiro Yamada , Russell King , Microchip Linux Driver Support , linux-arm-kernel@lists.infradead.org, Mark Einon , Jakub Kicinski , "David S. Miller" , Lars Povlsen Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c > + > +static inline int sparx5_mact_get_status(struct sparx5 *sparx5) > +{ > + return spx5_rd(sparx5, LRN_COMMON_ACCESS_CTRL); > +} > + > +static inline int sparx5_mact_wait_for_completion(struct sparx5 *sparx5) > +{ > + u32 val; > + > + return readx_poll_timeout(sparx5_mact_get_status, > + sparx5, val, > + LRN_COMMON_ACCESS_CTRL_MAC_TABLE_ACCESS_SHOT_GET(val) == 0, > + TABLE_UPDATE_SLEEP_US, TABLE_UPDATE_TIMEOUT_US); > +} No inline functions in C files please. > +void sparx5_mact_init(struct sparx5 *sparx5) > +{ > + mutex_init(&sparx5->lock); > + > + mutex_lock(&sparx5->lock); > + > + /* Flush MAC table */ > + spx5_wr(LRN_COMMON_ACCESS_CTRL_CPU_ACCESS_CMD_SET(MAC_CMD_CLEAR_ALL) | > + LRN_COMMON_ACCESS_CTRL_MAC_TABLE_ACCESS_SHOT_SET(1), > + sparx5, LRN_COMMON_ACCESS_CTRL); > + > + if (sparx5_mact_wait_for_completion(sparx5) != 0) > + dev_warn(sparx5->dev, "MAC flush error\n"); > + > + mutex_unlock(&sparx5->lock); It always seems odd to me, when you initialise a mutex, and then immediately take it. Who are you locking against? I'm not saying it is wrong though, especially if you have code in spx5_wr() and spx5_rd() which check the lock is actually taken. I've found a number of locking bugs in mv88e6xxx by having such checks. > + > + sparx5_set_ageing(sparx5, 10 * MSEC_PER_SEC); /* 10 sec */ BR_DEFAULT_AGEING_TIME is 300 seconds. Is this the same thing? > +static int sparx5_port_bridge_join(struct sparx5_port *port, > + struct net_device *bridge) > +{ > + struct sparx5 *sparx5 = port->sparx5; > + > + if (bitmap_empty(sparx5->bridge_mask, SPX5_PORTS)) > + /* First bridged port */ > + sparx5->hw_bridge_dev = bridge; > + else > + if (sparx5->hw_bridge_dev != bridge) > + /* This is adding the port to a second bridge, this is > + * unsupported > + */ > + return -ENODEV; Just checking my understanding. You have a 64 port switch, which only supports a single bridge? -EOPNOTSUPP seems like a better return code. > + > + set_bit(port->portno, sparx5->bridge_mask); > + > + /* Port enters in bridge mode therefor don't need to copy to CPU > + * frames for multicast in case the bridge is not requesting them > + */ > + __dev_mc_unsync(port->ndev, sparx5_mc_unsync); Did you copy that from the mellanox driver? I think in DSA we take the opposite approach. Multicast/broadcast goes to the CPU until the CPU says it does not want it. > +static void sparx5_port_bridge_leave(struct sparx5_port *port, > + struct net_device *bridge) > +{ > + struct sparx5 *sparx5 = port->sparx5; > + > + clear_bit(port->portno, sparx5->bridge_mask); > + if (bitmap_empty(sparx5->bridge_mask, SPX5_PORTS)) > + sparx5->hw_bridge_dev = NULL; > + > + /* Clear bridge vlan settings before updating the port settings */ > + port->vlan_aware = 0; > + port->pvid = NULL_VID; > + port->vid = NULL_VID; > + > + /* Port enters in host more therefore restore mc list */ s/more/mode > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_vlan.c > @@ -0,0 +1,223 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Microchip Sparx5 Switch driver > + * > + * Copyright (c) 2020 Microchip Technology Inc. and its subsidiaries. > + */ > + > +#include "sparx5_main.h" > + > +static int sparx5_vlant_set_mask(struct sparx5 *sparx5, u16 vid) Is the t in vlant typ0? > +int sparx5_vlan_vid_add(struct sparx5_port *port, u16 vid, bool pvid, > + bool untagged) > +{ > + struct sparx5 *sparx5 = port->sparx5; > + int ret; > + > + /* Make the port a member of the VLAN */ > + set_bit(port->portno, sparx5->vlan_mask[vid]); > + ret = sparx5_vlant_set_mask(sparx5, vid); > + if (ret) > + return ret; > + > + /* Default ingress vlan classification */ > + if (pvid) > + port->pvid = vid; > + > + /* Untagged egress vlan clasification */ classification > + if (untagged && port->vid != vid) { > + if (port->vid) { > + netdev_err(port->ndev, > + "Port already has a native VLAN: %d\n", > + port->vid); > + return -EBUSY; > + } > + port->vid = vid; > + } > + > + sparx5_vlan_port_apply(sparx5, port); > + > + return 0; > +} > +void sparx5_update_fwd(struct sparx5 *sparx5) > +{ > + u32 mask[3]; > + DECLARE_BITMAP(workmask, SPX5_PORTS); > + int port; > + > + /* Divide up fwd mask in 32 bit words */ > + bitmap_to_arr32(mask, sparx5->bridge_fwd_mask, SPX5_PORTS); > + > + /* Update flood masks */ > + for (port = PGID_UC_FLOOD; port <= PGID_BCAST; port++) { > + spx5_wr(mask[0], sparx5, ANA_AC_PGID_CFG(port)); > + spx5_wr(mask[1], sparx5, ANA_AC_PGID_CFG1(port)); > + spx5_wr(mask[2], sparx5, ANA_AC_PGID_CFG2(port)); > + } > + > + /* Update SRC masks */ > + for (port = 0; port < SPX5_PORTS; port++) { > + if (test_bit(port, sparx5->bridge_fwd_mask)) { > + /* Allow to send to all bridged but self */ > + bitmap_copy(workmask, sparx5->bridge_fwd_mask, SPX5_PORTS); > + clear_bit(port, workmask); > + bitmap_to_arr32(mask, workmask, SPX5_PORTS); > + spx5_wr(mask[0], sparx5, ANA_AC_SRC_CFG(port)); > + spx5_wr(mask[1], sparx5, ANA_AC_SRC_CFG1(port)); > + spx5_wr(mask[2], sparx5, ANA_AC_SRC_CFG2(port)); > + } else { > + spx5_wr(0, sparx5, ANA_AC_SRC_CFG(port)); > + spx5_wr(0, sparx5, ANA_AC_SRC_CFG1(port)); > + spx5_wr(0, sparx5, ANA_AC_SRC_CFG2(port)); > + } Humm, interesting. This seems to control what other ports a port can send to. That is one of the basic features you need for supporting multiple bridges. So i assume your problems is you cannot partition the MAC table? Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel