Message-ID: <61afe8a9.1c69fb81.897ba.6022@mx.google.com>
Date:   Wed, 8 Dec 2021 00:05:11 +0100
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [net-next RFC PATCH 0/6] Add support for qca8k mdio rw in
 Ethernet packet

On Wed, Dec 08, 2021 at 12:45:25AM +0200, Vladimir Oltean wrote:
> On Tue, Dec 07, 2021 at 10:47:59PM +0100, Ansuel Smith wrote:
> > The main problem here is that we really need a way to have shared data
> > between tagger and dsa driver. I also think that it would be limiting
> > using this only for mdio. For example qca8k can autocast mib with
> > Ethernet port and that would be another feature that the tagger would
> > handle.
> 
> This is cool. I suppose this is what QCA_HDR_RECV_TYPE_MIB is for.

Exactly that.

> But it wouldn't work with your design because the tagger doesn't hold
> any queues, it is basically a request/response which is always initiated
> by the switch driver. The hardware can't automatically push anything to
> software on its own. Maybe if the tagger wouldn't be stateless, that
> would be better? What if the qca8k switch driver would just provide some
> function pointers to the switch driver (these would be protocol
> sub-handlers for QCA_HDR_RECV_TYPE_MIB, QCA_HDR_RECV_TYPE_RW_REG_ACK
> etc), and your completion structure would be both initialized, as well
> as finalized, all from within the switch driver itself?
> 

Hm. Interesting idea. So qca8k would provide the way to parse the packet
and made the request. The tagger would just detect the packet and
execute the dedicated function.
About mib considering the driver autocast counter for every port and
every packet have the relevant port to it (set in the qca tag), the
idea was to put a big array and directly write the data. The ethtool
function will then just read the data and report it. (or even work
directly on the ethtool data array).

> > I like the idea of tagger-owend per-switch-tree private data.
> > Do we really need to hook logic?
> > Wonder if something like that would work:
> > 1. Each tagger declare size of his private data (if any).
> > 2. Change tag dsa helper make sure the privata data in dst gets
> >    allocated and freed.
> > 3. We create some helper to get the tagger private data pointer that
> >    dsa driver will use. (an error is returned if no data is present)
> > 4. Tagger will use the dst to access his own data.
> 
> I considered a simplified form like this, but I think the tagger private
> data will still stay in dp->priv, only its ownership will change.
> It is less flexible to just have an autoalloc size. Ok, you allocate a
> structure the size you need, but which dp->priv gets to have it?
> Maybe a certain tagging protocol will need dp1->priv == dp2->priv ==
> dp3->priv == ..., whereas a different tagging protocol will need unique
> different structures for each dp.
> 
> > 
> > In theory that way we should be able to make a ""connection"" between
> > dsa driver and the tagger and prevent any sort of strange stuff that
> > could result in bug/kernel panic.
> > 
> > I mean for the current task (mdio in ethernet packet) we just need to
> > put data, send the skb and wait for a response (and after parsing) get
> > the data from a response skb.
> 
> It would be a huge win IMO if we could avoid managing the lifetime of
> dp->priv _directly_. I'm thinking something along the lines of:
> 
> - every time we make the "dst->tag_ops = tag_ops;" assignment (see dsa2.c)
>   there is a connection event between the switch tree and the tagging
>   protocol (and also a disconnection event, if dst->tag_ops wasn't
>   previously NULL).
> 
> - we could add a new tag_ops->connect(dst) and tag_ops->disconnect(dst)
>   and call them. These could allocate/free the dp->priv memory for each
>   dp in &dst->ports.
> 
> - _after_ the tag_ops->connect() has been called (this makes sure that
>   the tagger memory has been allocated) we could also emit a cross-chip
>   notifier event:
> 
> 	/* DSA_NOTIFIER_TAG_PROTO_CONNECT */
> 	struct dsa_notifier_tag_proto_connect_info {
> 		const struct dsa_device_ops *tag_ops;
> 	};
> 
> 	struct dsa_notifier_tag_proto_connect_info info;
> 
> 	dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO, &info);
> 
>   The role of a cross-chip notifier is to fan-out a call exactly once to
>   every switch within a tree. This particular cross-chip notifier could
>   end up with an implementation in switch.c that lands with a call to:
> 
>   ds->ops->tag_proto_connect(ds, tag_ops);
> 
>   At this point, I'm a bit fuzzy on the details. I'm thinking of
>   something like this:
> 
> 	struct qca8k_tagger_private {
> 		void (*rw_reg_ack_handler)(struct dsa_port *dp, void *buf);
> 		void (*mib_autocast_handler)(struct dsa_port *dp, void *buf);
> 	};
> 
> 	static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, void *buf)
> 	{
> 		... (code moved from tagger)
> 	}
> 
> 	static void qca8k_mib_autocast_handler(struct dsa_port *dp, void *buf)
> 	{
> 		... (code moved from tagger)
> 	}
> 
> 	static int qca8k_tag_proto_connect(struct dsa_switch *ds,
> 					   const struct dsa_device_ops *tag_ops)
> 	{
> 		switch (tag_ops->proto) {
> 		case DSA_TAG_PROTO_QCA:
> 			struct dsa_port *dp;
> 
> 			dsa_switch_for_each_port(dp, ds) {
> 				struct qca8k_tagger_private *priv = dp->priv;
> 
> 				priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> 				priv->mib_autocast_handler = qca8k_mib_autocast_handler;
> 			}
> 
> 			break;
> 		default:
> 			return -EOPNOTSUPP;
> 		}
> 	}
> 
> 	static const struct dsa_switch_ops qca8k_switch_ops = {
> 		...
> 		.tag_proto_connect	= qca8k_tag_proto_connect,
> 	};
> 
>   My current idea is maybe not ideal and a bit fuzzy, because the switch
>   driver would need to be aware of the fact that the tagger private data
>   is in dp->priv, and some code in one folder needs to be in sync with
>   some code in another folder. But at least it should be safer this way,
>   because we are in more control over the exact connection that's being
>   made.
> 
> - to avoid leaking memory, we also need to patch dsa_tree_put() to issue
>   a disconnect event on unbind.
> 
> - the tagging protocol driver would always need to NULL-check the
>   function pointer before dereferencing it, because it may connect to a
>   switch driver that doesn't set them up (dsa_loop):
> 
> 	struct qca8k_tagger_private *priv = dp->priv;
> 
> 	if (priv->rw_reg_ack_handler)
> 		priv->rw_reg_ack_handler(dp, skb_mac_header(skb));

Ok so your idea is to make the driver the one controlling ""everything""
and keep the tagger as dummy as possible. That would also remove all the
need to put stuff in the global include dir. Looks complex but handy. We
still need to understand the state part. Any hint about that?

In the mean time I will try implement this.

-- 
	Ansuel

Powered by blists - more mailing lists