Re: [PATCH net-next v1 6/8] net: dsa: realtek: rtl8365mb: add QoS baseline and DCB default priority

[Luiz Angelo Daros de Luca]

Hi Oleksij,

> @@ -33,6 +33,8 @@ config NET_DSA_REALTEK_RTL8365MB
>         tristate "Realtek RTL8365MB switch driver"
>         depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
>         select NET_DSA_TAG_RTL8_4
> +       select NET_IEEE8021Q_HELPERS
> +       select DCB

Is 'select DCB' really desired here? Drivers generally avoid
unconditionally selecting DCB because it pulls in a non-negligible
amount of netlink and QoS machinery into the kernel, which is often
undesirable on memory/flash-constrained embedded targets (like MIPS
routers running OpenWrt). Since the DSA core already gates the dcb_ops
and only invokes .port_set_default_prio when CONFIG_DCB is enabled,
could DCB remain optional here (e.g. guarding the callbacks or
building rtl8365mb_dcb.c conditionally)?

>         help
>           Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
>

[...]

> +
> +/* The QoS priority and queue selectors are 3-bit register fields; derive a
> + * field's mask from its bit offset.
> + */
> +static inline u32 rtl8365mb_qos_sel_field_mask(unsigned int off)
> +{
> +       return GENMASK(off + 2, off);
> +}
> +
> +/* The priority-decision weight is an 8-bit register field; derive its mask
> + * from the field's bit offset, mirroring rtl8365mb_qos_sel_field_mask().
> + */
> +static inline u32 rtl8365mb_qos_weight_field_mask(unsigned int off)
> +{
> +       return GENMASK(off + 7, off);
> +}
> +
> +static int rtl8365mb_set_field(struct realtek_priv *priv, u32 reg, u32 mask,
> +                              u32 val)
> +{
> +       return regmap_update_bits(priv->map, reg, mask,
> +                                 (val << __ffs(mask)) & mask);
> +}
> +
> +static int rtl8365mb_get_field(struct realtek_priv *priv, u32 reg, u32 mask,
> +                              u32 *val)
> +{
> +       int ret;
> +
> +       ret = regmap_read(priv->map, reg, val);
> +       if (ret)
> +               return ret;
> +
> +       *val = (*val & mask) >> __ffs(mask);
> +       return 0;
> +}

The helper names ('set_field' / 'get_field') are a bit generic and
obscure that hardware regmap access is taking place. More importantly,
there is no need to hand-roll the shifting and masking with __ffs()
here. <linux/bitfield.h> provides the runtime-mask variants
field_prep() and field_get(), so you can simply do:

 return regmap_update_bits(priv->map, reg, mask, field_prep(mask, val));

and:

*val = field_get(mask, *val);

Or even drop these static wrappers entirely and use
field_prep()/field_get() directly at the call sites, matching common
driver practice.

> +
> +static u32 rtl8365mb_qos_pridec_reg(int table, int src)
> +{
> +       return table ? RTL8365MB_QOS_PRIDEC_TBL1_REG(src) :
> +                      RTL8365MB_QOS_PRIDEC_TBL0_REG(src);
> +}

There are other similar cases where the register is variable, as in
RTL8365MB_DIGITAL_INTERFACE_SELECT_REG. And even if the table will be
0 or 1, table == 1 is clearer than using it as a boolean.

> +
> +static int rtl8365mb_qos_set_pridec(struct realtek_priv *priv, int table,
> +                                   int src, u8 weight)
> +{
> +       int off = RTL8365MB_QOS_PRIDEC_OFFSET(src);
> +
> +       return rtl8365mb_set_field(priv, rtl8365mb_qos_pridec_reg(table, src),
> +                                  rtl8365mb_qos_weight_field_mask(off), weight);
> +}
> +

[...]

>
> @@ -3187,6 +3188,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>                 if (ret)
>                         goto out_teardown_irq;
>
> +               /* Default the port QoS (default priority = Best Effort) */
> +               ret = rtl8365mb_dcb_init_port(ds, dp->index);
> +               if (ret)
> +                       goto out_teardown_irq;
> +
>                 /* Set up per-port private data */
>                 p->priv = priv;
>                 p->index = dp->index;
> @@ -3274,6 +3280,13 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>          */
>         ds->num_tx_queues = mb->chip_info->num_tx_queues;
>
> +       /* Establish a defined QoS baseline: program the priority-to-queue map
> +        * for the chip's queue count and trust only the port default priority.
> +        */
> +       ret = rtl8365mb_dcb_init(ds);
> +       if (ret)
> +               goto out_teardown_irq;

Shouldn't the global QoS baseline (rtl8365mb_dcb_init) be established
before iterating over ports to set per-port defaults?

> +
>         /* Start statistics counter polling */
>         ret = rtl8365mb_stats_setup(priv);
>         if (ret) {
> @@ -3435,6 +3448,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops = {
>         .port_bridge_leave = rtl83xx_port_bridge_leave,
>         .port_pre_bridge_flags = rtl8365mb_port_pre_bridge_flags,
>         .port_bridge_flags = rtl83xx_port_bridge_flags,
> +       .port_get_default_prio = rtl8365mb_port_get_default_prio,
> +       .port_set_default_prio = rtl8365mb_port_set_default_prio,
>         .port_stp_state_set = rtl8365mb_port_stp_state_set,
>         .port_fast_age = rtl83xx_port_fast_age,
>         .port_fdb_add = rtl83xx_port_fdb_add,
> --
> 2.47.3
>

Best regards,

Luiz