netdev - Re: [net-next v2 00/12] add support for Renesas RZ/N1 ethernet subsystem devices

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]

Date:   Mon, 2 May 2022 17:31:50 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Clément Léger <clement.leger@...tlin.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Magnus Damm <magnus.damm@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Herve Codina <herve.codina@...tlin.com>,
        Miquèl Raynal <miquel.raynal@...tlin.com>,
        Milan Stevanovic <milan.stevanovic@...com>,
        Jimmy Lalande <jimmy.lalande@...com>,
        Pascal Eberhard <pascal.eberhard@...com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [net-next v2 00/12] add support for Renesas RZ/N1 ethernet
 subsystem devices

Hi Clément,

On Mon, May 2, 2022 at 5:10 PM Clément Léger <clement.leger@...tlin.com> wrote:
> Le Mon, 2 May 2022 14:27:38 +0200,
> Geert Uytterhoeven <geert@...ux-m68k.org> a écrit :
> > On Mon, May 2, 2022 at 8:52 AM Clément Léger
> > <clement.leger@...tlin.com> wrote:
> > > Le Fri, 29 Apr 2022 12:32:35 -0700,
> > > Jakub Kicinski <kuba@...nel.org> a écrit :
> > >
> > > > On Fri, 29 Apr 2022 16:34:53 +0200 Clément Léger wrote:
> > > > > The Renesas RZ/N1 SoCs features an ethernet subsystem which
> > > > > contains (most notably) a switch, two GMACs, and a MII
> > > > > converter [1]. This series adds support for the switch and the
> > > > > MII converter.
> > > > >
> > > > > The MII converter present on this SoC has been represented as a
> > > > > PCS which sit between the MACs and the PHY. This PCS driver is
> > > > > probed from the device-tree since it requires to be configured.
> > > > > Indeed the MII converter also contains the registers that are
> > > > > handling the muxing of ports (Switch, MAC, HSR, RTOS, etc)
> > > > > internally to the SoC.
> > > > >
> > > > > The switch driver is based on DSA and exposes 4 ports + 1 CPU
> > > > > management port. It include basic bridging support as well as
> > > > > FDB and statistics support.
> > > >
> > > > Build's not happy (W=1 C=1):
> > > >
> > > > drivers/net/dsa/rzn1_a5psw.c:574:29: warning: symbol
> > > > 'a5psw_switch_ops' was not declared. Should it be static? In file
> > > > included from ../drivers/net/dsa/rzn1_a5psw.c:17:
> > > > drivers/net/dsa/rzn1_a5psw.h:221:1: note: offset of packed
> > > > bit-field ‘port_mask’ has changed in GCC 4.4 221 | } __packed; | ^
> > > >
> > >
> > > Hi Jakub, I only had this one (due to the lack of W=1 C=1 I guess)
> > > which I thought (wrongly) that it was due to my GCC version:
> > >
> > >   CC      net/dsa/switch.o
> > >   CC      net/dsa/tag_8021q.o
> > > In file included from ../drivers/net/dsa/rzn1_a5psw.c:17:
> > > ../drivers/net/dsa/rzn1_a5psw.h:221:1: note: offset of packed
> > > bit-field ‘port_mask’ has changed in GCC 4.4 221 | } __packed;
> > >       | ^
> > >   CC      kernel/module.o
> > >   CC      drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.o
> > >   CC      drivers/net/ethernet/stmicro/stmmac/dwmac100_core.o
> > >
> > > I'll fix the other errors which are more trivial, however, I did not
> > > found a way to fix the packed bit-field warning.
> >
> > The "port_mask" field is split across 2 u8s.
> > What about using u16 instead, and adding explicit padding while
> > at it? The below gets rid of the warning, while the generated code
> > is the same.
> >
> > --- a/drivers/net/dsa/rzn1_a5psw.h
> > +++ b/drivers/net/dsa/rzn1_a5psw.h
> > @@ -169,10 +169,11 @@
> >
> >  struct fdb_entry {
> >         u8 mac[ETH_ALEN];
> > -       u8 valid:1;
> > -       u8 is_static:1;
> > -       u8 prio:3;
> > -       u8 port_mask:5;
> > +       u16 valid:1;
> > +       u16 is_static:1;
> > +       u16 prio:3;
> > +       u16 port_mask:5;
> > +       u16 reserved:6;
> >  } __packed;
>
> Hi Geert ! Indeed, while looking a bit more in depth at this error I
> found that using u16 avoids this error. I did switch to u16 but did not
> add any "reserved" field at the end and there is no more error. Do you
> think adding the "reserved" field would be preferable ?

As this structure reflects a hardware definition, I think it is better to
make this explicit.

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