| Message ID | 20220729151733.6032-3-arun.ramadoss@microchip.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: vlan configuration for bridge_vlan_unaware ports | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 11 of 11 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Jul 29, 2022 at 08:47:31PM +0530, Arun Ramadoss wrote: > To have the similar implementation among the ksz switches, removed the > vlan_filtering_is_global flag which is only present in the lan937x. > > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> > --- > drivers/net/dsa/microchip/lan937x_main.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c > index daedd2bf20c1..9c1fe38efd1a 100644 > --- a/drivers/net/dsa/microchip/lan937x_main.c > +++ b/drivers/net/dsa/microchip/lan937x_main.c > @@ -401,11 +401,6 @@ int lan937x_setup(struct dsa_switch *ds) > return ret; > } > > - /* The VLAN aware is a global setting. Mixed vlan > - * filterings are not supported. > - */ > - ds->vlan_filtering_is_global = true; > - You understand what this flag does, right? It ensures that if you have lan0 and lan1 under VLAN-aware br0, then lan2 which is standalone will declare NETIF_F_HW_VLAN_CTAG_FILTER. In turn, this makes the network stack know that lan2 won't accept VLAN-tagged packets unless there is an 8021q interface with the given VID on top of it. This 8021q interface calls vlan_vid_add() to populate the driver's VLAN RX filter with its VID, and this gets translated into dsa_slave_vlan_rx_add_vid() which ultimately reaches the driver's ->port_vlan_add() function. If VLAN filtering *is* a global setting, and looking at this call from ksz9477_port_vlan_filtering() which is not per port, I'd say it is: ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE, true); then what would happen is that all VLAN tagged traffic would be dropped on the standalone lan2. I'd say that the ksz9477 is buggy for not declaring vlan_filtering_is_global, rather than encouraging you to delete it from lan937x. In turn, fixing ksz9477 would make setting this flag from a common location possible, because ksz8 needs it too.
Hi Vladimir, Thanks for the comment. On Tue, 2022-08-02 at 13:40 +0300, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Fri, Jul 29, 2022 at 08:47:31PM +0530, Arun Ramadoss wrote: > > To have the similar implementation among the ksz switches, removed > > the > > vlan_filtering_is_global flag which is only present in the lan937x. > > > > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> > > --- > > drivers/net/dsa/microchip/lan937x_main.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/drivers/net/dsa/microchip/lan937x_main.c > > b/drivers/net/dsa/microchip/lan937x_main.c > > index daedd2bf20c1..9c1fe38efd1a 100644 > > --- a/drivers/net/dsa/microchip/lan937x_main.c > > +++ b/drivers/net/dsa/microchip/lan937x_main.c > > @@ -401,11 +401,6 @@ int lan937x_setup(struct dsa_switch *ds) > > return ret; > > } > > > > - /* The VLAN aware is a global setting. Mixed vlan > > - * filterings are not supported. > > - */ > > - ds->vlan_filtering_is_global = true; > > - > > You understand what this flag does, right? It ensures that if you > have > lan0 and lan1 under VLAN-aware br0, then lan2 which is standalone > will > declare NETIF_F_HW_VLAN_CTAG_FILTER. In turn, this makes the network > stack know that lan2 won't accept VLAN-tagged packets unless there is > an > 8021q interface with the given VID on top of it. This 8021q interface > calls vlan_vid_add() to populate the driver's VLAN RX filter with its > VID, and this gets translated into dsa_slave_vlan_rx_add_vid() which > ultimately reaches the driver's ->port_vlan_add() function. > > If VLAN filtering *is* a global setting, and looking at this call > from > ksz9477_port_vlan_filtering() which is not per port, I'd say it is: > > ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE, > true); > > then what would happen is that all VLAN tagged traffic would be > dropped > on the standalone lan2. > > I'd say that the ksz9477 is buggy for not declaring > vlan_filtering_is_global, > rather than encouraging you to delete it from lan937x. In turn, > fixing > ksz9477 would make setting this flag from a common location possible, > because ksz8 needs it too. I have done some study on this SW_VLAN_ENABLE bit. By default the pvid of the port is 1 and vlan port membership (0xNA04) is 0xff. So if the bit is 0, then unknown dest addr packets are broadcasted which is the default behaviour of switch. Now consider when the bit is 1, - If the invalid vlan packet is received, then based on drop invalid vid or unknown vid forward bit, packets are discarded or forwarded. - If the valid vlan packet is received, then broadcast to ports in vlan port membership list. The port membership register set using the ksz9477_cfg_port_member function. In summary, I infer that we can enable this bit in the port_setup and based on the port membership packets can be forwarded. Is my understanding correct? Can I remove this patch from this series and submit as the separate one?
On Tue, Aug 02, 2022 at 04:09:35PM +0000, Arun.Ramadoss@microchip.com wrote: > I have done some study on this SW_VLAN_ENABLE bit. By default the pvid > of the port is 1 and vlan port membership (0xNA04) is 0xff. So if the > bit is 0, then unknown dest addr packets are broadcasted which is the > default behaviour of switch. > Now consider when the bit is 1, > - If the invalid vlan packet is received, then based on drop invalid > vid or unknown vid forward bit, packets are discarded or forwarded. > - If the valid vlan packet is received, then broadcast to ports in vlan > port membership list. > The port membership register set using the ksz9477_cfg_port_member > function. > In summary, I infer that we can enable this bit in the port_setup and > based on the port membership packets can be forwarded. Is my > understanding correct? > Can I remove this patch from this series and submit as the separate > one? I'm not sure that the switch's capabilities are quite in line with the Linux kernel expectations if you always force the 802.1Q VLAN Enable bit to true. I am looking at Table 4-8: VLAN forwarding from the KSZ9563 datasheet, and it says that when the "Unknown VID forward" bit is set and we are in VLAN enable mode, packets are forwarded to the Unknown VID packet forward list regardless of ALU match (which is "don't care" in this table). In essence this is because the switch failed to resolve the unknown VID to a FID. Other switches have a default FID for this case, but it doesn't appear to hold true for KSZ. The last thing you want is for packets under a VLAN-unaware bridge to be always flooded to the ports in the Unknown VID packet forward list, and bypass ALU lookups.
diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c index daedd2bf20c1..9c1fe38efd1a 100644 --- a/drivers/net/dsa/microchip/lan937x_main.c +++ b/drivers/net/dsa/microchip/lan937x_main.c @@ -401,11 +401,6 @@ int lan937x_setup(struct dsa_switch *ds) return ret; } - /* The VLAN aware is a global setting. Mixed vlan - * filterings are not supported. - */ - ds->vlan_filtering_is_global = true; - /* Enable aggressive back off for half duplex & UNH mode */ lan937x_cfg(dev, REG_SW_MAC_CTRL_0, (SW_PAUSE_UNH_MODE | SW_NEW_BACKOFF | SW_AGGR_BACKOFF),
To have the similar implementation among the ksz switches, removed the vlan_filtering_is_global flag which is only present in the lan937x. Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> --- drivers/net/dsa/microchip/lan937x_main.c | 5 ----- 1 file changed, 5 deletions(-)