Re: [PATCH net-next v4 5/7] net: stmmac: support fp parameter of tc-mqprio

Alexander Lobakin
2024-08-20T15:20:31Z

From: Furong Xu <0x1207@gmail.com>
Date: Tue, 20 Aug 2024 17:38:33 +0800

> tc-mqprio can select whether traffic classes are express or preemptible.
>
> After some traffic tests, MAC merge layer statistics are all good.

[...]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> index 4c91fa766b13..1e87dbc9a406 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> @@ -670,3 +670,55 @@ void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size)
>
> writel(value, ioaddr + MTL_FPE_CTRL_STS);
> }
> +
> +int dwmac5_fpe_set_preemptible_tcs(struct net_device *ndev,
> + struct netlink_ext_ack *extack,
> + unsigned long tcs)
> +{
> + struct stmmac_priv *priv = netdev_priv(ndev);

Can't it be const?

> + void __iomem *ioaddr = priv->ioaddr;
> + unsigned long queue_tcs = 0;

Why is @tcs and @queue_tcs unsigned long? You write @queue_tcs via
writel(), IOW it can't go past %U32_MAX.

> + int num_tc = ndev->num_tc;
> + u32 value, queue_weight;
> + u16 offset, count;

Just use u32 here for all three.

> + int tc, i;
> +
> + if (!tcs)
> + goto __update_queue_tcs;
> +
> + for (tc = 0; tc < num_tc; tc++) {

@tc can be declared right here in the loop declaration.
Also it's u32 as it can't be negative.

> + count = ndev->tc_to_txq[tc].count;
> + offset = ndev->tc_to_txq[tc].offset;
> +
> + if (tcs & BIT(tc))
> + queue_tcs |= GENMASK(offset + count - 1, offset);
> +
> + /* This is 1:1 mapping, go to next TC */
> + if (count == 1)
> + continue;
> +
> + if (priv->plat->tx_sched_algorithm == MTL_TX_ALGORITHM_SP) {
> + NL_SET_ERR_MSG_MOD(extack, "TX algorithm SP is not suitable for one TC to multiple TXQs mapping");
> + return -EINVAL;
> + }
> +
> + queue_weight = priv->plat->tx_queues_cfg[offset].weight;
> + for (i = 1; i < count; i++) {

Same as with @tc for everything.

> + if (queue_weight != priv->plat->tx_queues_cfg[offset + i].weight) {
> + NL_SET_ERR_MSG_FMT_MOD(extack, "TXQ weight [%u] differs across other TXQs in TC: [%u]",
> + queue_weight, tc);
> + return -EINVAL;
> + }
> + }
> + }
> +
> +__update_queue_tcs:

Again underscores.

> + value = readl(ioaddr + MTL_FPE_CTRL_STS);
> +
> + value &= ~PEC;
> + value |= FIELD_PREP(PEC, queue_tcs);
> +
> + writel(value, ioaddr + MTL_FPE_CTRL_STS);

These are also u32_replace_bits() as in the previous patch.

> +
> + return 0;
> +}
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
> index e369e65920fc..d3191c48354d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
> @@ -40,6 +40,7 @@
> #define MAC_PPSx_WIDTH(x) (0x00000b8c + ((x) * 0x10))
>
> #define MTL_FPE_CTRL_STS 0x00000c90
> +#define PEC GENMASK(15, 8)

Again some driver prefix would be nice to see.

> #define AFSZ GENMASK(1, 0)
>
> #define MTL_RXP_CONTROL_STATUS 0x00000ca0

[...]

> @@ -1174,6 +1175,13 @@ static int tc_query_caps(struct stmmac_priv *priv,
> struct tc_query_caps_base *base)
> {
> switch (base->type) {
> + case TC_SETUP_QDISC_MQPRIO: {
> + struct tc_mqprio_caps *caps = base->caps;
> +
> + caps->validate_queue_counts = true;

Why not base->caps->blah directly? I think it would fit 80 cols?

> +
> + return 0;
> + }
> case TC_SETUP_QDISC_TAPRIO: {
> struct tc_taprio_caps *caps = base->caps;
>
> @@ -1190,6 +1198,78 @@ static int tc_query_caps(struct stmmac_priv *priv,
> }
> }
>
> +static void stmmac_reset_tc_mqprio(struct net_device *ndev,
> + struct netlink_ext_ack *extack)
> +{
> + struct stmmac_priv *priv = netdev_priv(ndev);
> +
> + netdev_reset_tc(ndev);
> + netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
> +
> + stmmac_fpe_set_preemptible_tcs(priv, ndev, extack, 0);
> +}
> +
> +static int tc_setup_mqprio(struct stmmac_priv *priv,
> + struct tc_mqprio_qopt_offload *mqprio)
> +{
> + struct netlink_ext_ack *extack = mqprio->extack;
> + struct tc_mqprio_qopt *qopt = &mqprio->qopt;
> + struct net_device *ndev = priv->dev;
> + int num_stack_tx_queues = 0;
> + int num_tc = qopt->num_tc;
> + u16 offset, count;

Also u32 for most of these.

> + int tc, err;
> +
> + if (!num_tc) {
> + stmmac_reset_tc_mqprio(ndev, extack);
> + return 0;
> + }
> +
> + if (mqprio->preemptible_tcs && !ethtool_dev_mm_supported(ndev)) {
> + NL_SET_ERR_MSG_MOD(extack, "Device does not support preemption");
> + return -EOPNOTSUPP;
> + }
> +
> + err = netdev_set_num_tc(ndev, num_tc);
> + if (err)
> + return err;
> +
> + /* DWMAC CORE4+ can not programming TC:TXQ mapping to hardware.

"can't program" or "can't do programming" or "doesn't support programming".

> + * Synopsys Databook:
> + * "The number of Tx DMA channels is equal to the number of Tx queues,
> + * and is direct one-to-one mapping."
> + *
> + * Luckily, DWXGMAC CORE can.
> + *
> + * Thus preemptible_tcs should be handled in a per core manner.
> + */

[...]

Thanks,
Olek