Hi,

quoted

+void icssm_prueth_sw_fdb_tbl_init(struct prueth *prueth)
+{
+	struct fdb_tbl *t = prueth->fdb_tbl;
+
+	t->index_a = (struct fdb_index_array_t *)((__force const void *)
+			prueth->mem[V2_1_FDB_TBL_LOC].va +
+			V2_1_FDB_TBL_OFFSET);

We have

quoted

+#define V2_1_FDB_TBL_LOC          PRUETH_MEM_SHARED_RAM

and existing code like:

void __iomem *sram_base = prueth->mem[PRUETH_MEM_SHARED_RAM].va;

so it seems like

t->index_a = sram_base + V2_1_FDB_TBL_OFFSET;

with no needs for any casts, since sram_base is a void * so can be
assigned to any pointer type.

And there are lots of cascading defines like:

/* 4 queue descriptors for port 0 (host receive). 32 bytes */
#define HOST_QUEUE_DESC_OFFSET          (HOST_QUEUE_SIZE_ADDR + 16)

/* table offset for queue size:
* 3 ports * 4 Queues * 1 byte offset = 12 bytes
*/
#define HOST_QUEUE_SIZE_ADDR            (HOST_QUEUE_OFFSET_ADDR + 8)
/* table offset for queue:
* 4 Queues * 2 byte offset = 8 bytes
*/
#define HOST_QUEUE_OFFSET_ADDR          (HOST_QUEUE_DESCRIPTOR_OFFSET_ADDR + 8)
/* table offset for Host queue descriptors:
* 1 ports * 4 Queues * 2 byte offset = 8 bytes
*/
#define HOST_QUEUE_DESCRIPTOR_OFFSET_ADDR       (HOST_Q4_RX_CONTEXT_OFFSET + 8)

allowing code like:

sram = sram_base + HOST_QUEUE_SIZE_ADDR;
sram = sram_base + HOST_Q1_RX_CONTEXT_OFFSET;
sram = sram_base + HOST_QUEUE_OFFSET_ADDR;
sram = sram_base + HOST_QUEUE_DESCRIPTOR_OFFSET_ADDR;
sram = sram_base + HOST_QUEUE_DESC_OFFSET;

Sure, we will check the feasibility and come back.

quoted

+	t->mac_tbl_a = (struct fdb_mac_tbl_array_t *)((__force const void *)
+			t->index_a + FDB_INDEX_TBL_MAX_ENTRIES *
+			sizeof(struct fdb_index_tbl_entry_t));

So i think this could follow the same pattern, also allowing some of
these casts to be removed.

I just don't like casts, they suggest bad design.

Sure, we will check the feasibility and come back.

quoted

+static u8 icssm_pru_lock_done(struct fdb_tbl *fdb_tbl)
+{
+	return readb((u8 __iomem *)&fdb_tbl->locks->pru_locks);

And maybe the __iomem attribute can be added to struct, either per
member, or at the top level? It is all iomem, so we want sparse to be
able to check all accesses.

Sure, we will check the feasibility and come back.

quoted

+static int icssm_prueth_sw_fdb_spin_lock(struct fdb_tbl *fdb_tbl)
+{
+	u8 done;
+	int ret;
+
+	/* Take the host lock */
+	writeb(1, (u8 __iomem *)&fdb_tbl->locks->host_lock);
+
+	/* Wait for the PRUs to release their locks */
+	ret = read_poll_timeout(icssm_pru_lock_done, done, done == 0,
+				1, 10, false, fdb_tbl);
+	if (ret)
+		return -ETIMEDOUT;
+
+	return 0;

Documentation says:

* Returns: 0 on success and -ETIMEDOUT upon a timeout.

So no need for the if statement.

Sure, we will address this in the next version.

quoted

+static s16
+icssm_prueth_sw_fdb_search(struct fdb_mac_tbl_array_t *mac_tbl,
+			   struct fdb_index_tbl_entry_t *bucket_info,
+			   const u8 *mac)
+{
+	u8 mac_tbl_idx = bucket_info->bucket_idx;
+	int i;
+
+	for (i = 0; i < bucket_info->bucket_entries; i++, mac_tbl_idx++) {
+		if (ether_addr_equal(mac,
+				     mac_tbl->mac_tbl_entry[mac_tbl_idx].mac))
+			return mac_tbl_idx;
+	}
+
+	return -ENODATA;

It is traditional to return errno in an int. But i don't see why a s16
cannot be used.

For now we will modify the return type to integer at all applicable places.

quoted

+icssm_prueth_sw_fdb_find_bucket_insert_point(struct fdb_tbl *fdb,
+					     struct fdb_index_tbl_entry_t
+					     *bkt_info,
+					     const u8 *mac, const u8 port)
+{
+	struct fdb_mac_tbl_array_t *mac_tbl = fdb->mac_tbl_a;
+	struct fdb_mac_tbl_entry_t *e;
+	u8 mac_tbl_idx;
+	int i, ret;
+	s8 cmp;
+
+	mac_tbl_idx = bkt_info->bucket_idx;
+
+	for (i = 0; i < bkt_info->bucket_entries; i++, mac_tbl_idx++) {
+		e = &mac_tbl->mac_tbl_entry[mac_tbl_idx];
+		cmp = memcmp(mac, e->mac, ETH_ALEN);
+		if (cmp < 0) {
+			return mac_tbl_idx;
+		} else if (cmp == 0) {
+			if (e->port != port) {
+				/* MAC is already in FDB, only port is
+				 * different. So just update the port.
+				 * Note: total_entries and bucket_entries
+				 * remain the same.
+				 */
+				ret = icssm_prueth_sw_fdb_spin_lock(fdb);
+				if (ret) {
+					pr_err("PRU lock timeout\n");
+					return -ETIMEDOUT;
+				}

icssm_prueth_sw_fdb_spin_lock() returns an errno. Don't replace it.

Also, pr_err() is bad practice and probably checkpatch is telling you
this. Ideally you want to indicate which device has an error, so you
should be using dev_err(), or maybe netdev_err().

We’ll return the “ret” value and replace the pr_err() with netdev_err()
so the message shows which device had the error.

quoted

+	if (left > 0) {
+		hash_prev =
+			icssm_prueth_sw_fdb_hash
+			(FDB_MAC_TBL_ENTRY(left - 1)->mac);
+	}

quoted

+		empty_slot_idx =
+			icssm_prueth_sw_fdb_check_empty_slot_right(mt, mti);

There are a couple of odd indentations like this. I wounder if it
makes sense to shorten the prefix? Do you really need all of
icssm_prueth_sw_fdb_ ?

	Andrew

The long prefix is to keep the functions names clear, but we will shorten
it and fix the indentations.


Thanks and Regards,
Parvathi.