lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 30 Aug 2022 19:42:26 +0300 From: Vladimir Oltean <olteanv@...il.com> To: Mattias Forsblad <mattias.forsblad@...il.com> Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>, Vivien Didelot <vivien.didelot@...il.com>, Florian Fainelli <f.fainelli@...il.com>, "David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com> Subject: Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches On Tue, Aug 30, 2022 at 07:35:15PM +0300, Vladimir Oltean wrote: > > +void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master, > > + bool operational) > > +{ > > + struct mv88e6xxx_chip *chip = ds->priv; > > + > > + if (operational) > > + chip->rmu.ops = &mv88e6xxx_bus_ops; > > + else > > + chip->rmu.ops = NULL; > > +} > > There is a subtle but very important point to be careful about here, > which is compatibility with multiple CPU ports. If there is a second DSA > master whose state flaps from up to down, this should not affect the > fact that you can still use RMU over the first DSA master. But in your > case it does, so this is a case of how not to write code that accounts > for that. > > In fact, given this fact, I think your function prototypes for > chip->info->ops->rmu_enable() are all wrong / not sufficiently > reflective of what the hardware can do. If the hardware has a bit mask > of ports on which RMU operations are possible, why hardcode using > dsa_switch_upstream_port() and not look at which DSA masters/CPU ports > are actually up? At least for the top-most switch. For downstream > switches we can use dsa_switch_upstream_port(), I guess (even that can > be refined, but I'm not aware of setups using multiple DSA links, where > each DSA link ultimately goes to a different upstream switch). Hit "send" too soon. Wanted to give the extra hint that the "master" pointer is given to you here for a reason. You can look at struct dsa_port *cpu_dp = master->dsa_ptr, and figure out the index of the CPU port which can be used for RMU operations. I see that the macros are constructed in a very strange way: #define MV88E6352_G1_CTL2_RMU_MODE_DISABLED 0x0000 #define MV88E6352_G1_CTL2_RMU_MODE_PORT_4 0x1000 #define MV88E6352_G1_CTL2_RMU_MODE_PORT_5 0x2000 #define MV88E6352_G1_CTL2_RMU_MODE_PORT_6 0x3000 it's as if this is actually a bit mask of ports, and they all can be combined together. The bit in G1_CTL2 whose state we can flip can be made to depend on the number of the CPU port attached to the DSA master which changed state.
Powered by blists - more mailing lists