netdev - Re: [PATCH v5 13/15] octeontx2-af: Add support for CGX link management

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] [day] [month] [year] [list]

Date:   Thu, 4 Oct 2018 23:57:23 +0530
From:   Sunil Kovvuri <sunil.kovvuri@...il.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>, linux-soc@...r.kernel.org,
        Linu Cherian <lcherian@...vell.com>,
        Nithya Mani <nmani@...vell.com>,
        Sunil Goutham <sgoutham@...vell.com>
Subject: Re: [PATCH v5 13/15] octeontx2-af: Add support for CGX link management

On Mon, Oct 1, 2018 at 6:17 PM Arnd Bergmann <arnd@...db.de> wrote:
>
> On Mon, Oct 1, 2018 at 1:37 PM <sunil.kovvuri@...il.com> wrote:
>
> > +/* Response to cmd ID as CGX_CMD_GET_FW_VER with cmd status as
> > + * CGX_STAT_SUCCESS
> > + */
> > +struct cgx_ver_s {
> > +#if defined(__BIG_ENDIAN_BITFIELD)
> > +       uint64_t reserved2:47;
> > +       uint64_t minor_ver:4;
> > +       uint64_t major_ver:4;
> > +       uint64_t reserved1:9;
> > +#else
> > +       uint64_t reserved1:9;
> > +       uint64_t major_ver:4;
> > +       uint64_t minor_ver:4;
> > +       uint64_t reserved2:47;
> > +#endif
> > +};
>
> This is not exactly what I meant in my comment. I think this may work
> correctly on all architectures that we support in Linux, but my point
> was that the layout of bit fields within a structure is completely up
> to the implementation, so it's really better to just avoid it entirely.
>
> Note that the data is being read with a call like
>
> ereg.val = cgx_read(cgx, lmac->lmac_id,  CGX_EVENT_REG);
>
> which in turn is implemented using readq(), and that already
> performs a byte swap in order to do the right thing on big-endian
> architectures. The non-reserved bitfields in the two structures
> above span byte boundaries, so I can't even wrap my head around
> where the respective bits are supposed to be for all combinations
> of __BIG_ENDIAN/__LITTLE_ENDIAN and __BIG_ENDIAN_BITFIELD/
> __LITTLE_ENDIAN_BITFIELD.
>
>       Arnd


Hi Arnd,

Thanks for your patience in checking patches and giving us feedback.
I have submitted v6 removing bitfields and using BITMASKs to extract fields.

Regards,
Sunil.