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]

Date:   Fri, 15 Jan 2021 20:43:42 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     George McCollister <george.mccollister@...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 Fri, Jan 15, 2021 at 09:30:29AM -0600, George McCollister wrote:
> 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.

No, that was clear in the first place. It is even intelligible with the
comment as-is, otherwise I would have asked you what you mean. You don't
need to resent. I made a mistake when I said "you set bpdus = 1", you
obviously set bpdus = 0 for BR_STATE_DISABLED and 1 for everything else.
I think I should just get more sleep before giving review comments.