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: <b106399b-e341-2aa2-d92e-24f5a0c243c9@gmail.com>
Date:   Mon, 21 Dec 2020 15:17:02 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>,
        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 12/21/2020 3:06 PM, Vladimir Oltean wrote:
> On Mon, Dec 21, 2020 at 02:33:16PM -0800, Florian Fainelli wrote:
>> On 12/20/2020 8:53 PM, Florian Fainelli wrote:
>>> 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.
>>
>> There was not any bug other than there are two instances of a SYSTEMPORT
>> device in my system and they both receive the same notification.
>>
>> So we do need to qualify which of the notifier block matches the device
>> of interest, because if we do extract the private structure from the
>> device being notified, it is always going to match.
>>
>> Incremental fixup here:
>>
>> https://github.com/ffainelli/linux/commit/0eea16e706a73c56a36d701df483ff73211aae7f
> 
> ...duh.
> And when you come to think that I had deleted that code in my patch, not
> understanding what it's for... Coincidentally this is also the reason
> why I got the prints twice. Sorry :(

No worries, I had it "automatically" in my experiment with the
REGISTER/UNREGISTER and it only clicked this morning this was the key
thing here.

> 
>>
>> and you can add Tested-by: Florian Fainelli <f.fainelli@...il.com> when
>> you resubmit.
>>
>> Thanks, this is a really nice cleanup.
> 
> Thanks.
> 
> Do you think we need some getters for dp->index and dp->ds->index, to preserve
> some sort of data structure encapsulation from the outside world (although it's
> not as if the members of struct dsa_switch and struct dsa_port still couldn't
> be accessed directly)?
> 
> But then, there's the other aspect. We would have some shiny accessors for DSA
> properties, but we're resetting the net_device's number of TX queues.
> So much for data encapsulation.

If we move the dsa_port structure definition to be more private, and say
within dsa_priv.h, we will have to create quite some bit of churn within
the DSA driver to make them use getters and setters. Russell did a nice
job with the encapsulation with phylink and that would really be a good
model to follow, however this was a clean slate. It seems to me for now
that this is not worth the trouble.

Despite accessing the TX queues directly, the original DSA notifier was
trying to provide all the necessary data to the recipient of the
notification without having to know too much about what a DSA device is
but the amount of code eliminated is of superior value IMHO.
-- 
Florian