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

Andrew Lunn
2025-10-14T21:58:24Z

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