Re: [PATCH net-next v2 2/6] net: dsa: mv88e6xxx: move ATU code in its own file
From: Andrew LunnDate: Fri, 17 Feb 2017 23:36:51 +0100 > Please take your time to do this right. Lots of small patches, which > are obviously correct. Agreed.
Re: [PATCH net-next v2 2/6] net: dsa: mv88e6xxx: move ATU code in its own file
> Note that we are very close from 4.10, so unless there is a major issue > in the patchset, I'd prefer not to respin new versions. I'd gladly send > cosmetics fixup later though. I'm waiting for this patchset to land in > net-next to send the second one ready for cross-chip bridging in DSA. I know we are very close to v4.10. But i don't think these changes alone make the 6390 work properly. It appears that we still have: if (mv88e6xxx_6352_family(chip) || mv88e6xxx_6351_family(chip) || mv88e6xxx_6165_family(chip) || mv88e6xxx_6097_family(chip) || mv88e6xxx_6320_family(chip) || mv88e6xxx_6341_family(chip)) { /* Port ATU control: disable limiting the number of * address database entries that this port is allowed * to use. */ err = mv88e6xxx_port_write(chip, port, PORT_ATU_CONTROL, 0x); /* Priority Override: disable DA, SA and VTU priority * override. */ err = mv88e6xxx_port_write(chip, port, PORT_PRI_OVERRIDE, 0x); if (err) return err; } to deal with. I have been working on 6390 for a long time, and would like to see it working properly, but i'm not happy with the non-obvious changes here, and the structure of these patches. Please take your time to do this right. Lots of small patches, which are obviously correct. Andrew
Re: [PATCH net-next v2 2/6] net: dsa: mv88e6xxx: move ATU code in its own file
Hi Andrew, Andrew Lunnwrites: > This seems to be more than renaming a few functions. There looks to be > real changes here. I think these changes should be split out into a > separate patch with an explanation what is being changed. Keep this > patch for plain renames. > > It would also be easier to review if the patch just moved the code, no > changes. Then have patches which clean up the API. It is hard to see > what is move and what is cleanup. Plain move needs little review, > cleanup needs more review. With the current patch, it is hard to see > which is which. I understand your concerns about that. I've started that way (just moving code) but this ended up messier because of the naming convention which would required renaming and more diffstats. I found it actually easier to review the new code at once by reading all the addition block. Note that we are very close from 4.10, so unless there is a major issue in the patchset, I'd prefer not to respin new versions. I'd gladly send cosmetics fixup later though. I'm waiting for this patchset to land in net-next to send the second one ready for cross-chip bridging in DSA. Thanks, Vivien
Re: [PATCH net-next v2 2/6] net: dsa: mv88e6xxx: move ATU code in its own file
On Fri, Feb 17, 2017 at 10:05:27AM -0500, Vivien Didelot wrote: > Move the Global (1) ATU related code in its own file, and export the > necessary primitives. > > Use that opportunity to provide a cleaner API for the ATU, by renaming a > few underscore prefixed functions, and members of the > mv88e6xxx_atu_entry structures. > static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int > port, > const unsigned char *addr, u16 vid, > u8 state) > { > + struct mv88e6xxx_atu_entry entry = { 0 }; > struct mv88e6xxx_vtu_entry vlan; > - struct mv88e6xxx_atu_entry entry; > int err; > > /* Null VLAN ID corresponds to the port private database */ > @@ -2107,21 +1918,32 @@ static int mv88e6xxx_port_db_load_purge(struct > mv88e6xxx_chip *chip, int port, > if (err) > return err; > > - err = mv88e6xxx_atu_get(chip, vlan.fid, addr, ); > + entry.fid = vlan.fid; > + ether_addr_copy(entry.mac, addr); > + eth_addr_dec(entry.mac); > + err = mv88e6xxx_atu_getnext(chip, ); > if (err) > return err; > > + /* Initialize a fresh ATU entry if it isn't found */ > + if (entry.state == GLOBAL_ATU_DATA_STATE_UNUSED || > + !ether_addr_equal(entry.mac, addr)) { > + memset(, 0, sizeof(entry)); > + ether_addr_copy(entry.mac, addr); > + entry.fid = vlan.fid; > + } > + This seems to be more than renaming a few functions. There looks to be real changes here. I think these changes should be split out into a separate patch with an explanation what is being changed. Keep this patch for plain renames. It would also be easier to review if the patch just moved the code, no changes. Then have patches which clean up the API. It is hard to see what is move and what is cleanup. Plain move needs little review, cleanup needs more review. With the current patch, it is hard to see which is which. Andrew