On 23/03/2018 at 14:41:25 -0700, Florian Fainelli wrote:
> On 03/23/2018 01:11 PM, Alexandre Belloni wrote:
> > Add a driver for Microsemi Ocelot Ethernet switch support.
> >
> > This makes two modules:
> > mscc_ocelot_common handles all the common features that doesn't depend on
> > how the switch is integrated in the SoC. Currently, it handles offloading
> > bridging to the hardware. ocelot_io.c handles register accesses. This is
> > unfortunately needed because the register layout is packed and then depends
> > on the number of ports available on the switch. The register definition
> > files are automatically generated.
> >
> > ocelot_board handles the switch integration on the SoC and on the board.
> >
> > Frame injection and extraction to/from the CPU port is currently done using
> > register accesses which is quite slow. DMA is possible but the port is not
> > able to absorb the whole switch bandwidth.
> >
> > Signed-off-by: Alexandre Belloni <alexandre.bell...@bootlin.com>
>
> Random drive by comments because this is quite a number of lines to review!
>
> Overall, looks quite good for a first version. Out of curiosity, is
> there a particular switch test you ran this driver against? LNST?
>
We have a really small custom test suite.
> > + /* Add dummy CRC */
> > + ocelot_write_rix(ocelot, 0, QS_INJ_WR, grp);
> > + skb_tx_timestamp(skb);
> > +
> > + dev->stats.tx_packets++;
> > + dev->stats.tx_bytes += skb->len;
> > + dev_kfree_skb_any(skb);
>
> No interrupt to indicate transmit completion?
>
No, unfortunately, the TX interrupts only indicates there is room to
start injecting frames, not that they have been transmitted.
>
> > +static int ocelot_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
> > + struct net_device *dev, const unsigned char *addr,
> > + u16 vid, u16 flags)
> > +{
> > + struct ocelot_port *port = netdev_priv(dev);
> > + struct ocelot *ocelot = port->ocelot;
> > +
> > + if (!vid) {
> > + if (!port->vlan_aware)
> > + /* If the bridge is not VLAN aware and no VID was
> > + * provided, set it to 1 as bridges have a default VID
> > + * of 1. Otherwise the MAC entry wouldn't match incoming
> > + * packets as the VID would differ (0 != 1).
> > + */
> > + vid = 1;
> > + else
> > + /* If the bridge is VLAN aware a VID must be provided as
> > + * otherwise the learnt entry wouldn't match any frame.
> > + */
> > + return -EINVAL;
> > + }
>
> So if we are targeting vid = 0 we end-up with vid = 1 possibly?
>
I've removed that part that is not needed for now and will rework when
sending VLAN support.
> > + ocelot_write_gix(ocelot, port_cfg, ANA_PORT_PORT_CFG,
> > + ocelot_port->chip_port);
> > +
> > + /* Apply FWD mask. The loop is needed to add/remove the current port as
> > + * a source for the other ports.
> > + */
> > + for (port = 0; port < ocelot->num_phys_ports; port++) {
> > + if (ocelot->bridge_fwd_mask & BIT(port)) {
> > + unsigned long mask = ocelot->bridge_fwd_mask &
> > ~BIT(port);
> > +
> > + for (i = 0; i < ocelot->num_phys_ports; i++) {
> > + unsigned long bond_mask = ocelot->lags[i];
> > +
> > + if (!bond_mask)
> > + continue;
> > +
> > + if (bond_mask & BIT(port)) {
> > + mask &= ~bond_mask;
> > + break;
> > + }
> > + }
> > +
> > + ocelot_write_rix(ocelot,
> > + BIT(ocelot->num_phys_ports) | mask,
> > + ANA_PGID_PGID, PGID_SRC + port);
> > + } else {
> > + /* Only the CPU port, this is compatible with link
> > + * aggregation.
> > + */
> > + ocelot_write_rix(ocelot,
> > + BIT(ocelot->num_phys_ports),
> > + ANA_PGID_PGID, PGID_SRC + port);
> > + }
>
> All of this sounds like it should be moved into the br_join/leave, this
> does not appear to be the right place to do that.
>
No, I've triple checked because this is a comment that both Andrew and
you had. Once a port is added to the PGID MASK, it will start forwarding
frames so we really want that to happen only when the port is in
BR_STATE_FORWARDING state. Else, we may forward frames between the
addition of the port to the bridge and setting the port to the
BR_STATE_BLOCKING state.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
