Re: [RFC PATCH 2/3] net: sparx5: Add Sparx5 switchdev driver

[Lars Povlsen]

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