Re: [PATCH net-next v4 3/6] net: ethernet: ti: cpsw_ale: use regfields for number of Entries and Policers

[Roger Quadros <rogerq@kernel.org>]

Hi Geert,

On 23/09/2024 16:41, Geert Uytterhoeven wrote:
> Hi Roger,
> 
> On Tue, Sep 10, 2024 at 11:25 AM Roger Quadros <rogerq@kernel.org> wrote:
>> Use regfields for number of ALE Entries and Policers.
>>
>> The variants that support Policers/Classifiers have the number
>> of policers encoded in the ALE_STATUS register.
>>
>> Use that and show the number of Policers in the ALE info message.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> Reviewed-by: Simon Horman <horms@kernel.org>
>> ---
>> Changelog:
>> v4:
>> - reverse Xmas tree declaration order fixes
> 
> Thanks for your patch, which is now commit 11cbcfeaa79e5c76 ("net:
> ethernet: ti: cpsw_ale: use regfields for number of Entries
> and Policers").
> 
> This is causing the following warning on BeagleBone Black:
> 
>     WARNING: CPU: 0 PID: 34 at drivers/base/regmap/regmap.c:1208
> devm_regmap_field_alloc+0xac/0xc8
>     invalid empty mask defined
>     CPU: 0 UID: 0 PID: 34 Comm: kworker/u4:3 Not tainted
> 6.11.0-rc7-boneblack-01443-g11cbcfeaa79e #152
>     Hardware name: Generic AM33XX (Flattened Device Tree)
>     Workqueue: events_unbound deferred_probe_work_func
>     Call trace:
>      unwind_backtrace from show_stack+0x10/0x14
>      show_stack from dump_stack_lvl+0x68/0x88
>      dump_stack_lvl from __warn+0x6c/0x1a8
>      __warn from warn_slowpath_fmt+0x1bc/0x1d0
>      warn_slowpath_fmt from devm_regmap_field_alloc+0xac/0xc8
>      devm_regmap_field_alloc from cpsw_ale_create+0x10c/0x36c
>      cpsw_ale_create from cpsw_init_common+0x1fc/0x310
> 
>> --- a/drivers/net/ethernet/ti/cpsw_ale.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
>> @@ -1303,6 +1303,9 @@ static const struct reg_field ale_fields_cpsw_nu[] = {
>>         /* CPSW_ALE_IDVER_REG */
>>         [MINOR_VER]     = REG_FIELD(ALE_IDVER, 0, 7),
>>         [MAJOR_VER]     = REG_FIELD(ALE_IDVER, 8, 10),
>> +       /* CPSW_ALE_STATUS_REG */
>> +       [ALE_ENTRIES]   = REG_FIELD(ALE_STATUS, 0, 7),
>> +       [ALE_POLICERS]  = REG_FIELD(ALE_STATUS, 8, 15),
> 
> You are adding these entries only to ale_fields_cpsw_nu[], not
> to ale_fields_cpsw[], while cpsw_ale_regfield_init() loops over
> ALE_FIELDS_MAX entries, whether they are valid or not:
> 
>     static int cpsw_ale_regfield_init(struct cpsw_ale *ale)
>     {
>             const struct reg_field *reg_fields = ale->params.reg_fields;
>             struct device *dev = ale->params.dev;
>             struct regmap *regmap = ale->regmap;
>             int i;
> 
>             for (i = 0; i < ALE_FIELDS_MAX; i++) {
>                     ale->fields[i] = devm_regmap_field_alloc(dev, regmap,
>                                                              reg_fields[i]);
> 
>                     [...]
>             }
> 
>             return 0;
>     }
> 
> I tried fixing this by skipping entries where all of .reg, .lsb,
> and .msb are zero, but that doesn't work as that runs beyond the
> end of ale_fields_cpsw[], thus operating on random data.
> I think you do have to store the size of the array, instead of assuming
> ALE_FIELDS_MAX entries everywhere.

Thanks for the report and suggestion. I will send a fix soon.

> 
>> --- a/drivers/net/ethernet/ti/cpsw_ale.h
>> +++ b/drivers/net/ethernet/ti/cpsw_ale.h
>> @@ -33,6 +34,8 @@ struct regmap;
>>  enum ale_fields {
>>         MINOR_VER,
>>         MAJOR_VER,
>> +       ALE_ENTRIES,
>> +       ALE_POLICERS,
>>         /* terminator */
>>         ALE_FIELDS_MAX,
>>  };
>>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

-- 
cheers,
-roger