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

[MD Danish Anwar <danishanwar@ti.com>]

Hi Andrew,

On 22/01/24 4:38 pm, MD Danish Anwar wrote:
> 
> 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.
> 

I have got confirmation from firmware team regarding "Does it still
learn while in BLOCK?"

ICSSG does not learn in BLOCK state. Learning will be done in Forwading
mode only. The ICSSG firmware doesn't support any state where "the port
will accept traffic only for the purpose of updating MAC address tables."

We only learn in forwarding mode where the packets is forwarded as well.

So, this will be the handling of different states by ICSSG firmware.

	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_LISTENING:
	case BR_STATE_BLOCKING:
		emac_state = ICSSG_EMAC_PORT_BLOCK;
		break;
	default:
		return -EOPNOTSUPP;
	}

I will make this change and other changes requested in this series and
post a v3 soon. Please let me know if you have any question.

>>
>>> +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