| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1336430040.1005.1760615639799.JavaMail.zimbra@couthit.local>
Date: Thu, 16 Oct 2025 17:23:59 +0530 (IST)
From: Parvathi Pudi <parvathi@...thit.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: parvathi <parvathi@...thit.com>, andrew+netdev <andrew+netdev@...n.ch>,
davem <davem@...emloft.net>, edumazet <edumazet@...gle.com>,
kuba <kuba@...nel.org>, pabeni <pabeni@...hat.com>,
danishanwar <danishanwar@...com>, rogerq <rogerq@...nel.org>,
pmohan <pmohan@...thit.com>, basharath <basharath@...thit.com>,
afd <afd@...com>, linux-kernel <linux-kernel@...r.kernel.org>,
netdev <netdev@...r.kernel.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
pratheesh <pratheesh@...com>, Prajith Jayarajan <prajith@...com>,
Vignesh Raghavendra <vigneshr@...com>, praneeth <praneeth@...com>,
srk <srk@...com>, rogerq <rogerq@...com>,
krishna <krishna@...thit.com>, mohan <mohan@...thit.com>
Subject: Re: [PATCH net-next v3 1/3] net: ti: icssm-prueth: Adds helper
functions to configure and maintain FDB
Hi,
>> +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;
>
Sure, we will check the feasibility and come back.
>> + 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.
>> +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.
>> +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.
>> +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.
>> +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.
>> + 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
The long prefix is to keep the functions names clear, but we will shorten
it and fix the indentations.
Thanks and Regards,
Parvathi.
Powered by blists - more mailing lists