Re: [PATCH] net: ethernet: ti: cpsw_ale: optimize cpsw_ale_restore()

[Roger Quadros <rogerq@kernel.org>]

On 10/11/2022 05:19, Jakub Kicinski wrote:
> On Tue,  8 Nov 2022 15:56:43 +0200 Roger Quadros wrote:
>> If an entry was FREE then we don't have to restore it.
> 
> Motivation? Does it make the restore faster?

Yes, since this would be called during system suspend/resume path.
I will update the commit message to mention this.

> 
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>
>> Patch depends on
>> https://lore.kernel.org/netdev/20221104132310.31577-3-rogerq@kernel.org/T/
>>
>>  drivers/net/ethernet/ti/cpsw_ale.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
>> index 0c5e783e574c..41bcf34a22f8 100644
>> --- a/drivers/net/ethernet/ti/cpsw_ale.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
>> @@ -1452,12 +1452,15 @@ void cpsw_ale_dump(struct cpsw_ale *ale, u32 *data)
>>  	}
>>  }
>>  
>> +/* ALE table should be cleared (ALE_CLEAR) before cpsw_ale_restore() */
> 
> Maybe my tree is old but I see we clear only if there is a netdev that

This patch depends on this series
https://lore.kernel.org/netdev/20221104132310.31577-3-rogerq@kernel.org/T/

> needs to be opened but then always call ale_restore(). Is that okay?

If netdev is closed and opened ale_restore() is not called.
ale_restore() is only called during system suspend/resume path
since CPSW-ALE might have lost context during suspend and we want to restore
all valid ALE entries.

I have a question here. How should ageable entries be treated in this case?

> 
> I'd also s/should/must/ 

OK, will fix.

> 
>>  void cpsw_ale_restore(struct cpsw_ale *ale, u32 *data)
>>  {
>> -	int i;
>> +	int i, type;
>>  
>>  	for (i = 0; i < ale->params.ale_entries; i++) {
>> -		cpsw_ale_write(ale, i, data);
>> +		type = cpsw_ale_get_entry_type(data);
>> +		if (type != ALE_TYPE_FREE)
>> +			cpsw_ale_write(ale, i, data);
>>  		data += ALE_ENTRY_WORDS;
>>  	}
>>  }
> 

cheers,
-roger