| Message ID | 20241024103056.3201071-3-danishanwar@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce VLAN support in HSR | expand |
On 24/10/2024 11:30, MD Danish Anwar wrote: > From: Murali Karicheri <m-karicheri2@ti.com> > > This patch adds support for VLAN ctag based filtering at slave devices. > The slave ethernet device may be capable of filtering ethernet packets > based on VLAN ID. This requires that when the VLAN interface is created > over an HSR/PRP interface, it passes the VID information to the > associated slave ethernet devices so that it updates the hardware > filters to filter ethernet frames based on VID. This patch adds the > required functions to propagate the vid information to the slave > devices. > > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > --- > net/hsr/hsr_device.c | 71 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 70 insertions(+), 1 deletion(-) > > diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c > index 0ca47ebb01d3..ff586bdc2bde 100644 > --- a/net/hsr/hsr_device.c > +++ b/net/hsr/hsr_device.c > @@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device *dev, int change) > } > } > > +static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev, > + __be16 proto, u16 vid) > +{ > + struct hsr_port *port; > + struct hsr_priv *hsr; > + int ret = 0; > + > + hsr = netdev_priv(dev); > + > + hsr_for_each_port(hsr, port) { > + if (port->type == HSR_PT_MASTER) > + continue; > + > + ret = vlan_vid_add(port->dev, proto, vid); > + switch (port->type) { > + case HSR_PT_SLAVE_A: > + if (ret) { > + netdev_err(dev, "add vid failed for Slave-A\n"); > + return ret; > + } > + break; > + > + case HSR_PT_SLAVE_B: > + if (ret) { > + /* clean up Slave-A */ > + netdev_err(dev, "add vid failed for Slave-B\n"); > + vlan_vid_del(port->dev, proto, vid); > + return ret; > + } > + break; > + default: > + break; > + } > + } > + > + return 0; > +} This function doesn't match with hsr_ndo_vlan_rx_kill_vid(). vlan_vid_add() can potentially be executed for port->type equals to HSR_PT_INTERLINK, but the result will be ignored. And the vlan_vid_del() will never happen in this case. Is it desired behavior? Maybe it's better to synchronize add/del code and refactor error path to avoid coping the code? > + > +static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev, > + __be16 proto, u16 vid) > +{ > + struct hsr_port *port; > + struct hsr_priv *hsr; > + > + hsr = netdev_priv(dev); > + > + hsr_for_each_port(hsr, port) { > + if (port->type == HSR_PT_MASTER) > + continue; > + switch (port->type) { > + case HSR_PT_SLAVE_A: > + case HSR_PT_SLAVE_B: > + vlan_vid_del(port->dev, proto, vid); > + break; > + default: > + break; > + } > + } > + > + return 0; > +} > + > static const struct net_device_ops hsr_device_ops = { > .ndo_change_mtu = hsr_dev_change_mtu, > .ndo_open = hsr_dev_open, > @@ -523,6 +585,8 @@ static const struct net_device_ops hsr_device_ops = { > .ndo_change_rx_flags = hsr_change_rx_flags, > .ndo_fix_features = hsr_fix_features, > .ndo_set_rx_mode = hsr_set_rx_mode, > + .ndo_vlan_rx_add_vid = hsr_ndo_vlan_rx_add_vid, > + .ndo_vlan_rx_kill_vid = hsr_ndo_vlan_rx_kill_vid, > }; > > static const struct device_type hsr_type = { > @@ -569,7 +633,8 @@ void hsr_dev_setup(struct net_device *dev) > > dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA | > NETIF_F_GSO_MASK | NETIF_F_HW_CSUM | > - NETIF_F_HW_VLAN_CTAG_TX; > + NETIF_F_HW_VLAN_CTAG_TX | > + NETIF_F_HW_VLAN_CTAG_FILTER; > > dev->features = dev->hw_features; > } > @@ -647,6 +712,10 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2], > (slave[1]->features & NETIF_F_HW_HSR_FWD)) > hsr->fwd_offloaded = true; > > + if ((slave[0]->features & NETIF_F_HW_VLAN_CTAG_FILTER) && > + (slave[1]->features & NETIF_F_HW_VLAN_CTAG_FILTER)) > + hsr_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER; > + > res = register_netdevice(hsr_dev); > if (res) > goto err_unregister;
Hi Vadim, On 10/24/2024 7:06 PM, Vadim Fedorenko wrote: > On 24/10/2024 11:30, MD Danish Anwar wrote: >> From: Murali Karicheri <m-karicheri2@ti.com> >> >> This patch adds support for VLAN ctag based filtering at slave devices. >> The slave ethernet device may be capable of filtering ethernet packets >> based on VLAN ID. This requires that when the VLAN interface is created >> over an HSR/PRP interface, it passes the VID information to the >> associated slave ethernet devices so that it updates the hardware >> filters to filter ethernet frames based on VID. This patch adds the >> required functions to propagate the vid information to the slave >> devices. >> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >> --- >> net/hsr/hsr_device.c | 71 +++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 70 insertions(+), 1 deletion(-) >> >> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c >> index 0ca47ebb01d3..ff586bdc2bde 100644 >> --- a/net/hsr/hsr_device.c >> +++ b/net/hsr/hsr_device.c >> @@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device >> *dev, int change) >> } >> } >> +static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev, >> + __be16 proto, u16 vid) >> +{ >> + struct hsr_port *port; >> + struct hsr_priv *hsr; >> + int ret = 0; >> + >> + hsr = netdev_priv(dev); >> + >> + hsr_for_each_port(hsr, port) { >> + if (port->type == HSR_PT_MASTER) >> + continue; >> + >> + ret = vlan_vid_add(port->dev, proto, vid); >> + switch (port->type) { >> + case HSR_PT_SLAVE_A: >> + if (ret) { >> + netdev_err(dev, "add vid failed for Slave-A\n"); >> + return ret; >> + } >> + break; >> + >> + case HSR_PT_SLAVE_B: >> + if (ret) { >> + /* clean up Slave-A */ >> + netdev_err(dev, "add vid failed for Slave-B\n"); >> + vlan_vid_del(port->dev, proto, vid); >> + return ret; >> + } >> + break; >> + default: >> + break; >> + } >> + } >> + >> + return 0; >> +} > > This function doesn't match with hsr_ndo_vlan_rx_kill_vid(). > vlan_vid_add() can potentially be executed for port->type > equals to HSR_PT_INTERLINK, but the result will be ignored. And > the vlan_vid_del() will never happen in this case. Is it desired > behavior? Maybe it's better to synchronize add/del code and refactor > error path to avoid coping the code? > The kill_vid / add_vid is not similar because during add_vid, if vlan_vid_add() succeeds for one port but fails for other, we need to delete it for the earlier port. We can only continue if vlan_vid_add() succeeds for both ports. That's the reason the switch case handling of add_vid can not match the same for kill_vid. Since cleanup of port is needed, it's not possible to synchronize add/kill code We only care about HSR_PT_SLAVE_A and HSR_PT_SLAVE_B here. So it's okay to ignore HSR_PT_INTERLINK. It's a desired behaviour here. >> + >> +static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev, >> + __be16 proto, u16 vid) >> +{ >> + struct hsr_port *port; >> + struct hsr_priv *hsr; >> + >> + hsr = netdev_priv(dev); >> + >> + hsr_for_each_port(hsr, port) { >> + if (port->type == HSR_PT_MASTER) >> + continue; >> + switch (port->type) { >> + case HSR_PT_SLAVE_A: >> + case HSR_PT_SLAVE_B: >> + vlan_vid_del(port->dev, proto, vid); >> + break; >> + default: >> + break; >> + } >> + } >> + >> + return 0; >> +} >> + >> static const struct net_device_ops hsr_device_ops = { >> .ndo_change_mtu = hsr_dev_change_mtu, >> .ndo_open = hsr_dev_open, >> @@ -523,6 +585,8 @@ static const struct net_device_ops hsr_device_ops = { >> .ndo_change_rx_flags = hsr_change_rx_flags, >> .ndo_fix_features = hsr_fix_features, >> .ndo_set_rx_mode = hsr_set_rx_mode, >> + .ndo_vlan_rx_add_vid = hsr_ndo_vlan_rx_add_vid, >> + .ndo_vlan_rx_kill_vid = hsr_ndo_vlan_rx_kill_vid, >> }; >> static const struct device_type hsr_type = { >> @@ -569,7 +633,8 @@ void hsr_dev_setup(struct net_device *dev) >> dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | >> NETIF_F_HIGHDMA | >> NETIF_F_GSO_MASK | NETIF_F_HW_CSUM | >> - NETIF_F_HW_VLAN_CTAG_TX; >> + NETIF_F_HW_VLAN_CTAG_TX | >> + NETIF_F_HW_VLAN_CTAG_FILTER; >> dev->features = dev->hw_features; >> } >> @@ -647,6 +712,10 @@ int hsr_dev_finalize(struct net_device *hsr_dev, >> struct net_device *slave[2], >> (slave[1]->features & NETIF_F_HW_HSR_FWD)) >> hsr->fwd_offloaded = true; >> + if ((slave[0]->features & NETIF_F_HW_VLAN_CTAG_FILTER) && >> + (slave[1]->features & NETIF_F_HW_VLAN_CTAG_FILTER)) >> + hsr_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER; >> + >> res = register_netdevice(hsr_dev); >> if (res) >> goto err_unregister; >
On 10/24/24 12:30, MD Danish Anwar wrote: > From: Murali Karicheri <m-karicheri2@ti.com> > > This patch adds support for VLAN ctag based filtering at slave devices. > The slave ethernet device may be capable of filtering ethernet packets > based on VLAN ID. This requires that when the VLAN interface is created > over an HSR/PRP interface, it passes the VID information to the > associated slave ethernet devices so that it updates the hardware > filters to filter ethernet frames based on VID. This patch adds the > required functions to propagate the vid information to the slave > devices. > > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > --- > net/hsr/hsr_device.c | 71 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 70 insertions(+), 1 deletion(-) > > diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c > index 0ca47ebb01d3..ff586bdc2bde 100644 > --- a/net/hsr/hsr_device.c > +++ b/net/hsr/hsr_device.c > @@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device *dev, int change) > } > } > > +static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev, > + __be16 proto, u16 vid) > +{ > + struct hsr_port *port; > + struct hsr_priv *hsr; > + int ret = 0; > + > + hsr = netdev_priv(dev); > + > + hsr_for_each_port(hsr, port) { > + if (port->type == HSR_PT_MASTER) > + continue; If the desired behavior is to ignore INTERLINK port, I think you should explicitly skip them here, otherwise you will end-up in a nondeterministic state. > + ret = vlan_vid_add(port->dev, proto, vid); > + switch (port->type) { > + case HSR_PT_SLAVE_A: > + if (ret) { > + netdev_err(dev, "add vid failed for Slave-A\n"); > + return ret; > + } > + break; > + > + case HSR_PT_SLAVE_B: > + if (ret) { > + /* clean up Slave-A */ > + netdev_err(dev, "add vid failed for Slave-B\n"); > + vlan_vid_del(port->dev, proto, vid); This code relies on a specific port_list order - which is actually respected at list creation time. Still such assumption looks fragile and may lead to long term bugs. I think would be better to refactor the above loop handling arbitrary HSR_PT_SLAVE_A, HSR_PT_SLAVE_B order. Guestimate is that the complexity will not increase measurably. > + return ret; > + } > + break; > + default: > + break; > + } > + } > + > + return 0; > +} > + > +static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev, > + __be16 proto, u16 vid) > +{ > + struct hsr_port *port; > + struct hsr_priv *hsr; > + > + hsr = netdev_priv(dev); > + > + hsr_for_each_port(hsr, port) { > + if (port->type == HSR_PT_MASTER) > + continue; I think it would be more consistent just removing the above statement... > + switch (port->type) { > + case HSR_PT_SLAVE_A: > + case HSR_PT_SLAVE_B: > + vlan_vid_del(port->dev, proto, vid); > + break; > + default:> + break; ... MASTER and INTERLINK port will be ignored anyway. > + } > + } > + > + return 0; > +} > + > static const struct net_device_ops hsr_device_ops = { > .ndo_change_mtu = hsr_dev_change_mtu, > .ndo_open = hsr_dev_open, Cheers, Paolo
Hi Paolo, On 31/10/24 8:07 pm, Paolo Abeni wrote: > > > On 10/24/24 12:30, MD Danish Anwar wrote: >> From: Murali Karicheri <m-karicheri2@ti.com> >> >> This patch adds support for VLAN ctag based filtering at slave devices. >> The slave ethernet device may be capable of filtering ethernet packets >> based on VLAN ID. This requires that when the VLAN interface is created >> over an HSR/PRP interface, it passes the VID information to the >> associated slave ethernet devices so that it updates the hardware >> filters to filter ethernet frames based on VID. This patch adds the >> required functions to propagate the vid information to the slave >> devices. >> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >> --- >> net/hsr/hsr_device.c | 71 +++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 70 insertions(+), 1 deletion(-) >> >> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c >> index 0ca47ebb01d3..ff586bdc2bde 100644 >> --- a/net/hsr/hsr_device.c >> +++ b/net/hsr/hsr_device.c >> @@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device *dev, int change) >> } >> } >> >> +static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev, >> + __be16 proto, u16 vid) >> +{ >> + struct hsr_port *port; >> + struct hsr_priv *hsr; >> + int ret = 0; >> + >> + hsr = netdev_priv(dev); >> + >> + hsr_for_each_port(hsr, port) { >> + if (port->type == HSR_PT_MASTER) >> + continue; > > If the desired behavior is to ignore INTERLINK port, I think you should > explicitly skip them here, otherwise you will end-up in a > nondeterministic state. > Sure, I will change this to hsr_for_each_port(hsr, port) { if (port->type == HSR_PT_MASTER || port->type = HSR_PT_INTERLINK) continue; >> + ret = vlan_vid_add(port->dev, proto, vid); >> + switch (port->type) { >> + case HSR_PT_SLAVE_A: >> + if (ret) { >> + netdev_err(dev, "add vid failed for Slave-A\n"); >> + return ret; >> + } >> + break; >> + >> + case HSR_PT_SLAVE_B: >> + if (ret) { >> + /* clean up Slave-A */ >> + netdev_err(dev, "add vid failed for Slave-B\n"); >> + vlan_vid_del(port->dev, proto, vid); > > This code relies on a specific port_list order - which is actually > respected at list creation time. Still such assumption looks fragile and > may lead to long term bugs. > Agreed. The code is expecting HSR_PT_SLAVE_A to come first and add vid for it. Then vid is added for HSR_PT_SLAVE_B. if vlan_vid_add fails for HSR_PT_SLAVE_B, vid is deleted for HSR_PT_SLAVE_A before returning. > I think would be better to refactor the above loop handling arbitrary > HSR_PT_SLAVE_A, HSR_PT_SLAVE_B order. Guestimate is that the complexity > will not increase measurably. > I understand this will be better. But how would I figure out which port came first. The current implementation is to add vid for first port. If it fails it returns. If it passes it adds vid for second port. If it fails it clears vid of first port and returns. If it passes function returns 0. Now how do I keep this behavior and also handle ports arbitrary. If the same order is not guaranteed to be preserved, how would I know which port came first so that it can be deleted if second port fails? One idea I have is to keep two boolean flags is_slave_a_added, is_slave_b_added. And based on these flags we can determine if cleanup is needed or not. The add function will then look like this, static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid) { bool is_slave_a_added, is_slave_b_added; struct hsr_port *port; struct hsr_priv *hsr; int ret = 0; hsr = netdev_priv(dev); hsr_for_each_port(hsr, port) { if (port->type == HSR_PT_MASTER || port->type = HSR_PT_INTERLINK) continue; ret = vlan_vid_add(port->dev, proto, vid); switch (port->type) { case HSR_PT_SLAVE_A: if (ret) { /* clean up Slave-B */ netdev_err(dev, "add vid failed for Slave-A\n"); if (is_slave_b_added) vlan_vid_del(port->dev, proto, vid); return ret; } else { is_slave_a_added = true; } break; case HSR_PT_SLAVE_B: if (ret) { /* clean up Slave-A */ netdev_err(dev, "add vid failed for Slave-B\n"); if (is_slave_a_added) vlan_vid_del(port->dev, proto, vid); return ret; } else { is_slave_b_added = true; } break; default: break; } } return 0; } Let me know if this is OK. Or if you have some other method to handle the ports arbitrary. >> + return ret; >> + } >> + break; >> + default: >> + break; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev, >> + __be16 proto, u16 vid) >> +{ >> + struct hsr_port *port; >> + struct hsr_priv *hsr; >> + >> + hsr = netdev_priv(dev); >> + >> + hsr_for_each_port(hsr, port) { >> + if (port->type == HSR_PT_MASTER) >> + continue; > > I think it would be more consistent just removing the above statement... > Sure. I'll do it. >> + switch (port->type) { >> + case HSR_PT_SLAVE_A: >> + case HSR_PT_SLAVE_B: >> + vlan_vid_del(port->dev, proto, vid); >> + break; >> + default:> + break; > > ... MASTER and INTERLINK port will be ignored anyway. > Sure. >> + } >> + } >> + >> + return 0; >> +} >> + >> static const struct net_device_ops hsr_device_ops = { >> .ndo_change_mtu = hsr_dev_change_mtu, >> .ndo_open = hsr_dev_open, > > Cheers, > > Paolo >
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c index 0ca47ebb01d3..ff586bdc2bde 100644 --- a/net/hsr/hsr_device.c +++ b/net/hsr/hsr_device.c @@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device *dev, int change) } } +static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev, + __be16 proto, u16 vid) +{ + struct hsr_port *port; + struct hsr_priv *hsr; + int ret = 0; + + hsr = netdev_priv(dev); + + hsr_for_each_port(hsr, port) { + if (port->type == HSR_PT_MASTER) + continue; + + ret = vlan_vid_add(port->dev, proto, vid); + switch (port->type) { + case HSR_PT_SLAVE_A: + if (ret) { + netdev_err(dev, "add vid failed for Slave-A\n"); + return ret; + } + break; + + case HSR_PT_SLAVE_B: + if (ret) { + /* clean up Slave-A */ + netdev_err(dev, "add vid failed for Slave-B\n"); + vlan_vid_del(port->dev, proto, vid); + return ret; + } + break; + default: + break; + } + } + + return 0; +} + +static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev, + __be16 proto, u16 vid) +{ + struct hsr_port *port; + struct hsr_priv *hsr; + + hsr = netdev_priv(dev); + + hsr_for_each_port(hsr, port) { + if (port->type == HSR_PT_MASTER) + continue; + switch (port->type) { + case HSR_PT_SLAVE_A: + case HSR_PT_SLAVE_B: + vlan_vid_del(port->dev, proto, vid); + break; + default: + break; + } + } + + return 0; +} + static const struct net_device_ops hsr_device_ops = { .ndo_change_mtu = hsr_dev_change_mtu, .ndo_open = hsr_dev_open, @@ -523,6 +585,8 @@ static const struct net_device_ops hsr_device_ops = { .ndo_change_rx_flags = hsr_change_rx_flags, .ndo_fix_features = hsr_fix_features, .ndo_set_rx_mode = hsr_set_rx_mode, + .ndo_vlan_rx_add_vid = hsr_ndo_vlan_rx_add_vid, + .ndo_vlan_rx_kill_vid = hsr_ndo_vlan_rx_kill_vid, }; static const struct device_type hsr_type = { @@ -569,7 +633,8 @@ void hsr_dev_setup(struct net_device *dev) dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA | NETIF_F_GSO_MASK | NETIF_F_HW_CSUM | - NETIF_F_HW_VLAN_CTAG_TX; + NETIF_F_HW_VLAN_CTAG_TX | + NETIF_F_HW_VLAN_CTAG_FILTER; dev->features = dev->hw_features; } @@ -647,6 +712,10 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2], (slave[1]->features & NETIF_F_HW_HSR_FWD)) hsr->fwd_offloaded = true; + if ((slave[0]->features & NETIF_F_HW_VLAN_CTAG_FILTER) && + (slave[1]->features & NETIF_F_HW_VLAN_CTAG_FILTER)) + hsr_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER; + res = register_netdevice(hsr_dev); if (res) goto err_unregister;