Message-ID: <61b0275d.1c69fb81.efd64.83fb@mx.google.com>
Date:   Wed, 8 Dec 2021 04:32:43 +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 03:09:47AM +0200, Vladimir Oltean wrote:
> On Wed, Dec 08, 2021 at 01:42:59AM +0100, Ansuel Smith wrote:
> > On Wed, Dec 08, 2021 at 02:40:51AM +0200, Vladimir Oltean wrote:
> > > On Wed, Dec 08, 2021 at 02:04:32AM +0200, Vladimir Oltean wrote:
> > > > On Wed, Dec 08, 2021 at 01:47:36AM +0200, Vladimir Oltean wrote:
> > > > > > 2) is harder. But as far as i know, we have an 1:N setup.  One switch
> > > > > > driver can use N tag drivers. So we need the switch driver to be sure
> > > > > > the tag driver is what it expects. We keep the shared state in the tag
> > > > > > driver, so it always has valid data, but when the switch driver wants
> > > > > > to get a pointer to it, it needs to pass a enum dsa_tag_protocol and
> > > > > > if it does not match, the core should return -EINVAL or similar.
> > > > > 
> > > > > In my proposal, the tagger will allocate the memory from its side of the
> > > > > ->connect() call. So regardless of whether the switch driver side
> > > > > connects or not, the memory inside dp->priv is there for the tagger to
> > > > > use. The switch can access it or it can ignore it.
> > > > 
> > > > I don't think I actually said something useful here.
> > > > 
> > > > The goal would be to minimize use of dp->priv inside the switch driver,
> > > > outside of the actual ->connect() / ->disconnect() calls.
> > > > For example, in the felix driver which supports two tagging protocol
> > > > drivers, I think these two methods would be enough, and they would
> > > > replace the current felix_port_setup_tagger_data() and
> > > > felix_port_teardown_tagger_data() calls.
> > > > 
> > > > An additional benefit would be that in ->connect() and ->disconnect() we
> > > > get the actual tagging protocol in use. Currently the felix driver lacks
> > > > there, because felix_port_setup_tagger_data() just sets dp->priv up
> > > > unconditionally for the ocelot-8021q tagging protocol (luckily the
> > > > normal ocelot tagger doesn't need dp->priv).
> > > > 
> > > > In sja1105 the story is a bit longer, but I believe that can also be
> > > > cleaned up to stay within the confines of ->connect()/->disconnect().
> > > > 
> > > > So I guess we just need to be careful and push back against dubious use
> > > > during review.
> > > 
> > > I've started working on a prototype for converting sja1105 to this model.
> > > It should be clearer to me by tomorrow whether there is anything missing
> > > from this proposal.
> > 
> > I'm working on your suggestion and I should be able to post another RFC
> > this night if all works correctly with my switch.
> 
> Ok. The key point with my progress so far is that Andrew may be right
> and we might need separate tagger priv pointers per port and per switch.
> At least for the cleanliness of implementation. In the end I plan to
> remove dp->priv and stay with dp->tagger_priv and ds->tagger_priv.
> 
> Here's what I have so far. I have more changes locally, but the rest it
> isn't ready and overall also a bit irrelevant for the discussion.
> I'm going to sleep now.
>

BTW, I notice we made the same mistake. Don't know if it was the problem
and you didn't notice... The notifier is not ready at times of the first
tagger setup and the tag_proto_connect is never called.
Anyway sending v2 with your suggestion applied.

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index bdf308a5c55e..f0f702774c8d 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -82,12 +82,15 @@ enum dsa_tag_protocol {
>  };
>  
>  struct dsa_switch;
> +struct dsa_switch_tree;
>  
>  struct dsa_device_ops {
>  	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
>  	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev);
>  	void (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
>  			     int *offset);
> +	int (*connect)(struct dsa_switch_tree *dst);
> +	void (*disconnect)(struct dsa_switch_tree *dst);
>  	unsigned int needed_headroom;
>  	unsigned int needed_tailroom;
>  	const char *name;
> @@ -279,6 +282,8 @@ struct dsa_port {
>  	 */
>  	void *priv;
>  
> +	void *tagger_priv;
> +
>  	/*
>  	 * Original copy of the master netdev ethtool_ops
>  	 */
> @@ -337,6 +342,8 @@ struct dsa_switch {
>  	 */
>  	void *priv;
>  
> +	void *tagger_priv;
> +
>  	/*
>  	 * Configuration data for this switch.
>  	 */
> @@ -689,6 +696,8 @@ struct dsa_switch_ops {
>  						  enum dsa_tag_protocol mprot);
>  	int	(*change_tag_protocol)(struct dsa_switch *ds, int port,
>  				       enum dsa_tag_protocol proto);
> +	int	(*connect_tag_protocol)(struct dsa_switch *ds,
> +					enum dsa_tag_protocol proto);
>  
>  	/* Optional switch-wide initialization and destruction methods */
>  	int	(*setup)(struct dsa_switch *ds);
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 8814fa0e44c8..3787cbce1175 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -248,8 +248,12 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index)
>  
>  static void dsa_tree_free(struct dsa_switch_tree *dst)
>  {
> -	if (dst->tag_ops)
> +	if (dst->tag_ops) {
> +		if (dst->tag_ops->disconnect)
> +			dst->tag_ops->disconnect(dst);
> +
>  		dsa_tag_driver_put(dst->tag_ops);
> +	}
>  	list_del(&dst->list);
>  	kfree(dst);
>  }
> @@ -1136,6 +1140,36 @@ static void dsa_tree_teardown(struct dsa_switch_tree *dst)
>  	dst->setup = false;
>  }
>  
> +static int dsa_tree_bind_tag_proto(struct dsa_switch_tree *dst,
> +				   const struct dsa_device_ops *tag_ops)
> +{
> +	struct dsa_notifier_tag_proto_info info;
> +	int err;
> +
> +	if (dst->tag_ops && dst->tag_ops->disconnect)
> +		dst->tag_ops->disconnect(dst);
> +
> +	if (tag_ops->connect) {
> +		err = tag_ops->connect(dst);
> +		if (err)
> +			return err;
> +	}
> +
> +	info.tag_ops = tag_ops;
> +	err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_CONNECT, &info);
> +	if (err && err != -EOPNOTSUPP)
> +		goto out_disconnect;
> +
> +	dst->tag_ops = tag_ops;
> +
> +	return 0;
> +
> +out_disconnect:
> +	if (tag_ops->disconnect)
> +		tag_ops->disconnect(dst);
> +	return err;
> +}
> +
>  /* Since the dsa/tagging sysfs device attribute is per master, the assumption
>   * is that all DSA switches within a tree share the same tagger, otherwise
>   * they would have formed disjoint trees (different "dsa,member" values).
> @@ -1173,7 +1207,9 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
>  	if (err)
>  		goto out_unwind_tagger;
>  
> -	dst->tag_ops = tag_ops;
> +	err = dsa_tree_bind_tag_proto(dst, tag_ops);
> +	if (err)
> +		goto out_unwind_tagger;
>  
>  	rtnl_unlock();
>  
> @@ -1307,7 +1343,9 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
>  		 */
>  		dsa_tag_driver_put(tag_ops);
>  	} else {
> -		dst->tag_ops = tag_ops;
> +		err = dsa_tree_bind_tag_proto(dst, tag_ops);
> +		if (err)
> +			return err;
>  	}
>  
>  	dp->master = master;
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 38ce5129a33d..0db2b26b0c83 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -37,6 +37,7 @@ enum {
>  	DSA_NOTIFIER_VLAN_DEL,
>  	DSA_NOTIFIER_MTU,
>  	DSA_NOTIFIER_TAG_PROTO,
> +	DSA_NOTIFIER_TAG_PROTO_CONNECT,
>  	DSA_NOTIFIER_MRP_ADD,
>  	DSA_NOTIFIER_MRP_DEL,
>  	DSA_NOTIFIER_MRP_ADD_RING_ROLE,
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 9c92edd96961..06948f536829 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -647,6 +647,17 @@ static int dsa_switch_change_tag_proto(struct dsa_switch *ds,
>  	return 0;
>  }
>  
> +static int dsa_switch_connect_tag_proto(struct dsa_switch *ds,
> +					struct dsa_notifier_tag_proto_info *info)
> +{
> +	const struct dsa_device_ops *tag_ops = info->tag_ops;
> +
> +	if (!ds->ops->connect_tag_protocol)
> +		return -EOPNOTSUPP;
> +
> +	return ds->ops->connect_tag_protocol(ds, tag_ops->proto);
> +}
> +
>  static int dsa_switch_mrp_add(struct dsa_switch *ds,
>  			      struct dsa_notifier_mrp_info *info)
>  {
> @@ -766,6 +777,9 @@ static int dsa_switch_event(struct notifier_block *nb,
>  	case DSA_NOTIFIER_TAG_PROTO:
>  		err = dsa_switch_change_tag_proto(ds, info);
>  		break;
> +	case DSA_NOTIFIER_TAG_PROTO_CONNECT:
> +		err = dsa_switch_connect_tag_proto(ds, info);
> +		break;
>  	case DSA_NOTIFIER_MRP_ADD:
>  		err = dsa_switch_mrp_add(ds, info);
>  		break;
> diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
> index 6c293c2a3008..53362a0f0aab 100644
> --- a/net/dsa/tag_sja1105.c
> +++ b/net/dsa/tag_sja1105.c
> @@ -722,11 +722,59 @@ static void sja1110_flow_dissect(const struct sk_buff *skb, __be16 *proto,
>  	*proto = ((__be16 *)skb->data)[(VLAN_HLEN / 2) - 1];
>  }
>  
> +static void sja1105_disconnect(struct dsa_switch_tree *dst)
> +{
> +	struct dsa_port *dp;
> +
> +	dsa_tree_for_each_user_port(dp, dst) {
> +		if (dp->tagger_priv) {
> +			kfree(dp->tagger_priv);
> +			dp->tagger_priv = NULL;
> +		}
> +
> +		if (dp->ds->tagger_priv) {
> +			kfree(dp->ds->tagger_priv);
> +			dp->ds->tagger_priv = NULL;
> +		}
> +	}
> +}
> +
> +static int sja1105_connect(struct dsa_switch_tree *dst)
> +{
> +	struct sja1105_tagger_data *data;
> +	struct sja1105_port *sp;
> +	struct dsa_port *dp;
> +
> +	dsa_tree_for_each_user_port(dp, dst) {
> +		if (!dp->ds->tagger_priv) {
> +			data = kzalloc(sizeof(*data), GFP_KERNEL);
> +			if (!data)
> +				goto out;
> +
> +			dp->ds->tagger_priv = data;
> +		}
> +
> +		sp = kzalloc(sizeof(*sp), GFP_KERNEL);
> +		if (!sp)
> +			goto out;
> +
> +		dp->tagger_priv = sp;
> +	}
> +
> +	return 0;
> +
> +out:
> +	sja1105_disconnect(dst);
> +	return -ENOMEM;
> +}
> +
>  static const struct dsa_device_ops sja1105_netdev_ops = {
>  	.name = "sja1105",
>  	.proto = DSA_TAG_PROTO_SJA1105,
>  	.xmit = sja1105_xmit,
>  	.rcv = sja1105_rcv,
> +	.connect = sja1105_connect,
> +	.disconnect = sja1105_disconnect,
>  	.needed_headroom = VLAN_HLEN,
>  	.flow_dissect = sja1105_flow_dissect,
>  	.promisc_on_master = true,
> @@ -740,6 +788,8 @@ static const struct dsa_device_ops sja1110_netdev_ops = {
>  	.proto = DSA_TAG_PROTO_SJA1110,
>  	.xmit = sja1110_xmit,
>  	.rcv = sja1110_rcv,
> +	.connect = sja1105_connect,
> +	.disconnect = sja1105_disconnect,
>  	.flow_dissect = sja1110_flow_dissect,
>  	.needed_headroom = SJA1110_HEADER_LEN + VLAN_HLEN,
>  	.needed_tailroom = SJA1110_RX_TRAILER_LEN + SJA1110_MAX_PADDING_LEN,
> -- 
> 2.25.1

-- 
	Ansuel

Powered by blists - more mailing lists