| Message ID | 20230627094207.3385231-2-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b4638af8885af93cd70351081da1909c59342440 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix PTP received on wrong port with bridged SJA1105 DSA | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Jun 27, 2023 at 12:42:06PM +0300, Vladimir Oltean wrote: > Link-local traffic on bridged SJA1105 ports is sometimes tagged by the > hardware with source port information (when the port is under a VLAN > aware bridge). > > The tag_8021q source port identification has become more loose > ("imprecise") and will report a plausible rather than exact bridge port, > when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local > traffic always needs to know the precise source port. > > Modify the driver logic (and therefore: the tagging protocol itself) to > always include the source port information with link-local packets, > regardless of whether the port is standalone, under a VLAN-aware or > VLAN-unaware bridge. This makes it possible for the tagging driver to > give priority to that information over the tag_8021q VLAN header. > > The big drawback with INCL_SRCPT is that it makes it impossible to > distinguish between an original MAC DA of 01:80:C2:XX:YY:ZZ and > 01:80:C2:AA:BB:ZZ, because the tagger just patches MAC DA bytes 3 and 4 > with zeroes. Only if PTP RX timestamping is enabled, the switch will > generate a META follow-up frame containing the RX timestamp and the > original bytes 3 and 4 of the MAC DA. Those will be used to patch up the > original packet. Nonetheless, in the absence of PTP RX timestamping, we > have to live with this limitation, since it is more important to have > the more precise source port information for link-local traffic. > > Fixes: d7f9787a763f ("net: dsa: tag_8021q: add support for imprecise RX based on the VBID") > Fixes: 91495f21fcec ("net: dsa: tag_8021q: replace the SVL bridging with VLAN-unaware IVL bridging") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Tue, 2023-06-27 at 12:42 +0300, Vladimir Oltean wrote: > Link-local traffic on bridged SJA1105 ports is sometimes tagged by the > hardware with source port information (when the port is under a VLAN > aware bridge). > > The tag_8021q source port identification has become more loose > ("imprecise") and will report a plausible rather than exact bridge port, > when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local > traffic always needs to know the precise source port. > > Modify the driver logic (and therefore: the tagging protocol itself) to > always include the source port information with link-local packets, > regardless of whether the port is standalone, under a VLAN-aware or > VLAN-unaware bridge. This makes it possible for the tagging driver to > give priority to that information over the tag_8021q VLAN header. > > The big drawback with INCL_SRCPT is that it makes it impossible to > distinguish between an original MAC DA of 01:80:C2:XX:YY:ZZ and > 01:80:C2:AA:BB:ZZ, because the tagger just patches MAC DA bytes 3 and 4 > with zeroes. Only if PTP RX timestamping is enabled, the switch will > generate a META follow-up frame containing the RX timestamp and the > original bytes 3 and 4 of the MAC DA. Those will be used to patch up the > original packet. Nonetheless, in the absence of PTP RX timestamping, we > have to live with this limitation, since it is more important to have > the more precise source port information for link-local traffic. What if 2 different DSA are under the same linux bridge, so that the host has to forward in S/W the received frames? (and DA is incomplete) It looks like that such frames will never reach the relevant destination? Is such setup possible/relevant? Thanks, Paolo
On Thu, 2023-06-29 at 13:19 +0300, Vladimir Oltean wrote: > On Thu, Jun 29, 2023 at 11:36:38AM +0200, Paolo Abeni wrote: > > > The big drawback with INCL_SRCPT is that it makes it impossible to > > > distinguish between an original MAC DA of 01:80:C2:XX:YY:ZZ and > > > 01:80:C2:AA:BB:ZZ, because the tagger just patches MAC DA bytes 3 and 4 > > > with zeroes. Only if PTP RX timestamping is enabled, the switch will > > > generate a META follow-up frame containing the RX timestamp and the > > > original bytes 3 and 4 of the MAC DA. Those will be used to patch up the > > > original packet. Nonetheless, in the absence of PTP RX timestamping, we > > > have to live with this limitation, since it is more important to have > > > the more precise source port information for link-local traffic. > > > > What if 2 different DSA are under the same linux bridge, so that the > > host has to forward in S/W the received frames? (and DA is incomplete) > > > > It looks like that such frames will never reach the relevant > > destination? > > > > Is such setup possible/relevant? > > > > Thanks, > > > > Paolo > > > > They will have an incorrect MAC DA, with all the consequences of that. > > Given the fact that the incl_srcpt was enabled up until now for the > vlan_filtering=1 bridge case only, this was already possible to see. > However it was never reported to me as being a problem, unlike what > is being fixed here. Ok, the above sounds like a good enough reply to me. > I see no other escape than to unconditionally enable the send_meta > options as well, so that the overwritten MAC DA bytes can always be > reconstructed from the upcoming meta frames, even though the RX > timestamp (main payload of those meta frames) may or may not be useful. > Doing that might also have the benefit that it simplifies the code, > removing the need for tagger_data->rxtstamp_set_state() and > tagger_data->rxtstamp_get_state(), because with that simplification, > the tagger will always expect meta frames. > > Because of the lack of complaints, I was considering that as net-next > material though. [I'm mixing replies to your 2 emails here, I hope overall this is still human parsable ;) ] Quickly skimming over the patch you shared I *think* it could be -net material, too. Given the mentioned lack of complains for the potential issue, I think it could be a follow-up to this series. I'm applying it right now. Thanks! Paolo
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index a55a6436fc05..dd154b2b9680 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -866,11 +866,11 @@ static int sja1105_init_general_params(struct sja1105_private *priv) .hostprio = 7, .mac_fltres1 = SJA1105_LINKLOCAL_FILTER_A, .mac_flt1 = SJA1105_LINKLOCAL_FILTER_A_MASK, - .incl_srcpt1 = false, + .incl_srcpt1 = true, .send_meta1 = false, .mac_fltres0 = SJA1105_LINKLOCAL_FILTER_B, .mac_flt0 = SJA1105_LINKLOCAL_FILTER_B_MASK, - .incl_srcpt0 = false, + .incl_srcpt0 = true, .send_meta0 = false, /* Default to an invalid value */ .mirr_port = priv->ds->num_ports, @@ -2405,11 +2405,6 @@ int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled, general_params->tpid = tpid; /* EtherType used to identify outer tagged (S-tag) VLAN traffic */ general_params->tpid2 = tpid2; - /* When VLAN filtering is on, we need to at least be able to - * decode management traffic through the "backup plan". - */ - general_params->incl_srcpt1 = enabled; - general_params->incl_srcpt0 = enabled; for (port = 0; port < ds->num_ports; port++) { if (dsa_is_unused_port(ds, port))
Link-local traffic on bridged SJA1105 ports is sometimes tagged by the hardware with source port information (when the port is under a VLAN aware bridge). The tag_8021q source port identification has become more loose ("imprecise") and will report a plausible rather than exact bridge port, when under a bridge (be it VLAN-aware or VLAN-unaware). But link-local traffic always needs to know the precise source port. Modify the driver logic (and therefore: the tagging protocol itself) to always include the source port information with link-local packets, regardless of whether the port is standalone, under a VLAN-aware or VLAN-unaware bridge. This makes it possible for the tagging driver to give priority to that information over the tag_8021q VLAN header. The big drawback with INCL_SRCPT is that it makes it impossible to distinguish between an original MAC DA of 01:80:C2:XX:YY:ZZ and 01:80:C2:AA:BB:ZZ, because the tagger just patches MAC DA bytes 3 and 4 with zeroes. Only if PTP RX timestamping is enabled, the switch will generate a META follow-up frame containing the RX timestamp and the original bytes 3 and 4 of the MAC DA. Those will be used to patch up the original packet. Nonetheless, in the absence of PTP RX timestamping, we have to live with this limitation, since it is more important to have the more precise source port information for link-local traffic. Fixes: d7f9787a763f ("net: dsa: tag_8021q: add support for imprecise RX based on the VBID") Fixes: 91495f21fcec ("net: dsa: tag_8021q: replace the SVL bridging with VLAN-unaware IVL bridging") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v1->v2: none drivers/net/dsa/sja1105/sja1105_main.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)