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

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

[Siddharth Vadapalli]

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

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

[Siddharth Vadapalli]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Roger Quadros]

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

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

[Roger Quadros]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Patil, Tanmay]

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

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

[Patil, Tanmay]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 12 Jul 2023 16:36:57 +0530 you 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.
> 
> [...]

Here is the summary with links:
  - [net] net: ethernet: ti: cpsw_ale: Fix cpsw_ale_get_field()/cpsw_ale_set_field()
    https://git.kernel.org/netdev/net/c/b685f1a58956

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 12 Jul 2023 16:36:57 +0530 you 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.
> 
> [...]

Here is the summary with links:
  - [net] net: ethernet: ti: cpsw_ale: Fix cpsw_ale_get_field()/cpsw_ale_set_field()
    https://git.kernel.org/netdev/net/c/b685f1a58956

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel