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

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: <CAMuHMdUf-tKRDzkz2_m8qdFTFutefddU0NTratVrEjRTzA3yQQ@mail.gmail.com>
Date: Mon, 23 Sep 2024 15:41:27 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Roger Quadros <rogerq@...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>, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, 
	Simon Horman <horms@...nel.org>, Andrew Lunn <andrew@...n.ch>, Joe Damato <jdamato@...tly.com>, 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, bpf@...r.kernel.org
Subject: Re: [PATCH net-next v4 3/6] net: ethernet: ti: cpsw_ale: use
 regfields for number of Entries and Policers

Hi Roger,

On Tue, Sep 10, 2024 at 11:25 AM Roger Quadros <rogerq@...nel.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@...nel.org>
> Reviewed-by: Simon Horman <horms@...nel.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.

> --- 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@...ux-m68k.org

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