Page MenuHomeFreeBSD

fixed vlangroup opration at RT3050.
ClosedPublic

Authored by yamori813_yahoo.co.jp on Feb 15 2017, 8:50 AM.

Details

Summary

original code can't modify vlangroup at RT3050. check only member bit untag setting.

Test Plan

FON2405(RT3050)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

yamori813_yahoo.co.jp retitled this revision from to fixed vlangroup opration at RT3050..
yamori813_yahoo.co.jp updated this object.
yamori813_yahoo.co.jp edited the test plan for this revision. (Show Details)
sgalabov edited edge metadata.Feb 15 2017, 9:17 AM

Can you explain the problem in a bit more detail?
The code you're suggesting to change (from memory) is intended to do the following:

  1. check which ports are configured to untag VLANs
  2. get the subset of VLAN member ports that are also untagged.
  3. compare the 2 values and if they match - go ahead; if they don't match - error out.

The idea is that if you want to add a port as an untagged member of a certain VLAN it should already be configured as a port that passes only untagged traffic (for the case of RT305x, which can't do per-VLAN untagging, or for the case of, say RT5350, which can do per-VLAN untagging or per-port untagging, based on bit POC2_UNTAG_VLAN in the MTKSWITCH_POC2 register.

Can you describe the setup that is failing for you? Especially, if you're trying to add ports as untagged members of a given VLAN, have all these been also configured as untagged ports prior to this?

OK, one issue exists with the existing code:

  • if more ports are configured as "untagged" than are configured as "untagged members" for the given VLAN the check would fail.

But there's an issue with the proposed change too - it basically changes the semantics of the entire check.

Probably we should change the code like so:

if ((val & POC2_UNTAG_VLAN) == 0 ||
    sc->sc_switchtype == MTK_SWITCH_RT3050) {
        val &= VUB_MASK;
        tmp = v->es_untagged_ports & v->es_member_ports;
        if ((val & tmp) != tmp) {
                /* Cannot accomodate request */
                MTKSWITCH_UNLOCK(sc);
                return (ENOTSUP);
        }
}

if delete to member in vlangroup the current code make error.

% etherswitchcfg vlangroup0 members 0,1,2,3,5,6
etherswitchcfg: ioctl(IOETHERSWITCHSETVLANGROUP): Operation not supported

also can't add vlangroup.

% etherswitchcfg vlangroup1 members 0,1
etherswitchcfg: ioctl(IOETHERSWITCHSETVLANGROUP): Operation not supported

if statement is same as current code. But POC2_UNTAG_VLAN don't have RT3050. I think check RT3050 first is good.

This is always false in patch.

tmp = v->es_untagged_ports & v->es_member_ports;
/* fail if untagged members are not a subset of all members */
if (tmp != v->es_untagged_ports) {

Because of all v->es_untagged_ports bits contain in v->es_member_ports.

This is always false in patch.

tmp = v->es_untagged_ports & v->es_member_ports;
/* fail if untagged members are not a subset of all members */
if (tmp != v->es_untagged_ports) {

Because of all v->es_untagged_ports bits contain in v->es_member_ports.

Yes, it's expected to be false, but IMHO it makes the code flow more readable and checks for conditions that can't be satisfied in a more consistent way. The question is does it work right now?

# etherswitchcfg vlangroup0 members 0,6   <- work
vlangroup0:
        vlan: 1
        members 0,6
# etherswitchcfg vlangroup1 members 0,1  <- work
vlangroup1:
        vlan: 0
        members 0,1
# etherswitchcfg port2 addtag
port2:
        pvid: 1
        flags=4<ADDTAG>
        media: Ethernet autoselect (none)
        status: no carrier
# etherswitchcfg vlangroup2 members 0,1,2   <-  Is this OK ?
vlangroup2:
        vlan: 0
        members 0,1,2t
#

OK, v2 patch is here:
https://people.freebsd.org/~sgalabov/mtkswitch_v2.patch

Please let me know if that's any better (it should be).

v2 patch is good.

# etherswitchcfg vlangroup0 members 0,6
vlangroup0:
        vlan: 1
        members 0,6
# etherswitchcfg vlangroup1 members 0,1
vlangroup1:
        vlan: 0
        members 0,1
# etherswitchcfg port2 addtag
port2:
        pvid: 1
        flags=4<ADDTAG>
        media: Ethernet autoselect (none)
        status: no carrier
# etherswitchcfg vlangroup2 members 0,1,2
etherswitchcfg: ioctl(IOETHERSWITCHSETVLANGROUP): Operation not supported
# etherswitchcfg vlangroup2 members 0,1,2t
vlangroup2:
        vlan: 0
        members 0,1,2t
#

Thanks

adrian accepted this revision.Feb 20 2017, 3:15 AM
adrian edited edge metadata.

ok, commit away please!

This revision is now accepted and ready to land.Feb 20 2017, 3:15 AM
This revision was automatically updated to reflect the committed changes.