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.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,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 12BA2C433E0 for ; Wed, 23 Dec 2020 13:55:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CC24322C9F for ; Wed, 23 Dec 2020 13:55:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728751AbgLWNza (ORCPT ); Wed, 23 Dec 2020 08:55:30 -0500 Received: from esa.microchip.iphmx.com ([68.232.154.123]:49329 "EHLO esa.microchip.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728581AbgLWNz3 (ORCPT ); Wed, 23 Dec 2020 08:55:29 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1608731729; x=1640267729; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=0baOEf/dlj0n6rdHsUM8wiZs8wduZgzozALMV9bUji4=; b=MGbJvprX/TCEfjgcj3BpeRl95GgLSyEsyCsoyA0ID7Gj9RLdavnWe3Bo YUveByVYWmIeoWkhJMH/OQ25vmTRAiiC3bAW60E6ixjHbpa0L/Gbw/4cM 9gfSvhjjiJy7M/YuI6Pb9+kIOl0Rfnigq0G/VMZm4CNGQVRtYdWKdGbeA oQl3NnXGHf2w7ZDAJHYsxEBhBF3rMe0fROfeanCiHQuu+zfTeB+qOmCdm Ul2kpvMIvbeQNBmzAcOcVIfIZOsKuIkjiGZjMUVW6vOFI7tBD/dPeL+ns P0kItZ5fjhF8/ryT7HHagMtkh/0lLiKzRd072ycVEgnPskT4ZumucQBVs w==; IronPort-SDR: 6EvuGI0Ar1AW5mM5mGzskubsadcHIEgtHUesu9I12AUnIfpjRRSunWAsmS25WWeTFCkRgS6qLJ +Gtju27pU6s7zy0d5HQeBp8wdntgB3FgrafCSHcUzkJsioszCLgCAcEFyZbP+rnWJljdCZIgwO FYILuXs6bH9wQElRM6fZhmqow6cXY5m6NTW3A97NKEopG8tPKZdyspEYc9ICa0yG7ArgLIzrYQ 6uZ+lBMoqfWiCBfhYnlMMmoGd+RBuQYerUkXUN2JKUlAwBQqNmrsrto7lStRg4dgxlibPNTgCS IBY= X-IronPort-AV: E=Sophos;i="5.78,441,1599548400"; d="scan'208";a="98083193" Received: from smtpout.microchip.com (HELO email.microchip.com) ([198.175.253.82]) by esa4.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 23 Dec 2020 06:54:12 -0700 Received: from chn-vm-ex02.mchp-main.com (10.10.87.72) by chn-vm-ex02.mchp-main.com (10.10.87.72) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1979.3; Wed, 23 Dec 2020 06:54:12 -0700 Received: from localhost (10.10.115.15) by chn-vm-ex02.mchp-main.com (10.10.85.144) with Microsoft SMTP Server id 15.1.1979.3 via Frontend Transport; Wed, 23 Dec 2020 06:54:12 -0700 Date: Wed, 23 Dec 2020 14:54:11 +0100 From: Steen Hegelund To: Andrew Lunn 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 , , , Subject: Re: [RFC PATCH v2 5/8] net: sparx5: add switching, vlan and mactable support Message-ID: <20201223135411.qca4ury2h44yxbzu@mchp-dev-shegelun> References: <20201217075134.919699-1-steen.hegelund@microchip.com> <20201217075134.919699-6-steen.hegelund@microchip.com> <20201221002556.GC3107610@lunn.ch> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Disposition: inline In-Reply-To: <20201221002556.GC3107610@lunn.ch> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Andrew, On 21.12.2020 01:25, Andrew Lunn wrote: >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> +++ 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. OK. > >> +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. The driver has a workqueue and a notifier callback that may want to access the table, and will have to wait in line to be served, but since they have not yet been activated at this point, you are probably correct in saying that locking is not needed at init time. I will investigate this.. > >> + >> + sparx5_set_ageing(sparx5, 10 * MSEC_PER_SEC); /* 10 sec */ > >BR_DEFAULT_AGEING_TIME is 300 seconds. Is this the same thing? Same thing but different value. I think this should change to same default, but I will test it out. > >> +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? Yes, at least for the initial version. The switch has some virtualization features, but were not using that just yet. > >-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. > > It is "inspired" by the Ocelot driver. MC is explicitly opted in. >> +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 OK. > >> +++ 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? No, it is probably short for "vlan table". Again the inspiration comes from the Ocelot implementation. > >> +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 OK. > >> + 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? > No, the MAC table is VLAN aware. > > Andrew BR Steen --------------------------------------- Steen Hegelund steen.hegelund@microchip.com 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=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 88604C433E0 for ; Wed, 23 Dec 2020 13:55:36 +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 125DC22C9F for ; Wed, 23 Dec 2020 13:55:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 125DC22C9F Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=microchip.com 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-Type: Content-Transfer-Encoding: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=OdR/KnYKZjtv+qaiqpt2BkImYIzB6GC+eK0bw1GsUbY=; b=FWqb+szDLSq5ha8q/375ScT9q FEjSe8YG4A9UsUCmPV64uj0JQa6YdT8m/gUz4hfSWUiem3J5C6wWjDMAzzSPGShDp0qiCQUV8RhIq ILCOngfm/YO+RJf3u2R5vC/Il78PIoswtxWMEIn4Ge6QFpyOG9FTm1sbivDnS3ffnIq1nNoKVs82k c5kmxBZUWP6Fw1CUUjJcJ/tnXdUa3RFjE8yVbB+lduJtfZ8bSWXlFQG5IHlbCMZC9khHfvJkXuhjA PjiYOQJ5gKxXH6/A7FU69ugZEk070dFXUMrPiKzyBkMl7NAW0YaRMEspLEReJvIOEyU46s51expCF uAtfsEgJg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1ks4ao-0008Qq-VP; Wed, 23 Dec 2020 13:54:19 +0000 Received: from esa.microchip.iphmx.com ([68.232.154.123]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1ks4ak-0008PH-L7 for linux-arm-kernel@lists.infradead.org; Wed, 23 Dec 2020 13:54:17 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1608731654; x=1640267654; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=0baOEf/dlj0n6rdHsUM8wiZs8wduZgzozALMV9bUji4=; b=wbPMxcE27O1AOpb9BWKYnRaQhbINGlCQygF4hVBC1d2OKTq8vdbejakf WxJ0kD949Asnu5/q3wFhXSHbKq6u08pq7H7UfM/cre1a04SDfb3YFjLxC fabQOuorH2DfWEvj7LDFvCRUqaTT/myufdTf6xZqCzJl7JqoFGr7/cwJJ rJfo+Ts/GElqmgCN8g5ewHRr4EXG+6VqBadzM9EKZgQoZvqs0Fyx0sn+n fQeJcNCWlpgc4Tq+1WeSiBVRzC6Px3oSPoE/t78zSdTcRgrAtJSWU2rb5 04XesddyKdvRsbTO9hxqTR+PuYoo8/ZB7REEWBeWtbaOUF1Or15XvwV7c A==; IronPort-SDR: 6EvuGI0Ar1AW5mM5mGzskubsadcHIEgtHUesu9I12AUnIfpjRRSunWAsmS25WWeTFCkRgS6qLJ +Gtju27pU6s7zy0d5HQeBp8wdntgB3FgrafCSHcUzkJsioszCLgCAcEFyZbP+rnWJljdCZIgwO FYILuXs6bH9wQElRM6fZhmqow6cXY5m6NTW3A97NKEopG8tPKZdyspEYc9ICa0yG7ArgLIzrYQ 6uZ+lBMoqfWiCBfhYnlMMmoGd+RBuQYerUkXUN2JKUlAwBQqNmrsrto7lStRg4dgxlibPNTgCS IBY= X-IronPort-AV: E=Sophos;i="5.78,441,1599548400"; d="scan'208";a="98083193" Received: from smtpout.microchip.com (HELO email.microchip.com) ([198.175.253.82]) by esa4.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 23 Dec 2020 06:54:12 -0700 Received: from chn-vm-ex02.mchp-main.com (10.10.87.72) by chn-vm-ex02.mchp-main.com (10.10.87.72) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1979.3; Wed, 23 Dec 2020 06:54:12 -0700 Received: from localhost (10.10.115.15) by chn-vm-ex02.mchp-main.com (10.10.85.144) with Microsoft SMTP Server id 15.1.1979.3 via Frontend Transport; Wed, 23 Dec 2020 06:54:12 -0700 Date: Wed, 23 Dec 2020 14:54:11 +0100 From: Steen Hegelund To: Andrew Lunn Subject: Re: [RFC PATCH v2 5/8] net: sparx5: add switching, vlan and mactable support Message-ID: <20201223135411.qca4ury2h44yxbzu@mchp-dev-shegelun> References: <20201217075134.919699-1-steen.hegelund@microchip.com> <20201217075134.919699-6-steen.hegelund@microchip.com> <20201221002556.GC3107610@lunn.ch> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201221002556.GC3107610@lunn.ch> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201223_085414_837367_47AF5BDA X-CRM114-Status: GOOD ( 28.10 ) 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Andrew, On 21.12.2020 01:25, Andrew Lunn wrote: >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> +++ 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. OK. > >> +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. The driver has a workqueue and a notifier callback that may want to access the table, and will have to wait in line to be served, but since they have not yet been activated at this point, you are probably correct in saying that locking is not needed at init time. I will investigate this.. > >> + >> + sparx5_set_ageing(sparx5, 10 * MSEC_PER_SEC); /* 10 sec */ > >BR_DEFAULT_AGEING_TIME is 300 seconds. Is this the same thing? Same thing but different value. I think this should change to same default, but I will test it out. > >> +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? Yes, at least for the initial version. The switch has some virtualization features, but were not using that just yet. > >-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. > > It is "inspired" by the Ocelot driver. MC is explicitly opted in. >> +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 OK. > >> +++ 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? No, it is probably short for "vlan table". Again the inspiration comes from the Ocelot implementation. > >> +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 OK. > >> + 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? > No, the MAC table is VLAN aware. > > Andrew BR Steen --------------------------------------- Steen Hegelund steen.hegelund@microchip.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel