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

[Andrew Lunn <andrew@lunn.ch>]

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