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

[Parvathi Pudi <parvathi@couthit.com>]

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.