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

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Florian Fainelli <f.fainelli@gmail.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: Mon, 21 Dec 2020 23:41:18 +0000	[thread overview]
Message-ID: <20201221234117.m2jsyjz576hd47i5@skbuf> (raw)
In-Reply-To: <b106399b-e341-2aa2-d92e-24f5a0c243c9@gmail.com>

On Mon, Dec 21, 2020 at 03:17:02PM -0800, Florian Fainelli wrote:
> > 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.

We could make include/net/dsa.h a semi-private ("friend") header that
the DSA drivers could keep including, but the non-DSA world wouldn't.
Then we could create a new one in its place, with just the stuff that
the outside world needs: netdev_uses_dsa, etc.

> 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

I know. Personally, accessing dp->cpu_dp->master directly is where I was
going to draw the line and say that it's better to just keep the code as
is. What motivated me to make the change in the first place was the
realization that we now have the linkage visible from the outside world.

> but the amount of code eliminated is of superior value IMHO.

It's still a compromise, really. The atomic DSA notifier was ok, but if
we could do without it, why not...