linux-kernel - 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] [day] [month] [year] [list]

Message-ID: <02f2c50f-31f6-4d4a-9cbe-5f77d1d60706@lunn.ch>
Date: Thu, 25 Sep 2025 16:33:40 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Parvathi Pudi <parvathi@...thit.com>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, danishanwar@...com,
	rogerq@...nel.org, pmohan@...thit.com, basharath@...thit.com,
	afd@...com, linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, pratheesh@...com,
	prajith@...com, vigneshr@...com, praneeth@...com, srk@...com,
	rogerq@...com, krishna@...thit.com, mohan@...thit.com
Subject: Re: [PATCH net-next 1/3] net: ti: icssm-prueth: Adds helper
 functions to configure and maintain FDB

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

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

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

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

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