Re: [PATCH net-next v3 1/3] net: ti: icssm-prueth: Adds helper functions to configure and maintain FDB
From: Andrew Lunn <andrew@lunn.ch>
Date: 2025-10-14 21:58:15
Also in:
linux-arm-kernel, lkml
+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
+#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;
+ 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.
+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.
+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.
+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.
+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().
+ if (left > 0) {
+ hash_prev =
+ icssm_prueth_sw_fdb_hash
+ (FDB_MAC_TBL_ENTRY(left - 1)->mac);
+ }+ 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