Re: [PATCH net] net: dsa: Fix tagging attribute location

[Florian Fainelli <f.fainelli@gmail.com>]

On 11/28/18 1:40 PM, Florian Fainelli wrote:
> While introducing the DSA tagging protocol attribute, it was added to the DSA
> slave network devices, but those actually see untagged traffic (that is their
> whole purpose). Correct this mistake by putting the tagging sysfs attribute
> under the DSA master network device where this is the information that we need.
> 
> While at it, also correct the sysfs documentation mistake that missed the
> "dsa/" directory component of the attribute.
> 
> Fixes: 98cdb4807123 ("net: dsa: Expose tagging protocol to user-space")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

The changeset in the Fixes tag is in v4.20-rc and has not made it into
any final kernel yet in case you are wondering.

>  Documentation/ABI/testing/sysfs-class-net-dsa |  2 +-
>  net/dsa/master.c                              | 34 ++++++++++++++++++-
>  net/dsa/slave.c                               | 28 ---------------
>  3 files changed, 34 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-net-dsa b/Documentation/ABI/testing/sysfs-class-net-dsa
> index f240221e071e..985d84c585c6 100644
> --- a/Documentation/ABI/testing/sysfs-class-net-dsa
> +++ b/Documentation/ABI/testing/sysfs-class-net-dsa
> @@ -1,4 +1,4 @@
> -What:		/sys/class/net/<iface>/tagging
> +What:		/sys/class/net/<iface>/dsa/tagging
>  Date:		August 2018
>  KernelVersion:	4.20
>  Contact:	netdev@vger.kernel.org
> diff --git a/net/dsa/master.c b/net/dsa/master.c
> index c90ee3227dea..5e8c9bef78bd 100644
> --- a/net/dsa/master.c
> +++ b/net/dsa/master.c
> @@ -158,8 +158,31 @@ static void dsa_master_ethtool_teardown(struct net_device *dev)
>  	cpu_dp->orig_ethtool_ops = NULL;
>  }
>  
> +static ssize_t tagging_show(struct device *d, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct net_device *dev = to_net_dev(d);
> +	struct dsa_port *cpu_dp = dev->dsa_ptr;
> +
> +	return sprintf(buf, "%s\n",
> +		       dsa_tag_protocol_to_str(cpu_dp->tag_ops));
> +}
> +static DEVICE_ATTR_RO(tagging);
> +
> +static struct attribute *dsa_slave_attrs[] = {
> +	&dev_attr_tagging.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group dsa_group = {
> +	.name	= "dsa",
> +	.attrs	= dsa_slave_attrs,
> +};
> +
>  int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
>  {
> +	int ret;
> +
>  	/* If we use a tagging format that doesn't have an ethertype
>  	 * field, make sure that all packets from this point on get
>  	 * sent to the tag format's receive function.
> @@ -168,11 +191,20 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
>  
>  	dev->dsa_ptr = cpu_dp;
>  
> -	return dsa_master_ethtool_setup(dev);
> +	ret = dsa_master_ethtool_setup(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = sysfs_create_group(&dev->dev.kobj, &dsa_group);
> +	if (ret)
> +		dsa_master_ethtool_teardown(dev);
> +
> +	return ret;
>  }
>  
>  void dsa_master_teardown(struct net_device *dev)
>  {
> +	sysfs_remove_group(&dev->dev.kobj, &dsa_group);
>  	dsa_master_ethtool_teardown(dev);
>  
>  	dev->dsa_ptr = NULL;
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 7d0c19e7edcf..aec78f5aca72 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1058,27 +1058,6 @@ static struct device_type dsa_type = {
>  	.name	= "dsa",
>  };
>  
> -static ssize_t tagging_show(struct device *d, struct device_attribute *attr,
> -			    char *buf)
> -{
> -	struct net_device *dev = to_net_dev(d);
> -	struct dsa_port *dp = dsa_slave_to_port(dev);
> -
> -	return sprintf(buf, "%s\n",
> -		       dsa_tag_protocol_to_str(dp->cpu_dp->tag_ops));
> -}
> -static DEVICE_ATTR_RO(tagging);
> -
> -static struct attribute *dsa_slave_attrs[] = {
> -	&dev_attr_tagging.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group dsa_group = {
> -	.name	= "dsa",
> -	.attrs	= dsa_slave_attrs,
> -};
> -
>  static void dsa_slave_phylink_validate(struct net_device *dev,
>  				       unsigned long *supported,
>  				       struct phylink_link_state *state)
> @@ -1374,14 +1353,8 @@ int dsa_slave_create(struct dsa_port *port)
>  		goto out_phy;
>  	}
>  
> -	ret = sysfs_create_group(&slave_dev->dev.kobj, &dsa_group);
> -	if (ret)
> -		goto out_unreg;
> -
>  	return 0;
>  
> -out_unreg:
> -	unregister_netdev(slave_dev);
>  out_phy:
>  	rtnl_lock();
>  	phylink_disconnect_phy(p->dp->pl);
> @@ -1405,7 +1378,6 @@ void dsa_slave_destroy(struct net_device *slave_dev)
>  	rtnl_unlock();
>  
>  	dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
> -	sysfs_remove_group(&slave_dev->dev.kobj, &dsa_group);
>  	unregister_netdev(slave_dev);
>  	phylink_destroy(dp->pl);
>  	free_percpu(p->stats64);
> 

-- 
Florian