Re: [RFC PATCH v7 11/16] net: dsa: qca8k: add support for mib autocast in Ethernet packet

Vladimir Oltean
2022-01-25T15:14:58Z

On Sun, Jan 23, 2022 at 02:33:32AM +0100, Ansuel Smith wrote:
> The switch can autocast MIB counter using Ethernet packet.
> Add support for this and provide a handler for the tagger.
> The switch will send packet with MIB counter for each port, the switch
> will use completion API to wait for the correct packet to be received
> and will complete the task only when each packet is received.
> Although the handler will drop all the other packet, we still have to
> consume each MIB packet to complete the request. This is done to prevent
> mixed data with concurrent ethtool request.
>
> connect_tag_protocol() is used to add the handler to the tag_qca tagger,
> master_state_change() use the MIB lock to make sure no MIB Ethernet is
> in progress.
>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
> drivers/net/dsa/qca8k.c | 108 +++++++++++++++++++++++++++++++++++++++-
> drivers/net/dsa/qca8k.h | 17 ++++++-
> 2 files changed, 122 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 35711d010eb4..f51a6d8993ff 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -811,7 +811,10 @@ qca8k_mib_init(struct qca8k_priv *priv)
> int ret;
>
> mutex_lock(&priv->reg_mutex);
> - ret = regmap_set_bits(priv->regmap, QCA8K_REG_MIB, QCA8K_MIB_FLUSH | QCA8K_MIB_BUSY);
> + ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
> + QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
> + FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_FLUSH) |
> + QCA8K_MIB_BUSY);
> if (ret)
> goto exit;
>
> @@ -1882,6 +1885,97 @@ qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data)
> ETH_GSTRING_LEN);
> }
>
> +static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *skb)
> +{
> + const struct qca8k_match_data *match_data;
> + struct qca8k_mib_hdr_data *mib_hdr_data;
> + struct qca8k_priv *priv = ds->priv;
> + const struct qca8k_mib_desc *mib;
> + struct mib_ethhdr *mib_ethhdr;
> + int i, mib_len, offset = 0;
> + u64 *data;
> + u8 port;
> +
> + mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
> + mib_hdr_data = &priv->mib_hdr_data;
> +
> + /* The switch autocast every port. Ignore other packet and
> + * parse only the requested one.
> + */
> + port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
> + if (port != mib_hdr_data->req_port)
> + goto exit;
> +
> + match_data = device_get_match_data(priv->dev);
> + data = mib_hdr_data->data;
> +
> + for (i = 0; i < match_data->mib_count; i++) {
> + mib = &ar8327_mib[i];
> +
> + /* First 3 mib are present in the skb head */
> + if (i < 3) {
> + data[i] = mib_ethhdr->data[i];
> + continue;
> + }
> +
> + mib_len = sizeof(uint32_t);
> +
> + /* Some mib are 64 bit wide */
> + if (mib->size == 2)
> + mib_len = sizeof(uint64_t);
> +
> + /* Copy the mib value from packet to the */
> + memcpy(data + i, skb->data + offset, mib_len);
> +
> + /* Set the offset for the next mib */
> + offset += mib_len;
> + }
> +
> +exit:
> + /* Complete on receiving all the mib packet */
> + if (refcount_dec_and_test(&mib_hdr_data->port_parsed))
> + complete(&mib_hdr_data->rw_done);
> +}
> +
> +static int
> +qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
> +{
> + struct dsa_port *dp = dsa_to_port(ds, port);
> + struct qca8k_mib_hdr_data *mib_hdr_data;
> + struct qca8k_priv *priv = ds->priv;
> + int ret;
> +
> + mib_hdr_data = &priv->mib_hdr_data;
> +
> + mutex_lock(&mib_hdr_data->mutex);
> +
> + reinit_completion(&mib_hdr_data->rw_done);
> +
> + mib_hdr_data->req_port = dp->index;
> + mib_hdr_data->data = data;
> + refcount_set(&mib_hdr_data->port_parsed, QCA8K_NUM_PORTS);
> +
> + mutex_lock(&priv->reg_mutex);
> +
> + /* Send mib autocast request */
> + ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
> + QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
> + FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_CAST) |
> + QCA8K_MIB_BUSY);
> +
> + mutex_unlock(&priv->reg_mutex);
> +
> + if (ret)
> + goto exit;
> +
> + ret = wait_for_completion_timeout(&mib_hdr_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
> +
> +exit:
> + mutex_unlock(&mib_hdr_data->mutex);
> +
> + return ret;
> +}
> +
> static void
> qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
> uint64_t *data)
> @@ -1893,6 +1987,10 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
> u32 hi = 0;
> int ret;
>
> + if (priv->mgmt_master &&
> + qca8k_get_ethtool_stats_eth(ds, port, data) > 0)
> + return;
> +
> match_data = of_device_get_match_data(priv->dev);
>
> for (i = 0; i < match_data->mib_count; i++) {
> @@ -2573,7 +2671,8 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> if (dp->index != 0)
> return;
>
> - mutex_lock(&priv->mgmt_hdr_data.mutex);
> + mutex_unlock(&priv->mgmt_hdr_data.mutex);

Why do you unlock mgmt_hdr_data here?

> + mutex_lock(&priv->mib_hdr_data.mutex);
>
> if (operational)
> priv->mgmt_master = master;
> @@ -2581,6 +2680,7 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> priv->mgmt_master = NULL;
>
> mutex_unlock(&priv->mgmt_hdr_data.mutex);
> + mutex_unlock(&priv->mib_hdr_data.mutex);

Please unlock in the reverse order of locking.

> }
>
> static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> @@ -2593,6 +2693,7 @@ static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> tagger_data = ds->tagger_data;
>
> tagger_data->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> + tagger_data->mib_autocast_handler = qca8k_mib_autocast_handler;
>
> break;
> default:
> @@ -2721,6 +2822,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> mutex_init(&priv->mgmt_hdr_data.mutex);
> init_completion(&priv->mgmt_hdr_data.rw_done);
>
> + mutex_init(&priv->mib_hdr_data.mutex);
> + init_completion(&priv->mib_hdr_data.rw_done);
> +
> priv->ds->dev = &mdiodev->dev;
> priv->ds->num_ports = QCA8K_NUM_PORTS;
> priv->ds->priv = priv;
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index a358a67044d3..dc1365542aa3 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -67,7 +67,7 @@
> #define QCA8K_REG_MODULE_EN 0x030
> #define QCA8K_MODULE_EN_MIB BIT(0)
> #define QCA8K_REG_MIB 0x034
> -#define QCA8K_MIB_FLUSH BIT(24)
> +#define QCA8K_MIB_FUNC GENMASK(26, 24)
> #define QCA8K_MIB_CPU_KEEP BIT(20)
> #define QCA8K_MIB_BUSY BIT(17)
> #define QCA8K_MDIO_MASTER_CTRL 0x3c
> @@ -317,6 +317,12 @@ enum qca8k_vlan_cmd {
> QCA8K_VLAN_READ = 6,
> };
>
> +enum qca8k_mid_cmd {
> + QCA8K_MIB_FLUSH = 1,
> + QCA8K_MIB_FLUSH_PORT = 2,
> + QCA8K_MIB_CAST = 3,
> +};
> +
> struct ar8xxx_port_status {
> int enabled;
> };
> @@ -340,6 +346,14 @@ struct qca8k_mgmt_hdr_data {
> u32 data[4];
> };
>
> +struct qca8k_mib_hdr_data {
> + struct completion rw_done;
> + struct mutex mutex; /* Process one command at time */
> + refcount_t port_parsed; /* Counter to track parsed port */
> + u8 req_port;
> + u64 *data; /* pointer to ethtool data */
> +};
> +
> struct qca8k_ports_config {
> bool sgmii_rx_clk_falling_edge;
> bool sgmii_tx_clk_falling_edge;
> @@ -367,6 +381,7 @@ struct qca8k_priv {
> unsigned int port_mtu[QCA8K_NUM_PORTS];
> const struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
> struct qca8k_mgmt_hdr_data mgmt_hdr_data;
> + struct qca8k_mib_hdr_data mib_hdr_data;
> };
>
> struct qca8k_mib_desc {
> --
> 2.33.1
>