netdev - Re: [PATCH net-next v5 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

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: <CAFSKS=P5u8YAL=1Rww0VqdHkcf11j7R-bJ02sj6pWoxvqRm3jw@mail.gmail.com>
Date:   Fri, 15 Jan 2021 09:30:29 -0600
From:   George McCollister <george.mccollister@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Rob Herring <robh@...nel.org>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        "open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>
Subject: Re: [PATCH net-next v5 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

On Thu, Jan 14, 2021 at 4:48 PM Vladimir Oltean <olteanv@...il.com> wrote:
>
> On Thu, Jan 14, 2021 at 01:57:33PM -0600, George McCollister wrote:
[snip]
>
> Reviewed-by: Vladimir Oltean <olteanv@...il.com>
>
> This driver is good to go, just one small nitpick below, you can fix it
> up afterwards if you want.
>
> > +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
> > +                                    u8 state)
> > +{
> > +     struct xrs700x *priv = ds->priv;
> > +     unsigned int bpdus = 1;
> > +     unsigned int val;
> > +
> > +     switch (state) {
> > +     case BR_STATE_DISABLED:
> > +             bpdus = 0;
> > +             fallthrough;
> > +     case BR_STATE_BLOCKING:
> > +     case BR_STATE_LISTENING:
> > +             val = XRS_PORT_DISABLED;
> > +             break;
> > +     case BR_STATE_LEARNING:
> > +             val = XRS_PORT_LEARNING;
> > +             break;
> > +     case BR_STATE_FORWARDING:
> > +             val = XRS_PORT_FORWARDING;
> > +             break;
> > +     default:
> > +             dev_err(ds->dev, "invalid STP state: %d\n", state);
> > +             return;
> > +     }
> > +
> > +     regmap_fields_write(priv->ps_forward, port, val);
> > +
> > +     /* Enable/disable inbound policy added by xrs700x_port_add_bpdu_ipf()
> > +      * which allows BPDU forwarding to the CPU port when the front facing
> > +      * port is in disabled/learning state.
>                       ~~~~~~~~
> You probably mean blocking. When the port is in BR_STATE_DISABLED, you
> set bpdus = 1, which makes sense.

That doesn't sound quite right, let me try to explain this differently
and you can tell me if it makes more sense. If so I'll update it, add
the reviewed-bys and post v6:

Enable/disable inbound policy added by xrs700x_port_add_bpdu_ipf()
which allows BPDU forwarding to the CPU port. The policy must be
enabled when the front facing port is in BLOCKING, LISTENING and
LEARNING BR_STATEs since the switch doesn't otherwise forward BPDUs
when the port is set to XRS_PORT_DISABLED and XRS_PORT_LEARNING.

>
> > +      */
> > +     regmap_update_bits(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 1, bpdus);
> > +
> > +     dev_dbg_ratelimited(priv->dev, "%s - port: %d, state: %u, val: 0x%x\n",
> > +                         __func__, port, state, val);
> > +}