Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

Den 21. sep. 2017 16:21, skrev Andrew Lunn:
> Hi Egil
>
> > +static void lan9303_bridge_ports(struct lan9303 *chip)
> > +{
> > +	/* ports bridged: remove mirroring */
> > +	lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR, 0);
> > +}
>
> Could you replace the 0 with something symbolic which makes this
> easier to understand.
>
> #define LAN9303_SWE_PORT_MIRROR_DISABLED 0

OK

> > +
> >   static int lan9303_handle_reset(struct lan9303 *chip)
> >   {
> >   	if (!chip->reset_gpio)
> > @@ -844,6 +866,69 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
> >   	}
> >   }
> >   
> > +static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,
> > +				    struct net_device *br)
> > +{
> > +	struct lan9303 *chip = ds->priv;
> > +
> > +	dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
> > +	if (ds->ports[1].bridge_dev ==  ds->ports[2].bridge_dev) {
> > +		lan9303_bridge_ports(chip);
> > +		chip->is_bridged = true;  /* unleash stp_state_set() */
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void lan9303_port_bridge_leave(struct dsa_switch *ds, int port,
> > +				      struct net_device *br)
> > +{
> > +	struct lan9303 *chip = ds->priv;
> > +
> > +	dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
> > +	if (chip->is_bridged) {
> > +		lan9303_separate_ports(chip);
> > +		chip->is_bridged = false;
> > +	}
> > +}
> > +
> > +static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
> > +				       u8 state)
> > +{
> > +	int portmask, portstate;
> > +	struct lan9303 *chip = ds->priv;
> > +
> > +	dev_dbg(chip->dev, "%s(port %d, state %d)\n",
> > +		__func__, port, state);
> > +	if (!chip->is_bridged)
> > +		return; /* touching SWE_PORT_STATE will break port separation */
>
> I'm wondering how this is supposed to work. Please add a good comment
> here, since the hardware is forcing you to do something odd.
>
> Maybe it would be a good idea to save the STP state in chip.  And then
> when chip->is_bridged is set true, change the state in the hardware to
> the saved value?
>
> What happens when port 0 is added to the bridge, there is then a
> minute pause and then port 1 is added? I would expect that as soon as
> port 0 is added, the STP state machine for port 0 will start and move
> into listening and then forwarding. Due to hardware limitations it
> looks like you cannot do this. So what state is the hardware
> effectively in? Blocking? Forwarding?
>
> Then port 1 is added. You can then can respect the states. port 1 will
> do blocking->listening->forwarding, but what about port 0? The calls
> won't get repeated? How does it transition to forwarding?
>
>    Andrew

I see your point with the "minute pause" argument. Although a bit
contrived use case, it is easy to fix by caching the STP state, as
you suggest. So I can do that.

The port separation method is from the original version of the driver,
not by me.

I have read through the datasheet to see if there are other ways
to make port separation, but I could not find anything.
If anybody care to read through the 300+ page lan9303.pdf and prove
me wrong, I am happy to do it differently.

How does other DSA HW chips handle port separation? Knowing that
could perhaps help me know what to look for.

Egil
'

> > +
> > +	switch (state) {
> > +	case BR_STATE_DISABLED:
> > +		portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
> > +		break;
> > +	case BR_STATE_BLOCKING:
> > +	case BR_STATE_LISTENING:
> > +		portstate = LAN9303_SWE_PORT_STATE_BLOCKING_PORT0;
> > +		break;
> > +	case BR_STATE_LEARNING:
> > +		portstate = LAN9303_SWE_PORT_STATE_LEARNING_PORT0;
> > +		break;
> > +	case BR_STATE_FORWARDING:
> > +		portstate = LAN9303_SWE_PORT_STATE_FORWARDING_PORT0;
> > +		break;
> > +	default:
> > +		portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
> > +		dev_err(chip->dev, "unknown stp state: port %d, state %d\n",
> > +			port, state);
> > +	}
> > +
> > +	portmask = 0x3 << (port * 2);
> > +	portstate     <<= (port * 2);
> > +	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
> > +				      portstate, portmask);
> > +}