linux-kernel - Re: [PATCH] net: ethernet: ti: cpsw_ale: optimize cpsw_ale

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <32eacc9d-3866-149a-579a-41f8e405123f@kernel.org>
Date:   Thu, 10 Nov 2022 11:39:47 +0200
From:   Roger Quadros <rogerq@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
        vigneshr@...com, srk@...com, linux-omap@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: ethernet: ti: cpsw_ale: optimize cpsw_ale_restore()

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@...nel.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