netdev - Re: [PATCH net-next v2 5/7] net: ethernet: ti: cpsw

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: <ab602b9d-dfdf-4d46-9d11-08801c5c52c8@kernel.org>
Date: Mon, 1 Jul 2024 12:46:49 +0300
From: Roger Quadros <rogerq@...nel.org>
To: Simon Horman <horms@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Siddharth Vadapalli <s-vadapalli@...com>,
 Julien Panis <jpanis@...libre.com>, Andrew Lunn <andrew@...n.ch>,
 srk@...com, vigneshr@...com, danishanwar@...com, pekka Varis
 <p-varis@...com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-omap@...r.kernel.org
Subject: Re: [PATCH net-next v2 5/7] net: ethernet: ti: cpsw_ale: add
 policer/classifier helpers



On 01/07/2024 10:35, Simon Horman wrote:
> On Fri, Jun 28, 2024 at 03:01:54PM +0300, Roger Quadros wrote:
>> The Policer registers in the ALE register space are just shadow registers
>> and use an index field in the policer table control register to read/write
>> to the actual Polier registers.
>> Add helper functions to Read and Write to Policer registers.
>>
>> Also add a helper function to set the thread value to classifier/policer
>> mapping. Any packet that first matches the classifier will be sent to the
>> thread (flow) that is set in the classifer to thread mapping table.
> 
> nit: classifier
> 
>      Flagged by checkpatch.pl --codespell

will fix.

> 
>> If not set then it goes to the default flow.
>>
>> Signed-off-by: Roger Quadros <rogerq@...nel.org>
>> ---
>>  drivers/net/ethernet/ti/cpsw_ale.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
>> index 7bd0dc20f894..75a17184d34c 100644
>> --- a/drivers/net/ethernet/ti/cpsw_ale.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
>> @@ -1626,3 +1626,27 @@ u32 cpsw_ale_get_num_entries(struct cpsw_ale *ale)
>>  {
>>  	return ale ? ale->params.ale_entries : 0;
>>  }
>> +
>> +/* Reads the specified policer index into ALE POLICER registers */
>> +static void cpsw_ale_policer_read_idx(struct cpsw_ale *ale, u32 idx)
>> +{
>> +	idx &= ALE_POLICER_TBL_INDEX_MASK;
>> +	writel_relaxed(idx, ale->params.ale_regs + ALE_POLICER_TBL_CTL);
>> +}
>> +
>> +/* Writes the ALE POLICER registers into the specified policer index */
>> +static void cpsw_ale_policer_write_idx(struct cpsw_ale *ale, u32 idx)
>> +{
>> +	idx &= ALE_POLICER_TBL_INDEX_MASK;
>> +	idx |= ALE_POLICER_TBL_WRITE_ENABLE;
>> +	writel_relaxed(idx, ale->params.ale_regs + ALE_POLICER_TBL_CTL);
>> +}
>> +
>> +/* enables/disables the custom thread value for the specified policer index */
>> +static void cpsw_ale_policer_thread_idx_enable(struct cpsw_ale *ale, u32 idx,
>> +					       u32 thread_id, bool enable)
>> +{
>> +	regmap_field_write(ale->fields[ALE_THREAD_CLASS_INDEX], idx);
>> +	regmap_field_write(ale->fields[ALE_THREAD_VALUE], thread_id);
>> +	regmap_field_write(ale->fields[ALE_THREAD_ENABLE], enable ? 1 : 0);
>> +}
>>
> 
> I like that this patch-set is broken out into nice discrete patches,
> including this one. So I'm in two minds about the comment I'm about to
> make, but here goes.
> 
> As these helpers are unused this raises Warnings with W=1 builds on
> gcc-13 and clang-18, which is generally undesirable for networking patches.
> 
> I can think of a few options here;
> * Ignore the warnings
> * Squash this patch into the following one
> * Add some annotations, e.g. __maybe_unused or __always_unused.
>   Likely dropped in the next patch.
> 
> I think I lean towards the last option.
> But I won't push the point any further regardless.

Thanks for the suggestions. I do not like using any of the __unused flags.
So I'm leaning towards your second solution. Will fix this in next revision.

-- 
cheers,
-roger