Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <4a4f8880-9ea9-1aca-a202-18d5c50abd82@gmail.com>
Date:   Tue, 25 Jan 2022 19:34:52 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [RFC PATCH v7 08/16] net: dsa: tag_qca: add support for handling
 mgmt and MIB Ethernet packet
Content-Language: en-US
To:     Ansuel Smith <ansuelsmth@gmail.com>, Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
References: <20220123013337.20945-1-ansuelsmth@gmail.com>
 <20220123013337.20945-9-ansuelsmth@gmail.com>
From:   Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <20220123013337.20945-9-ansuelsmth@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> Add connect/disconnect helper to assign private struct to the dsa switch.
> Add support for Ethernet mgm and MIB if the dsa driver provide an handler
> to correctly parse and elaborate the data.

s/mgm/mgmt/
s/dsa/DSA/


> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>   include/linux/dsa/tag_qca.h |  7 +++++++
>   net/dsa/tag_qca.c           | 39 ++++++++++++++++++++++++++++++++++---
>   2 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
> index 87dd84e31304..de5a45f5b398 100644
> --- a/include/linux/dsa/tag_qca.h
> +++ b/include/linux/dsa/tag_qca.h
> @@ -72,4 +72,11 @@ struct mib_ethhdr {
>   	__be16 hdr;		/* qca hdr */
>   } __packed;
>   
> +struct qca_tagger_data {
> +	void (*rw_reg_ack_handler)(struct dsa_switch *ds,
> +				   struct sk_buff *skb);
> +	void (*mib_autocast_handler)(struct dsa_switch *ds,
> +				     struct sk_buff *skb);
> +};
> +
>   #endif /* __TAG_QCA_H */
> diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> index fdaa1b322d25..dc81c72133eb 100644
> --- a/net/dsa/tag_qca.c
> +++ b/net/dsa/tag_qca.c
> @@ -5,6 +5,7 @@
>   
>   #include <linux/etherdevice.h>
>   #include <linux/bitfield.h>
> +#include <net/dsa.h>
>   #include <linux/dsa/tag_qca.h>
>   
>   #include "dsa_priv.h"
> @@ -32,11 +33,16 @@ 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 qca_tagger_data *tagger_data;
> +	struct dsa_port *dp = dev->dsa_ptr;
> +	struct dsa_switch *ds = dp->ds;
>   	u16 hdr, pk_type;
>   	__be16 *phdr;
>   	int port;
>   	u8 ver;
>   
> +	tagger_data = ds->tagger_data;
> +
>   	if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
>   		return NULL;
>   
> @@ -51,13 +57,19 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
>   	/* Get pk type */
>   	pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
>   
> -	/* Ethernet MDIO read/write packet */
> -	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
> +	/* Ethernet mgmt read/write packet */
> +	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) {
> +		if (tagger_data->rw_reg_ack_handler)

if (likely()) in case that happens to make a difference?

> +			tagger_data->rw_reg_ack_handler(ds, skb);
>   		return NULL;
> +	}
>   
>   	/* Ethernet MIB counter packet */
> -	if (pk_type == QCA_HDR_RECV_TYPE_MIB)
> +	if (pk_type == QCA_HDR_RECV_TYPE_MIB) {
> +		if (tagger_data->mib_autocast_handler)

Likewise

In any case, this looks good to me:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian