netdev - Re: DSA using cpsw and lan9303

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: <20220216170027.yrkj5r4zberrx3qb@skbuf>
Date:   Wed, 16 Feb 2022 19:00:27 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Måns Rullgård <mans@...sr.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org,
        Egil Hjelmeland <privat@...l-hjelmeland.no>,
        Andrew Lunn <andrew@...n.ch>,
        Juergen Borleis <jbe@...gutronix.de>,
        Grygorii Strashko <grygorii.strashko@...com>,
        lorenzo@...nel.org
Subject: Re: DSA using cpsw and lan9303

On Wed, Feb 16, 2022 at 04:26:34PM +0200, Vladimir Oltean wrote:
> On Wed, Feb 16, 2022 at 02:23:24PM +0000, Måns Rullgård wrote:
> > Vladimir Oltean <olteanv@...il.com> writes:
> > 
> > > On Wed, Feb 16, 2022 at 01:17:47PM +0000, Måns Rullgård wrote:
> > >> > Some complaints about accessing the CPU port as dsa_to_port(chip->ds, 0),
> > >> > but it's not the first place in this driver where that is done.
> > >> 
> > >> What would be the proper way to do it?
> > >
> > > Generally speaking:
> > >
> > > 	struct dsa_port *cpu_dp;
> > >
> > > 	dsa_switch_for_each_cpu_port(cpu_dp, ds)
> > > 		break;
> > >
> > > 	// use cpu_dp
> > >
> > > If your code runs after dsa_tree_setup_default_cpu(), which contains the
> > > "DSA: tree %d has no CPU port\n" check, you don't even need to check
> > > whether cpu_dp was found or not - it surely was. Everything that runs
> > > after dsa_register_switch() has completed successfully - for example the
> > > DSA ->setup() method - qualifies here.
> > 
> > In this particular driver, the setup function contains this:
> > 
> > 	/* Make sure that port 0 is the cpu port */
> > 	if (!dsa_is_cpu_port(ds, 0)) {
> > 		dev_err(chip->dev, "port 0 is not the CPU port\n");
> > 		return -EINVAL;
> > 	}
> > 
> > I take this to mean that port 0 is guaranteed to be the cpu port.  Of
> > course, it can't hurt to be thorough just in case that check is ever
> > removed.
> 
> Yes, I saw that, and I said that there are other places in the driver
> that assume port 0 is the CPU port. Although I don't know why that is,
> if the switch can only operate like that, etc. I just pointed out how it
> would be preferable to get a hold of the CPU port in a regular DSA
> driver without any special constraints.

Ah, silly me, I should have paid more attention on where you're actually
inserting the code. You could have done:

static int lan9303_port_enable(struct dsa_switch *ds, int port,
			       struct phy_device *phy)
{
	struct dsa_port *dp = dsa_to_port(ds, port);
	struct lan9303 *chip = ds->priv;

	if (!dsa_port_is_user(dp))
		return 0;

	vlan_vid_add(dp->cpu_dp->master, htons(ETH_P_8021Q), port);

	return lan9303_enable_processing_port(chip, port);
}

the advantage being that if this driver ever supports the remapping of
the CPU port, or multiple CPU ports, this logic wouldn't need to be
changed, as it also conveys the user-to-CPU port affinity.

Anyway, doesn't really matter, and you certainly don't need to resend
for this. Sorry again for not paying too much attention.