Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <32eacc9d-3866-149a-579a-41f8e405123f@kernel.org>
Date:   Thu, 10 Nov 2022 11:39:47 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH] net: ethernet: ti: cpsw_ale: optimize cpsw_ale_restore()
Content-Language: en-US
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     davem@davemloft.net, edumazet@google.com, pabeni@redhat.com,
        vigneshr@ti.com, srk@ti.com, linux-omap@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20221108135643.15094-1-rogerq@kernel.org>
 <20221109191941.6af4f71d@kernel.org>
From:   Roger Quadros <rogerq@kernel.org>
In-Reply-To: <20221109191941.6af4f71d@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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