netdev - Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220830164226.ohmn6bkwagz6n3pg@skbuf>
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.