netdev - Re: [PATCHv5 2/3] net: socionext: Add Synquacer NetSec driver

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:   Fri, 5 Jan 2018 22:23:21 +0530
From:   Jassi Brar <jaswinder.singh@...aro.org>
To:     David Miller <davem@...emloft.net>
Cc:     Jassi Brar <jassisinghbrar@...il.com>, netdev@...r.kernel.org,
        Devicetree List <devicetree@...r.kernel.org>,
        Arnd Bergmann <arnd.bergmann@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>
Subject: Re: [PATCHv5 2/3] net: socionext: Add Synquacer NetSec driver

On 3 January 2018 at 21:05, David Miller <davem@...emloft.net> wrote:
> From: jassisinghbrar@...il.com
> Date: Mon,  1 Jan 2018 10:44:49 +0530
>
>> +#define DRING_TAIL(r)                ((r)->tail)
>> +
>> +#define DRING_HEAD(r)                ((r)->head)
>
> These macros do not help readability at all.
>
>> +#define MOVE_TAIL(r)         do { \
>> +                                     if (++(r)->tail == DESC_NUM) \
>> +                                             (r)->tail = 0; \
>> +                             } while (0)
>> +
>> +#define MOVE_HEAD(r)         do { \
>> +                                     if (++(r)->head == DESC_NUM) \
>> +                                             (r)->head = 0; \
>> +                             } while (0)
>> +
>> +#define JUMP_HEAD(r, n)      do { \
>> +                                     int i; \
>> +                                     for (i = 0; i < (n); i++) \
>> +                                             MOVE_HEAD(r); \
>> +                             } while (0)
>
> Neither do these.
>
OK, I have removed these.

> And JUMP_HEAD is so inefficient, it's a constant time calculation:
>
>         r->head += n;
>         if (r->head >= DESC_NUM)
>                 r->head -= DESC_NUM;
>
How about the more compact    r->head = (r->head + n) % DESC_NUM

> All of this stuff can be done inline without all of these CPP macros
> which are discouraged, have multiple evaluation issues, and decrease
> the amount of type checking going on.
>
> If you absolutely must have helpers, use static functions (without
> the inline keyword, let the compiler device).
>
>> +static inline int available_descs(struct netsec_desc_ring *r)
>
> No inline functions in foo.c files, let the compiler device.
>
OK. I have already started selling it downstream.

>> +/*************************************************************/
>> +/*********************** NETDEV_OPS **************************/
>> +/*************************************************************/
>
> Please, comments are not billboards or Star Wars openning sequences.
> Simplify this, thank you.
>
Done  :)

>> +
>> +static void netsec_set_tx_de(struct netsec_priv *priv,
>> +                          struct netsec_desc_ring *dring,
>> +                          const struct netsec_tx_pkt_ctrl *tx_ctrl,
>> +                          const struct netsec_desc *desc,
>> +                          struct sk_buff *skb)
>> +{
>> +     struct netsec_de *de;
>> +     int idx = DRING_HEAD(dring);
>> +     u32 attr;
>
> Please order local variables from longest to shortest line.
>
> Please audit your entire submission for this issue.
>
That was the only occurrence. I already am particular about following
that style.

Thank you.