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

["Patil, Tanmay" <t-patil@ti.com>]

On 7/12/2023 7:20 PM, Roger Quadros wrote:
> 
> 
> 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?
> 

As the hi_val has been declared and left shifted in this function, we 
are sure that the trailing bits are all '0'. Hence we can directly add them.

>>   }
>>   
>>   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);
>>   }
>