Re: [PATCH net-next v11 1/3] net: ti: icssm-prueth: Add helper functions to configure and maintain FDB

[Parvathi Pudi <parvathi@couthit.com>]

Hi,

> On 1/5/26 1:23 PM, Parvathi Pudi wrote:
>> +static void icssm_prueth_sw_fdb_update_index_tbl(struct prueth *prueth,
>> +						 u16 left, u16 right)
>> +{
>> +	unsigned int hash, hash_prev;
>> +	u8 mac[ETH_ALEN];
>> +	unsigned int i;
>> +
>> +	/* To ensure we don't improperly update the
>> +	 * bucket index, initialize with an invalid
>> +	 * hash in case we are in leftmost slot
>> +	 */
>> +	hash_prev = 0xff;
> 
> Why 0xff is an invalid index if the hash table size is 256?
> 

Although the hash table has 256 entries, valid indices are in the range of 0-255,
and 0xff(255) is never used as a previous index reference in this context.

Initializing the hash_prev to 0xff allows the code to detect the leftmost slot
case and avoid incorrectly updating the bucket index when no valid previous
entry exists.

>> +
>> +	if (left > 0) {
>> +		memcpy_fromio(mac, FDB_MAC_TBL_ENTRY(left - 1)->mac, ETH_ALEN);
>> +		hash_prev = icssm_prueth_sw_fdb_hash(mac);
>> +	}
>> +
>> +	/* For each moved element, update the bucket index */
>> +	for (i = left; i <= right; i++) {
>> +		memcpy_fromio(mac, FDB_MAC_TBL_ENTRY(i)->mac, ETH_ALEN);
>> +		hash = icssm_prueth_sw_fdb_hash(mac);
>> +
>> +		/* Only need to update buckets once */
>> +		if (hash != hash_prev)
>> +			writew(i, &FDB_IDX_TBL_ENTRY(hash)->bucket_idx);
>> +
>> +		hash_prev = hash;
>> +	}
>> +}
>> +
>> +static struct fdb_mac_tbl_entry __iomem *
>> +icssm_prueth_sw_find_free_mac(struct prueth *prueth, struct fdb_index_tbl_entry
>> +			      __iomem *bucket_info, u8 suggested_mac_tbl_idx,
>> +			      bool *update_indexes, const u8 *mac)
>> +{
>> +	s16 empty_slot_idx = 0, left = 0, right = 0;
>> +	unsigned int mti = suggested_mac_tbl_idx;
>> +	struct fdb_mac_tbl_array __iomem *mt;
>> +	struct fdb_tbl *fdb;
>> +	u8 flags;
>> +
>> +	fdb = prueth->fdb_tbl;
>> +	mt = fdb->mac_tbl_a;
>> +
>> +	flags = readb(&FDB_MAC_TBL_ENTRY(mti)->flags);
>> +	if (!(flags & FLAG_ACTIVE)) {
>> +		/* Claim the entry */
>> +		flags |= FLAG_ACTIVE;
>> +		writeb(flags, &FDB_MAC_TBL_ENTRY(mti)->flags);
>> +
>> +		return FDB_MAC_TBL_ENTRY(mti);
>> +	}
>> +
>> +	if (fdb->total_entries == FDB_MAC_TBL_MAX_ENTRIES)
>> +		return NULL;
>> +
>> +	empty_slot_idx = icssm_prueth_sw_fdb_empty_slot_left(mt, mti);
>> +	if (empty_slot_idx == -1) {
>> +		/* Nothing available on the left. But table isn't full
>> +		 * so there must be space to the right,
>> +		 */
>> +		empty_slot_idx = icssm_prueth_sw_fdb_empty_slot_right(mt, mti);
>> +
>> +		/* Shift right */
>> +		left = mti;
>> +		right = empty_slot_idx;
>> +		icssm_prueth_sw_fdb_move_range_right(prueth, left, right);
>> +
>> +		/* Claim the entry */
>> +		flags = readb(&FDB_MAC_TBL_ENTRY(mti)->flags);
>> +		flags |= FLAG_ACTIVE;
>> +		writeb(flags, &FDB_MAC_TBL_ENTRY(mti)->flags);
>> +
>> +		memcpy_toio(FDB_MAC_TBL_ENTRY(mti)->mac, mac, ETH_ALEN);
>> +
>> +		/* There is a chance we moved something in a
>> +		 * different bucket, update index table
>> +		 */
>> +		icssm_prueth_sw_fdb_update_index_tbl(prueth, left, right);
>> +
>> +		return FDB_MAC_TBL_ENTRY(mti);
> 
> AI review found what looks like a valid issue above:
> 
> """
> In this branch, FLAG_ACTIVE is set on FDB_MAC_TBL_ENTRY(mti) but the
> function returns FDB_MAC_TBL_ENTRY(empty_slot_idx). The caller in
> icssm_prueth_sw_insert_fdb_entry() then writes the MAC address to the
> returned entry (empty_slot_idx), leaving entry mti marked active with
> stale data.
> 
> Should FLAG_ACTIVE be set on empty_slot_idx instead? For comparison,
> the other paths in this function (lines 270-277, 294-306, and 330-342)
> all set FLAG_ACTIVE on the same entry they return and write MAC data to.
> """
> 

This looks valid, we will address this in the next version.

> Generally speaking the hash table handling looks complex and error
> prone. Is keeping the collided entries sorted really a win? I guess that
> always head-inserting would simplify the code a bit.
> 
> /P

Thanks and Regards,
Parvathi.