netdev - Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for Prestera family ASIC 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:   Wed, 9 Sep 2020 17:09:02 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Vadym Kochan <vadym.kochan@...ision.eu>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Andrew Lunn <andrew@...n.ch>,
        Oleksandr Mazur <oleksandr.mazur@...ision.eu>,
        Serhiy Boiko <serhiy.boiko@...ision.eu>,
        Serhiy Pshyk <serhiy.pshyk@...ision.eu>,
        Volodymyr Mytnyk <volodymyr.mytnyk@...ision.eu>,
        Taras Chornyi <taras.chornyi@...ision.eu>,
        Andrii Savka <andrii.savka@...ision.eu>,
        netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mickey Rachamim <mickeyr@...vell.com>
Subject: Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for
 Prestera family ASIC devices

On Wed, Sep 9, 2020 at 5:00 PM Vadym Kochan <vadym.kochan@...ision.eu> wrote:
> On Tue, Sep 08, 2020 at 12:38:04PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 8, 2020 at 11:35 AM Vadym Kochan <vadym.kochan@...ision.eu> wrote:
> > > On Fri, Sep 04, 2020 at 10:12:07PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan <vadym.kochan@...ision.eu> wrote:

...

> > > > > +                       if (b == 0)
> > > > > +                               continue;
> > > > > +
> > > > > +                       prestera_sdma_rx_desc_set_next(sdma,
> > > > > +                                                      ring->bufs[b - 1].desc,
> > > > > +                                                      buf->desc_dma);
> > > > > +
> > > > > +                       if (b == PRESTERA_SDMA_RX_DESC_PER_Q - 1)
> > > > > +                               prestera_sdma_rx_desc_set_next(sdma, buf->desc,
> > > > > +                                                              head->desc_dma);
> > > >
> > > > I guess knowing what the allowed range of bnum the above can be optimized.
> > > >
> > > You mean to replace PRESTERA_SDMA_RX_DESC_PER_Q by bnum ?
> >
> > I don't know what you meant in above. It might be a bug, it might be
> > that bnum is redundant and this definition may be used everywhere...
> > But I believe there is room for improvement when I see pattern like
> >
> >   for (i < X) {
> >     ...
> >     if (i == 0) {
> >       ...
> >     } else if (i == X - 1) {
> >       ...
> >     }
> >   }
> >
> > Either it can be while-loop (or do-while) with better semantics for
> > the first and last item to handle or something else.
> > Example from another review [1] in case you wonder how changes can be
> > made. Just think about it.
> >
> > [1]: https://www.spinics.net/lists/linux-pci/msg60826.html (before)
> > https://www.spinics.net/lists/linux-pci/msg62043.html (after)
> >
>
> I came up with the following approach which works:
>
> -------------->8------------------------------------------------
> tail = &ring->bufs[bnum - 1];
> head = &ring->bufs[0];
> next = head;
> prev = next;
> ring->next_rx = 0;
>
> do {
>         err = prestera_sdma_buf_init(sdma, next);
>         if (err)
>                 return err;
>
>         err = prestera_sdma_rx_skb_alloc(sdma, next);
>         if (err)
>                 return err;
>
>         prestera_sdma_rx_desc_init(sdma, next->desc, next->buf_dma);
>
>         prestera_sdma_rx_desc_set_next(sdma, prev->desc, next->desc_dma);
>
>         prev = next;
>         next++;
> } while (prev != tail);
>
> /* make a circular list */
> prestera_sdma_rx_desc_set_next(sdma, tail->desc, head->desc_dma);
> --------------8<------------------------------------------------
>
> Now it looks more list-oriented cyclic logic.

And much better, thanks!

Note, you have two places in the code with something similar (I mean
loop handling), perhaps you may improve both.

-- 
With Best Regards,
Andy Shevchenko