Date:   Wed, 8 Dec 2021 00:45:25 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Ansuel Smith <ansuelsmth@...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 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.
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?

> 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));

Powered by blists - more mailing lists