netdev - Re: [PATCH net-next 4/4 v4] net: dsa: rtl8366rb: Support setting STP state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdbBYr+wiuHqoPhk7OwZQZAcPMaUMnEU+ZYbERUbaaaAaw@mail.gmail.com>
Date:   Mon, 4 Oct 2021 23:07:21 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Mauri Sandberg <sandberg@...lfence.com>,
        DENG Qingfang <dqfext@...il.com>
Subject: Re: [PATCH net-next 4/4 v4] net: dsa: rtl8366rb: Support setting STP state

On Wed, Sep 29, 2021 at 11:54 PM Vladimir Oltean <olteanv@...il.com> wrote:

> > +/* Spanning tree status (STP) control, two bits per port per FID */
> > +#define RTL8368S_SPT_STATE_BASE              0x0050 /* 0x0050..0x0057 */
>
> What does "SPT" stand for?

No idea but I guess "spanning tree". It's what the register is named
in the vendor code:

/*
@func int32 | rtl8368s_setAsicSpanningTreeStatus | Configure spanning
tree state per each port.
@parm enum PORTID | port | Physical port number (0~5).
@parm uint32 | fid | FID of 8 SVL/IVL in port (0~7).
@parm enum SPTSTATE | state | Spanning tree state for FID of 8 SVL/IVL.
@rvalue SUCCESS | Success.
@rvalue ERRNO_SMI | SMI access error.
@rvalue ERRNO_PORT_NUM | Invalid port number.
@rvalue ERRNO_FID | Invalid FID.
@rvalue ERRNO_STP_STATE | Invalid spanning tree state
@common
    System supports 8 SVL/IVL configuration and each port has
dedicated spanning tree state setting for each FID. There are four
states supported by ASIC.

    Disable state         ASIC did not receive and transmit packets at
port with disable state.
    Blocking state        ASIC will receive BPDUs without L2 auto
learning and does not transmit packet out of port in blocking state.
    Learning state        ASIC will receive packets with L2 auto
learning and transmit out BPDUs only.
    Forwarding state    The port will receive and transmit packets normally.

*/
int32 rtl8368s_setAsicSpanningTreeStatus(enum PORTID port, enum
FIDVALUE fid, enum SPTSTATE state)
{
    uint32 regAddr;
    uint32 regData;
    uint32 regBits;
    int32 retVal;

    if(port >=PORT_MAX)
        return ERRNO_PORT_NUM;

    if(fid > RTL8368S_FIDMAX)
        return ERRNO_FID;

    if(state > FORWARDING)
        return ERRNO_STP_STATE;

    regAddr = RTL8368S_SPT_STATE_BASE + fid;
    regBits = RTL8368S_SPT_STATE_MSK << (port*RTL8368S_SPT_STATE_BITS);
    regData = (state << (port*RTL8368S_SPT_STATE_BITS)) & regBits;


    retVal = rtl8368s_setAsicRegBits(regAddr,regBits,regData);

    return retVal;
}

Maybe it is just the coder mixing up STP and SPT, but the register is indeed
named like this in their code.

> Also, is there any particular reason why these are named after RTL8368S,
> when the entire driver has a naming scheme which follows RTL8366RB?

Ooops, my bad. The RTL8368S == RTL8366RB AFAICT, the product
numbers from Realtek makes no sense.

> > +     mask = (RTL8368S_SPT_STATE_MSK << (port * 2));
>
> Could you not add a port argument:
>
> #define RTL8366RB_STP_MASK                      GENMASK(1, 0)
> #define RTL8366RB_STP_STATE(port, state)        (((state) << ((port) * 2))
> #define RTL8366RB_STP_STATE_MASK(port)          RTL8366RB_STP_STATE(RTL8366RB_STP_MASK, (port))
>
>         regmap_update_bits(smi->map, RTL8366RB_STP_STATE_BASE + i,
>                            RTL8366RB_STP_STATE_MASK(port),
>                            RTL8366RB_STP_STATE(port, val));

Yup that's neat, I'll do this!

Yours,
Linus Walleij