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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Reviewed-by: Simon Horman <horms@xxxxxxxxxx>
>> ---
>> 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
