netdev - Re: [RFC PATCH net-next 3/4] net: systemport: use standard netdevice notifier to detect DSA presence

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: <20201219121237.tq3pxquyaq4q547t@skbuf>
Date:   Sat, 19 Dec 2020 12:12:38 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Florian Fainelli <f.fainelli@...il.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "bcm-kernel-feedback-list@...adcom.com" 
        <bcm-kernel-feedback-list@...adcom.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH net-next 3/4] net: systemport: use standard netdevice
 notifier to detect DSA presence

On Fri, Dec 18, 2020 at 08:08:56PM -0800, Florian Fainelli wrote:
> On 12/18/2020 2:38 PM, Vladimir Oltean wrote:
> > The SYSTEMPORT driver maps each port of the embedded Broadcom DSA switch
> > port to a certain queue of the master Ethernet controller. For that it
> > currently uses a dedicated notifier infrastructure which was added in
> > commit 60724d4bae14 ("net: dsa: Add support for DSA specific notifiers").
> >
> > However, since commit 2f1e8ea726e9 ("net: dsa: link interfaces with the
> > DSA master to get rid of lockdep warnings"), DSA is actually an upper of
> > the Broadcom SYSTEMPORT as far as the netdevice adjacency lists are
> > concerned. So naturally, the plain NETDEV_CHANGEUPPER net device notifiers
> > are emitted. It looks like there is enough API exposed by DSA to the
> > outside world already to make the call_dsa_notifiers API redundant. So
> > let's convert its only user to plain netdev notifiers.
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
>
> The CHANGEUPPER has a slightly different semantic than the current DSA
> notifier, and so events that would look like this during
> bcm_sysport_init_tx_ring() (good):
>
> [    6.781064] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0
> [    6.789214] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0
> [    6.797337] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0
> [    6.805464] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0
> [    6.813583] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1
> [    6.821701] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1
> [    6.829819] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1
> [    6.837944] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1
> [    6.846063] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=2
> [    6.854183] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=2
> [    6.862303] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=2
> [    6.870425] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=2
> [    6.878544] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=5
> [    6.886663] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=5
> [    6.894783] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=5
> [    6.902906] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=5
>
> now we are getting (bad):
>
> [    6.678157] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0
> [    6.686302] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0
> [    6.694434] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0
> [    6.702554] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0
> [    6.710679] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0
> [    6.718797] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0
> [    6.726914] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0
> [    6.735033] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0
> [    6.743156] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1
> [    6.751275] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1
> [    6.759395] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1
> [    6.767514] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1
> [    6.775636] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1
> [    6.783754] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1
> [    6.791874] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1
> [    6.799992] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1
>
> Looking further in bcm_sysport_map_queues() we are getting the following:
>
>     6.223042] brcm-systemport 9300000.ethernet eth0: mapping q=0, p=0
> [    6.229369] brcm-systemport 9300000.ethernet eth0: mapping q=1, p=0
> [    6.235659] brcm-systemport 9300000.ethernet eth0: mapping q=2, p=0
> [    6.241945] brcm-systemport 9300000.ethernet eth0: mapping q=3, p=0
> [    6.248232] brcm-systemport 9300000.ethernet eth0: mapping q=4, p=0
> [    6.254519] brcm-systemport 9300000.ethernet eth0: mapping q=5, p=0
> [    6.260805] brcm-systemport 9300000.ethernet eth0: mapping q=6, p=0
> [    6.267092] brcm-systemport 9300000.ethernet eth0: mapping q=7, p=0
>
> which means that the call to netif_set_real_num_tx_queues() that is
> executed for the SYSTEMPORT Lite is not taking effect because it is
> after the register_netdevice(). Insead of using a CHANGEUPPER notifier,
> we can use a REGISTER notifier event and doing that works just fine with
> the same semantics as the DSA notifier being removed. This incremental
> patch on top of your patch works for me (tm):
>
> https://github.com/ffainelli/linux/commit/f5095ab5c1f31db133d62273928b224674626b75

This is odd, the netif_set_real_num_tx_queues() call should not fail or
be ignored even if the interface was registered. I had tested this already
on my enetc + felix combo on LS1028A.

static int enetc_dsa_join(struct net_device *dev,
			  struct net_device *slave_dev)
{
	int err;

	netdev_err(slave_dev, "Hello!\n");

	err = netif_set_real_num_tx_queues(slave_dev,
					   slave_dev->num_tx_queues / 2);
	if (err)
		return err;

	netdev_err(slave_dev, "New number of real TX queues: %d\n",
		   slave_dev->real_num_tx_queues);

	return 0;
}

prints:

[    7.002328] mscc_felix 0000:00:00.5 swp0 (uninitialized): PHY [0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[    7.021190] mscc_felix 0000:00:00.5 swp0: Hello!
[    7.028657] mscc_felix 0000:00:00.5 swp0: New number of real TX queues: 4
[    7.035589] mscc_felix 0000:00:00.5 swp0: Hello!
[    7.040380] mscc_felix 0000:00:00.5 swp0: New number of real TX queues: 4
[    7.290236] mscc_felix 0000:00:00.5 swp1 (uninitialized): PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[    7.314383] mscc_felix 0000:00:00.5 swp1: Hello!
[    7.321292] mscc_felix 0000:00:00.5 swp1: New number of real TX queues: 4
[    7.328223] mscc_felix 0000:00:00.5 swp1: Hello!
[    7.332967] mscc_felix 0000:00:00.5 swp1: New number of real TX queues: 4
[    7.574254] mscc_felix 0000:00:00.5 swp2 (uninitialized): PHY [0000:00:00.3:12] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[    7.598431] mscc_felix 0000:00:00.5 swp2: Hello!
[    7.605215] mscc_felix 0000:00:00.5 swp2: New number of real TX queues: 4
[    7.612145] mscc_felix 0000:00:00.5 swp2: Hello!
[    7.616889] mscc_felix 0000:00:00.5 swp2: New number of real TX queues: 4
[    7.858868] mscc_felix 0000:00:00.5 swp3 (uninitialized): PHY [0000:00:00.3:13] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[    7.884240] mscc_felix 0000:00:00.5 swp3: Hello!
[    7.891086] mscc_felix 0000:00:00.5 swp3: New number of real TX queues: 4
[    7.898018] mscc_felix 0000:00:00.5 swp3: Hello!
[    7.902763] mscc_felix 0000:00:00.5 swp3: New number of real TX queues: 4

(I am not sure why the notifier is called twice though)

You are saying that here:

	num_tx_queues = slave_dev->real_num_tx_queues;

num_tx_queues remains assigned to 8? Does this mean that netif_set_real_num_tx_queues
has returned an error code? Can you check why?