Re: [PATCH net-next v7 06/10] net: dsa: realtek: rtl8365mb: use raw VID for PVID on family D

[Luiz Angelo Daros de Luca]

> Family D has no working VLAN member config (MC) table. Its PVID is a
> raw 12-bit VID in a per-port register at 0x0700 + port, whereas family C
> stores a 5-bit MC table index, two ports per register, at
> 0x0700 + (port >> 1).
>
> Running the family C code on family D therefore reads an MC table that
> does not exist, and writes an MC index into the VID register of port
> (port >> 1) instead of the one of the port being configured.
>
> Read the PVID straight from its register in
> rtl8365mb_vlan_port_get_pvid(), and make rtl8365mb_vlan_pvid_port_set()
> and rtl8365mb_vlan_pvid_port_clear() use direct-VID variants on family D
> that skip the MC table. They keep the family C frame filter handling and
> rollback on error.
>
> Also clear the family D PVID registers when the VLAN support is set up.
> Unlike on family C, where VLANMC index 0 is the neutral PVID, the
> register holds a raw VID, and after a reset it keeps its hardware (or
> EEPROM) default. Without clearing it an unconfigured port could report
> a stale PVID from rtl8365mb_vlan_port_get_pvid(), and
> rtl8365mb_port_vlan_filtering() would accept untagged frames into it.
>
> No functional change for family C.
>
> The family D branch is not reachable until the chip table entry is
> added in the last patch.
>
> Signed-off-by: Mieczyslaw Nalewaj <namiltd@xxxxxxxxx>
> Reviewed-by: Linus Walleij <linusw@xxxxxxxxxx>
> ---
>  drivers/net/dsa/realtek/rtl8365mb_main.c |   9 +
>  drivers/net/dsa/realtek/rtl8365mb_vlan.c | 205 ++++++++++++++++++++++-
>  drivers/net/dsa/realtek/rtl8365mb_vlan.h |   1 +
>  3 files changed, 210 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index d69eed0..e82205c 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
> @@ -2360,6 +2360,15 @@ static int rtl8365mb_vlan_setup(struct dsa_switch *ds)
>         int ret;
>
>         dsa_switch_for_each_available_port(dp, ds) {
> +               /* Start every port with the neutral PVID */
> +               ret = rtl8365mb_vlan_port_reset_pvid(priv, dp->index);

We already have a clear and a set operation. Why do we need a separate reset?
[...]

> --- a/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> @@ -44,6 +44,10 @@
>   *
>   * VLANMC index 0 is reserved as a neutral PVID, used for standalone ports.
>   *
> + * Everything above describes chips that have a VLAN membership configuration
> + * table. RTL8365MB_FAMILY_D chips do not have one: their PVID is a raw 12-bit
> + * VID in a per-port register, and no VLANMC entries are allocated.
> + *
>   */
>
>  #include "rtl8365mb_vlan.h"
> @@ -120,6 +124,11 @@
>  #define   RTL8365MB_VLAN_PVID_CTRL_PORT_MCIDX_MASK(_p) \
>                 (0x1F << RTL8365MB_VLAN_PVID_CTRL_PORT_MCIDX_OFFSET(_p))
>
> +#define RTL8365MB_D_VLAN_PVID_CTRL_BASE                        0x0700
> +#define RTL8365MB_D_VLAN_PVID_CTRL_REG(port) \
> +       (RTL8365MB_D_VLAN_PVID_CTRL_BASE + (port))
> +#define RTL8365MB_D_VLAN_PVID_CTRL_MASK                        GENMASK(11, 0)
> +
>  /* Frame type filtering registers */
>  #define RTL8365MB_VLAN_ACCEPT_FRAME_TYPE_BASE  0x07aa
>  #define RTL8365MB_VLAN_ACCEPT_FRAME_TYPE_REG(port) \
> @@ -171,7 +180,7 @@ struct rtl8365mb_vlan4k {
>   * This structure is used to get/set entries in the VLAN membership
>   * configuration database. This feature is largely vestigial, but
>   * still needed for at least the following features:
> - *   - PVID configuration
> + *   - PVID configuration (family C only, family D has no VLAN MC table)
>   *   - ACL configuration
>   *   - selection of VLAN by the CPU tag when VSEL=1, although the switch
>   *     can also select VLAN based on the VLAN tag if VSEL=0
> @@ -740,14 +749,51 @@ int rtl8365mb_vlan_port_get_pvid(struct realtek_priv *priv, int port, u16 *pvid)
>         u8 vlanmc_idx;
>         int ret;
>
> -       ret = rtl8365mb_vlan_get_pvid_mc(priv, port, &vlanmc_idx, &vlanmc);
> -       if (ret)
> -               return ret;
> +       if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D) {
> +               u32 data;
> +
> +               ret = regmap_read(priv->map,
> +                                 RTL8365MB_D_VLAN_PVID_CTRL_REG(port),
> +                                 &data);
> +               if (ret)
> +                       return ret;
> +
> +               *pvid = data & RTL8365MB_D_VLAN_PVID_CTRL_MASK;
> +       } else {
> +               ret = rtl8365mb_vlan_get_pvid_mc(priv, port, &vlanmc_idx,
> +                                                &vlanmc);
> +               if (ret)
> +                       return ret;
> +
> +               *pvid = vlanmc.evid;
> +       }
>
> -       *pvid = vlanmc.evid;
>         return 0;
>  }

If the functions don't share most of their code, they shouldn't be
lumped together inside a shared function with an if/else.

>
> +/**
> + * rtl8365mb_vlan_port_reset_pvid() - Put a port's PVID in its neutral state
> + * @priv: realtek_priv pointer
> + * @port: the port number
> + *
> + * On family C the neutral PVID is VLANMC index 0, which is the reset value, so
> + * there is nothing to do. On family D the PVID is a raw VID in a per-port
> + * register whose value after reset is not defined by the driver (it may come
> + * from the hardware default or from an EEPROM), so clear it to 0.

I'm not entirely convinced by this. VLANMC index 0 isn't a
hardware-defined neutral index; it's just a convention we use where
VID == 0. The driver treats VID 0 as "unset" alongside the ingress
filter. Furthermore, checking rtl8365mb_get_family(priv) !=
RTL8365MB_FAMILY_D doesn't scale well if we eventually introduce a
family E.

> + *
> + * Context: Can sleep.
> + * Return: 0 on success, or a negative error code on failure.
> + */
> +int rtl8365mb_vlan_port_reset_pvid(struct realtek_priv *priv, int port)
> +{
> +       if (rtl8365mb_get_family(priv) != RTL8365MB_FAMILY_D)
> +               return 0;
> +
> +       return regmap_update_bits(priv->map,
> +                                 RTL8365MB_D_VLAN_PVID_CTRL_REG(port),
> +                                 RTL8365MB_D_VLAN_PVID_CTRL_MASK, 0);
> +}
> +
>  /*
>   * rtl8365mb_vlan_port_get_framefilter() - Get the ingress frame filtering mode
>   * for a port
> @@ -809,6 +855,68 @@ rtl8365mb_vlan_port_set_framefilter(struct realtek_priv *priv,
>                                   val);
>  }
>
> +/*
> + * rtl8365mb_vlan_pvid_port_set_direct() - Configure a port's PVID as a raw

The term "direct" doesn't give me much meaning. Since it has
essentially the same signature, it should simply be a clean,
family-specific implementation (same goes for the other functions).

Best regards,

Luiz