Re: [net-next PATCH RFC v6 10/16] net: dsa: tag_qca: add support for handling mdio Ethernet and MIB packet

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, Dec 14, 2021 at 11:44:03PM +0100, Ansuel Smith wrote:
> Add connect/disconnect helper to assign private struct to the cpu port
> dsa priv.
> Add support for Ethernet mdio packet and MIB packet if the dsa driver
> provide an handler to correctly parse and elaborate the data.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  include/linux/dsa/tag_qca.h |  7 +++++++
>  net/dsa/tag_qca.c           | 35 +++++++++++++++++++++++++++++++++--
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
> index cd6275bac103..203e72dad9bb 100644
> --- a/include/linux/dsa/tag_qca.h
> +++ b/include/linux/dsa/tag_qca.h
> @@ -69,4 +69,11 @@ struct mib_ethhdr {
>  	__be16 hdr;		/* qca hdr */
>  } __packed;
>  
> +struct tag_qca_priv {

Could you keep the "priv" name for later, in case you need to hide some
tagger storage from the switch driver? This could be renamed to
"qca_tagger_data".

> +	void (*rw_reg_ack_handler)(struct dsa_port *dp,
> +				   struct sk_buff *skb);
> +	void (*mib_autocast_handler)(struct dsa_port *dp,
> +				     struct sk_buff *skb);
> +};
> +
>  #endif /* __TAG_QCA_H */
> diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> index f5547d357647..59e04157f53b 100644
> --- a/net/dsa/tag_qca.c
> +++ b/net/dsa/tag_qca.c
> @@ -32,11 +32,15 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
>  {
> +	struct dsa_port *dp = dev->dsa_ptr;

cpu_dp, for clarity

> +	struct tag_qca_priv *priv;
>  	u16  hdr, pk_type;
>  	__be16 *phdr;
>  	int port;
>  	u8 ver;
>  
> +	priv = dp->ds->tagger_data;
> +
>  	if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
>  		return NULL;
>  
> @@ -52,12 +56,18 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
>  	pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
>  
>  	/* Ethernet MDIO read/write packet */
> -	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
> +	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) {
> +		if (priv->rw_reg_ack_handler)
> +			priv->rw_reg_ack_handler(dp, skb);

Minor nitpick, but why not pass the "ds" as argument?

>  		return NULL;
> +	}
>  
>  	/* Ethernet MIB counter packet */
> -	if (pk_type == QCA_HDR_RECV_TYPE_MIB)
> +	if (pk_type == QCA_HDR_RECV_TYPE_MIB) {
> +		if (priv->mib_autocast_handler)
> +			priv->mib_autocast_handler(dp, skb);
>  		return NULL;
> +	}
>  
>  	/* Remove QCA tag and recalculate checksum */
>  	skb_pull_rcsum(skb, QCA_HDR_LEN);
> @@ -73,9 +83,30 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
>  	return skb;
>  }
>  
> +static int qca_tag_connect(struct dsa_switch *ds)
> +{
> +	struct tag_qca_priv *priv;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ds->tagger_data = priv;
> +
> +	return 0;
> +}
> +
> +static void qca_tag_disconnect(struct dsa_switch *ds)
> +{
> +	kfree(ds->tagger_data);
> +	ds->tagger_data = NULL;
> +}
> +
>  static const struct dsa_device_ops qca_netdev_ops = {
>  	.name	= "qca",
>  	.proto	= DSA_TAG_PROTO_QCA,
> +	.connect = qca_tag_connect,
> +	.disconnect = qca_tag_disconnect,
>  	.xmit	= qca_tag_xmit,
>  	.rcv	= qca_tag_rcv,
>  	.needed_headroom = QCA_HDR_LEN,
> -- 
> 2.33.1
>