| Message ID | 1455296981-27998-4-git-send-email-vivien.didelot@savoirfairelinux.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 2/12/2016 8:09 PM, Vivien Didelot wrote: > The DSA drivers now have access to the VLAN prepare phase and the bridge > net_device. It is easier to check for overlapping bridges from within > the driver. Thus add such check in mv88e6xxx_port_vlan_prepare. > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > --- > drivers/net/dsa/mv88e6xxx.c | 64 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c > index 2e515e8..685dcb0 100644 > --- a/drivers/net/dsa/mv88e6xxx.c > +++ b/drivers/net/dsa/mv88e6xxx.c > @@ -1471,14 +1471,78 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid, > return 0; > } > > +static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port, > + u16 vid_begin, u16 vid_end) > +{ > + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); > + struct mv88e6xxx_vtu_stu_entry vlan; > + int i, err; > + > + if (!vid_begin) > + return -EOPNOTSUPP; > + > + mutex_lock(&ps->smi_mutex); > + > + err = _mv88e6xxx_vtu_vid_write(ds, vid_begin - 1); > + if (err) > + goto unlock; > + > + do { > + err = _mv88e6xxx_vtu_getnext(ds, &vlan); > + if (err) > + goto unlock; Why are you not using *break*? > + > + if (!vlan.valid) > + break; > + > + if (vlan.vid > vid_end) > + break; > + > + for (i = 0; i < ps->num_ports; ++i) { > + if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i)) > + continue; > + > + if (vlan.data[i] == > + GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER) > + continue; > + > + if (ps->ports[i].bridge_dev == > + ps->ports[port].bridge_dev) > + break; /* same bridge, check next VLAN */ > + > + netdev_warn(ds->ports[port], > + "hardware VLAN %d already used by %s\n", > + vlan.vid, > + netdev_name(ps->ports[i].bridge_dev)); > + err = -EOPNOTSUPP; > + goto unlock; > + } Why not *break*? > + } while (vlan.vid < vid_end); > + > +unlock: > + mutex_unlock(&ps->smi_mutex); > + > + return err; > +} > + [...] MBR, Sergei
Hi Sergei, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes: > Hello. > > On 2/12/2016 8:09 PM, Vivien Didelot wrote: > >> The DSA drivers now have access to the VLAN prepare phase and the bridge >> net_device. It is easier to check for overlapping bridges from within >> the driver. Thus add such check in mv88e6xxx_port_vlan_prepare. >> >> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> >> --- >> drivers/net/dsa/mv88e6xxx.c | 64 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 64 insertions(+) >> >> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c >> index 2e515e8..685dcb0 100644 >> --- a/drivers/net/dsa/mv88e6xxx.c >> +++ b/drivers/net/dsa/mv88e6xxx.c >> @@ -1471,14 +1471,78 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid, >> return 0; >> } >> >> +static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port, >> + u16 vid_begin, u16 vid_end) >> +{ >> + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); >> + struct mv88e6xxx_vtu_stu_entry vlan; >> + int i, err; >> + >> + if (!vid_begin) >> + return -EOPNOTSUPP; >> + >> + mutex_lock(&ps->smi_mutex); >> + >> + err = _mv88e6xxx_vtu_vid_write(ds, vid_begin - 1); >> + if (err) >> + goto unlock; >> + >> + do { >> + err = _mv88e6xxx_vtu_getnext(ds, &vlan); >> + if (err) >> + goto unlock; > > Why are you not using *break*? I use goto for explicit error handling, and break for expected flow. >> + >> + if (!vlan.valid) >> + break; >> + >> + if (vlan.vid > vid_end) >> + break; >> + >> + for (i = 0; i < ps->num_ports; ++i) { >> + if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i)) >> + continue; >> + >> + if (vlan.data[i] == >> + GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER) >> + continue; >> + >> + if (ps->ports[i].bridge_dev == >> + ps->ports[port].bridge_dev) >> + break; /* same bridge, check next VLAN */ >> + >> + netdev_warn(ds->ports[port], >> + "hardware VLAN %d already used by %s\n", >> + vlan.vid, >> + netdev_name(ps->ports[i].bridge_dev)); >> + err = -EOPNOTSUPP; >> + goto unlock; >> + } > > Why not *break*? Because here it would only break the for loop, and not the while loop. > >> + } while (vlan.vid < vid_end); >> + >> +unlock: >> + mutex_unlock(&ps->smi_mutex); >> + >> + return err; >> +} >> + > [...] > > MBR, Sergei Thanks, -v
On 02/13/2016 10:53 PM, Vivien Didelot wrote: >>> The DSA drivers now have access to the VLAN prepare phase and the bridge >>> net_device. It is easier to check for overlapping bridges from within >>> the driver. Thus add such check in mv88e6xxx_port_vlan_prepare. >>> >>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> >>> --- >>> drivers/net/dsa/mv88e6xxx.c | 64 +++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 64 insertions(+) >>> >>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c >>> index 2e515e8..685dcb0 100644 >>> --- a/drivers/net/dsa/mv88e6xxx.c >>> +++ b/drivers/net/dsa/mv88e6xxx.c >>> @@ -1471,14 +1471,78 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid, >>> return 0; >>> } >>> >>> +static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port, >>> + u16 vid_begin, u16 vid_end) >>> +{ >>> + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); >>> + struct mv88e6xxx_vtu_stu_entry vlan; >>> + int i, err; >>> + >>> + if (!vid_begin) >>> + return -EOPNOTSUPP; >>> + >>> + mutex_lock(&ps->smi_mutex); >>> + >>> + err = _mv88e6xxx_vtu_vid_write(ds, vid_begin - 1); >>> + if (err) >>> + goto unlock; >>> + >>> + do { >>> + err = _mv88e6xxx_vtu_getnext(ds, &vlan); >>> + if (err) >>> + goto unlock; >> >> Why are you not using *break*? > > I use goto for explicit error handling, and break for expected flow. Thought you'd say so. :-) I still think *break* is preferable... >>> + >>> + if (!vlan.valid) >>> + break; >>> + >>> + if (vlan.vid > vid_end) >>> + break; >>> + >>> + for (i = 0; i < ps->num_ports; ++i) { >>> + if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i)) >>> + continue; >>> + >>> + if (vlan.data[i] == >>> + GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER) >>> + continue; >>> + >>> + if (ps->ports[i].bridge_dev == >>> + ps->ports[port].bridge_dev) >>> + break; /* same bridge, check next VLAN */ >>> + >>> + netdev_warn(ds->ports[port], >>> + "hardware VLAN %d already used by %s\n", >>> + vlan.vid, >>> + netdev_name(ps->ports[i].bridge_dev)); >>> + err = -EOPNOTSUPP; >>> + goto unlock; >>> + } >> >> Why not *break*? > > Because here it would only break the for loop, and not the while loop. Oops, I overlooked the *for* loop. Sorry about that. >> >>> + } while (vlan.vid < vid_end); >>> + >>> +unlock: >>> + mutex_unlock(&ps->smi_mutex); >>> + >>> + return err; >>> +} >>> + >> [...] > > Thanks, > -v MBR, Sergei
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c index 2e515e8..685dcb0 100644 --- a/drivers/net/dsa/mv88e6xxx.c +++ b/drivers/net/dsa/mv88e6xxx.c @@ -1471,14 +1471,78 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid, return 0; } +static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port, + u16 vid_begin, u16 vid_end) +{ + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); + struct mv88e6xxx_vtu_stu_entry vlan; + int i, err; + + if (!vid_begin) + return -EOPNOTSUPP; + + mutex_lock(&ps->smi_mutex); + + err = _mv88e6xxx_vtu_vid_write(ds, vid_begin - 1); + if (err) + goto unlock; + + do { + err = _mv88e6xxx_vtu_getnext(ds, &vlan); + if (err) + goto unlock; + + if (!vlan.valid) + break; + + if (vlan.vid > vid_end) + break; + + for (i = 0; i < ps->num_ports; ++i) { + if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i)) + continue; + + if (vlan.data[i] == + GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER) + continue; + + if (ps->ports[i].bridge_dev == + ps->ports[port].bridge_dev) + break; /* same bridge, check next VLAN */ + + netdev_warn(ds->ports[port], + "hardware VLAN %d already used by %s\n", + vlan.vid, + netdev_name(ps->ports[i].bridge_dev)); + err = -EOPNOTSUPP; + goto unlock; + } + } while (vlan.vid < vid_end); + +unlock: + mutex_unlock(&ps->smi_mutex); + + return err; +} + int mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port, const struct switchdev_obj_port_vlan *vlan, struct switchdev_trans *trans) { + int err; + /* We reserve a few VLANs to isolate unbridged ports */ if (vlan->vid_end >= 4000) return -EOPNOTSUPP; + /* If the requested port doesn't belong to the same bridge as the VLAN + * members, do not support it (yet) and fallback to software VLAN. + */ + err = mv88e6xxx_port_check_hw_vlan(ds, port, vlan->vid_begin, + vlan->vid_end); + if (err) + return err; + /* We don't need any dynamic resource from the kernel (yet), * so skip the prepare phase. */
The DSA drivers now have access to the VLAN prepare phase and the bridge net_device. It is easier to check for overlapping bridges from within the driver. Thus add such check in mv88e6xxx_port_vlan_prepare. Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- drivers/net/dsa/mv88e6xxx.c | 64 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)