From: Florian Fainelli <f.fainelli@gmail.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Andrew Lunn <andrew@lunn.ch>,
Vivien Didelot <vivien.didelot@gmail.com>,
"bcm-kernel-feedback-list@broadcom.com"
<bcm-kernel-feedback-list@broadcom.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [RFC PATCH net-next 3/4] net: systemport: use standard netdevice notifier to detect DSA presence
Date: Sun, 20 Dec 2020 20:53:58 -0800 [thread overview]
Message-ID: <f2f420d3-baa0-e999-d23a-3e817e706cc7@gmail.com> (raw)
In-Reply-To: <20201219121237.tq3pxquyaq4q547t@skbuf>
On 12/19/2020 4:12 AM, Vladimir Oltean wrote:
> 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@nxp.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.
Yes that part is fine, see below.
>
> 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)
This is the actual issue that messes up the queue assignment for
bcmsysport because we assign queue/switch port pairs and don't really
expect to be re-doing that once ring->inspect is set to true.
>
> 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?
>
The call to netif_set_real_num_tx_queues() succeeds and
slave_dev->real_num_tx_queues is changed to 4 accordingly. The loop that
assigns the internal queue mapping (priv->ring_map) is correctly limited
to 4, however we get two calls per switch port instead of one. I did not
have much time to debug why we get called twice but I will be looking
into this tomorrow.
--
Florian
next prev parent reply other threads:[~2020-12-21 4:54 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-18 22:38 [RFC PATCH net-next 0/4] Reduce coupling between DSA and Broadcom SYSTEMPORT driver Vladimir Oltean
2020-12-18 22:38 ` [RFC PATCH net-next 1/4] net: dsa: move the Broadcom tag information in a separate header file Vladimir Oltean
2020-12-19 0:20 ` Florian Fainelli
2020-12-18 22:38 ` [RFC PATCH net-next 2/4] net: dsa: export dsa_slave_dev_check Vladimir Oltean
2020-12-19 0:20 ` Florian Fainelli
2020-12-18 22:38 ` [RFC PATCH net-next 3/4] net: systemport: use standard netdevice notifier to detect DSA presence Vladimir Oltean
2020-12-19 0:26 ` Florian Fainelli
2020-12-19 4:08 ` Florian Fainelli
2020-12-19 12:12 ` Vladimir Oltean
2020-12-21 4:53 ` Florian Fainelli [this message]
2020-12-21 22:33 ` Florian Fainelli
2020-12-21 23:06 ` Vladimir Oltean
2020-12-21 23:17 ` Florian Fainelli
2020-12-21 23:41 ` Vladimir Oltean
2020-12-18 22:38 ` [RFC PATCH net-next 4/4] net: dsa: remove the DSA specific notifiers Vladimir Oltean
2020-12-19 0:24 ` Florian Fainelli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f2f420d3-baa0-e999-d23a-3e817e706cc7@gmail.com \
--to=f.fainelli@gmail.com \
--cc=andrew@lunn.ch \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=vivien.didelot@gmail.com \
--cc=vladimir.oltean@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).