linux-kernel - Re: [RFC PATCH 2/3] net: sparx5: Add Sparx5 switchdev 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: <87h7p5pyr1.fsf@microchip.com>
Date:   Tue, 1 Dec 2020 12:11:14 +0100
From:   Lars Povlsen <lars.povlsen@...rochip.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Steen Hegelund <steen.hegelund@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        "Bjarni Jonasson" <bjarni.jonasson@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Microsemi List <microsemi@...ts.bootlin.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/3] net: sparx5: Add Sparx5 switchdev driver


Andrew Lunn writes:

>> +static void sparx5_attr_stp_state_set(struct sparx5_port *port,
>> +                                   struct switchdev_trans *trans,
>> +                                   u8 state)
>> +{
>> +     struct sparx5 *sparx5 = port->sparx5;
>> +
>> +     if (!test_bit(port->portno, sparx5->bridge_mask)) {
>> +             netdev_err(port->ndev,
>> +                        "Controlling non-bridged port %d?\n", port->portno);
>> +             return;
>> +     }
>> +
>> +     switch (state) {
>> +     case BR_STATE_FORWARDING:
>> +             set_bit(port->portno, sparx5->bridge_fwd_mask);
>> +             break;
>> +     default:
>> +             clear_bit(port->portno, sparx5->bridge_fwd_mask);
>> +             break;
>> +     }
>
> That is pretty odd. What about listening, learning, blocking?
>

This only handles simple forward/block. We'll add the learning state as
well.

>> +static int sparx5_port_bridge_join(struct sparx5_port *port,
>> +                                struct net_device *bridge)
>> +{
>> +     struct sparx5 *sparx5 = port->sparx5;
>> +
>> +     if (bitmap_empty(sparx5->bridge_mask, SPX5_PORTS))
>> +             /* First bridged port */
>> +             sparx5->hw_bridge_dev = bridge;
>> +     else
>> +             if (sparx5->hw_bridge_dev != bridge)
>> +                     /* This is adding the port to a second bridge, this is
>> +                      * unsupported
>> +                      */
>> +                     return -ENODEV;
>> +
>> +     set_bit(port->portno, sparx5->bridge_mask);
>> +
>> +     /* Port enters in bridge mode therefor don't need to copy to CPU
>> +      * frames for multicast in case the bridge is not requesting them
>> +      */
>> +     __dev_mc_unsync(port->ndev, sparx5_mc_unsync);
>> +
>> +     return 0;
>> +}
>
> This looks suspiciously empty? Don't you need to tell the hardware
> which ports this port is bridges to? Normally you see some code which
> walks all the ports and finds those in the same bridge, and sets a bit
> which allows these ports to talk to each other. Is that code somewhere
> else?
>

This is applied when the STP state is handled - sparx5_update_fwd().

This is pretty much as in the ocelot driver, which can a somewhat
similar switch - and driver - architecture.

>         Andrew

Thank you for your comments,

---Lars

--
Lars Povlsen,
Microchip