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

[Geert Uytterhoeven]

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.

> --- 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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds