Re: [RFC PATCH v2 2/3] net: ti: icssg-switch: Add switchdev based driver for ethernet switch support

[MD Danish Anwar]

On 19/01/24 7:42 pm, Andrew Lunn wrote:
>> +static int prueth_switchdev_stp_state_set(struct prueth_emac *emac,
>> +					  u8 state)
>> +{
>> +	enum icssg_port_state_cmd emac_state;
>> +	int ret = 0;
>> +
>> +	switch (state) {
>> +	case BR_STATE_FORWARDING:
>> +		emac_state = ICSSG_EMAC_PORT_FORWARD;
>> +		break;
>> +	case BR_STATE_DISABLED:
>> +		emac_state = ICSSG_EMAC_PORT_DISABLE;
>> +		break;
>> +	case BR_STATE_LEARNING:
>> +	case BR_STATE_LISTENING:
>> +	case BR_STATE_BLOCKING:
>> +		emac_state = ICSSG_EMAC_PORT_BLOCK;
>> +		break;
> 
> That is unusual. Does it still learn while in BLOCK? It might be you
> need to flush the FDB for this port when it changes from BLOCKING to
> LISTENING or LEARNING?

ICSSG firmware supports four different states,
1. ICSSG_EMAC_PORT_DISABLE - port is in completed disable state and no
traffic is being processed.
2. ICSSG_EMAC_PORT_BLOCK - All traffic is blocked except for special
packets (eg LLDP BPDU frames)
3. ICSSG_EMAC_PORT_FORWARD - Port is fully active and every packet is
being processed. The port will also learn the mac address.
4. ICSSG_EMAC_PORT_FORWARD_WO_LEARNING - Port is fully active and every
packet is being processed but the port will also learn the mac address.

We don't have any state where we only learn and not do the forwarding.
So for BR_STATE_LISTENING and BR_STATE_BLOCKING I think state
ICSSG_EMAC_PORT_BLOCK is OK. For learning I am not sure what should be
the state. If both learning and forwarding is OK then we can set the
state to BR_STATE_FORWARDING.

> 
>> +static void prueth_switchdev_event_work(struct work_struct *work)
>> +{
>> +	struct prueth_switchdev_event_work *switchdev_work =
>> +		container_of(work, struct prueth_switchdev_event_work, work);
>> +	struct prueth_emac *emac = switchdev_work->emac;
>> +	struct switchdev_notifier_fdb_info *fdb;
>> +	int port_id = emac->port_id;
>> +	int ret;
>> +
>> +	rtnl_lock();
>> +	switch (switchdev_work->event) {
>> +	case SWITCHDEV_FDB_ADD_TO_DEVICE:
>> +		fdb = &switchdev_work->fdb_info;
>> +
>> +		netdev_dbg(emac->ndev, "prueth_fdb_add: MACID = %pM vid = %u flags = %u %u -- port %d\n",
>> +			   fdb->addr, fdb->vid, fdb->added_by_user,
>> +			   fdb->offloaded, port_id);
>> +
>> +		if (!fdb->added_by_user)
>> +			break;
>> +		if (memcmp(emac->mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
>> +			break;
> 
> ether_addr_equal(). Please review all the code and use these helpers
> when possible.

Sure.

> 
> So you don't add an FDB for the interfaces own MAC address? Does the
> switch know the interfaces MAC address?
> 

Interface's own mac address isn't needed to be added to FDB. Switch
already knows the interfaces mac_addr as during emac_ndo_open() we do
write the interface's mac_addr to MII_G_RT register [1] and adding the
mac_addr to MII_G_RT register is enough to let the firmware know.

In case we want interface to have more than 1 mac_addr, the the extra
mac_addr needs to be added to FDB.

>        Andrew

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/ti/icssg/icssg_prueth.c?h=v6.8-rc1#n1329

Thanks for the reviews and comments on all the patches. Please let me
know if more changes are needed in this series.

-- 
Thanks and Regards,
Danish