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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <464535038.433548.1758892449616.JavaMail.zimbra@couthit.local>
Date: Fri, 26 Sep 2025 18:44:09 +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 1/3] net: ti: icssm-prueth: Adds helper
 functions to configure and maintain FDB

Hi,

>> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth_fdb_tbl.h
>> @@ -0,0 +1,66 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2019-2021 Texas Instruments Incorporated - https://www.ti.com
>> */
>> +#ifndef __NET_TI_PRUSS_FDB_TBL_H
>> +#define __NET_TI_PRUSS_FDB_TBL_H
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/debugfs.h>
>> +#include "icssm_prueth.h"
>> +
>> +#define ETHER_ADDR_LEN 6
> 
> Please use ETH_ALEN everywhere.
>

Sure, we will address this in the next version.

>> +static void icssm_prueth_sw_fdb_spin_lock(struct fdb_tbl *fdb_tbl)
>> +{
>> +	/* Take the host lock */
>> +	writeb(1, (u8 __iomem *)&fdb_tbl->locks->host_lock);
>> +
>> +	/* Wait for the PRUs to release their locks */
>> +	while (readb((u8 __iomem *)&fdb_tbl->locks->pru_locks))
>> +		;
> 
> Don't use endless loops. What happens if the firmware crashed. Please
> use something from iopoll.h and handle -ETIMEDOUT.
> 

Sure, we will replace the while loop with “read_poll_timeout()” API from
iopoll.h and handle the return value appropriately in the next version.

>> +static void icssm_mac_copy(u8 *dst, const u8 *src)
>> +{
>> +	u8 i;
>> +
>> +	for (i = 0; i < ETHER_ADDR_LEN; i++) {
>> +		*(dst) = *(src);
>> +		dst++;
>> +		src++;
>> +	}
>> +}
> 
> There is a kernel helper for this.
> 

We will use the “ether_addr_copy()” API where ever it is applicable.

>> +
>> +static s8 icssm_mac_cmp(const u8 *mac_a, const u8 *mac_b)
>> +{
>> +	s8  ret = 0, i;
>> +
>> +	for (i = 0; i < ETHER_ADDR_LEN; i++) {
>> +		if (mac_a[i] == mac_b[i])
>> +			continue;
>> +
>> +		ret = mac_a[i] < mac_b[i] ? -1 : 1;
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> I suspect there is also a helper for this. Please don't reinvent what
> the kernel already has.
> 

Sure, we will check and address this as well.

>> +static s16
>> +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;
>> +	s8 cmp;
>> +	int i;
>> +
>> +	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 = icssm_mac_cmp(mac, e->mac);
>> +		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.
>> +				 */
>> +				icssm_prueth_sw_fdb_spin_lock(fdb);
>> +				e->port = port;
>> +				icssm_prueth_sw_fdb_spin_unlock(fdb);
>> +			}
>> +
>> +			/* MAC and port are the same, touch the fdb */
>> +			e->age = 0;
>> +			return -1;
> 
> Returning values like this can result in bugs. It is better to use a
> real error code, just in case this ever makes it way back to
> userspace.
> 
> 	Andrew

Sure, we will check and address this in the next version.


Thanks and Regards,
Parvathi.