Re: [PATCH net] net: ethernet: ti: cpsw_ale: Fix cpsw_ale_get_field()/cpsw_ale_set_field()

[Roger Quadros <rogerq@kernel.org>]

On 12/07/2023 14:06, Siddharth Vadapalli wrote:
> From: Tanmay Patil <t-patil@ti.com>
> 
> CPSW ALE has 75 bit ALE entries which are stored within three 32 bit words.
> The cpsw_ale_get_field() and cpsw_ale_set_field() functions assume that the
> field will be strictly contained within one word. However, this is not
> guaranteed to be the case and it is possible for ALE field entries to span
> across up to two words at the most.
> 
> Fix the methods to handle getting/setting fields spanning up to two words.
> 
> Fixes: db82173f23c5 ("netdev: driver: ethernet: add cpsw address lookup engine support")
> Signed-off-by: Tanmay Patil <t-patil@ti.com>
> [s-vadapalli@ti.com: rephrased commit message and added Fixes tag]
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/net/ethernet/ti/cpsw_ale.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
> index 0c5e783e574c..64bf22cd860c 100644
> --- a/drivers/net/ethernet/ti/cpsw_ale.c
> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
> @@ -106,23 +106,37 @@ struct cpsw_ale_dev_id {
>  
>  static inline int cpsw_ale_get_field(u32 *ale_entry, u32 start, u32 bits)
>  {
> -	int idx;
> +	int idx, idx2;
> +	u32 hi_val = 0;
>  
>  	idx    = start / 32;
> +	idx2 = (start + bits - 1) / 32;
> +	/* Check if bits to be fetched exceed a word */
> +	if (idx != idx2) {
> +		idx2 = 2 - idx2; /* flip */
> +		hi_val = ale_entry[idx2] << ((idx2 * 32) - start);
> +	}
>  	start -= idx * 32;
>  	idx    = 2 - idx; /* flip */
> -	return (ale_entry[idx] >> start) & BITMASK(bits);
> +	return (hi_val + (ale_entry[idx] >> start)) & BITMASK(bits);

Should this be bit-wise OR instead of ADD?

>  }
>  
>  static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32 bits,
>  				      u32 value)
>  {
> -	int idx;
> +	int idx, idx2;
>  
>  	value &= BITMASK(bits);
> -	idx    = start / 32;
> +	idx = start / 32;
> +	idx2 = (start + bits - 1) / 32;
> +	/* Check if bits to be set exceed a word */
> +	if (idx != idx2) {
> +		idx2 = 2 - idx2; /* flip */
> +		ale_entry[idx2] &= ~(BITMASK(bits + start - (idx2 * 32)));
> +		ale_entry[idx2] |= (value >> ((idx2 * 32) - start));
> +	}
>  	start -= idx * 32;
> -	idx    = 2 - idx; /* flip */
> +	idx = 2 - idx; /* flip */
>  	ale_entry[idx] &= ~(BITMASK(bits) << start);
>  	ale_entry[idx] |=  (value << start);
>  }

-- 
cheers,
-roger