Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Vadym Kochan <vadym.kochan@plvision.eu>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Jiri Pirko <jiri@mellanox.com>,
	Ido Schimmel <idosch@mellanox.com>, Andrew Lunn <andrew@lunn.ch>,
	Oleksandr Mazur <oleksandr.mazur@plvision.eu>,
	Serhiy Boiko <serhiy.boiko@plvision.eu>,
	Serhiy Pshyk <serhiy.pshyk@plvision.eu>,
	Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>,
	Taras Chornyi <taras.chornyi@plvision.eu>,
	Andrii Savka <andrii.savka@plvision.eu>,
	netdev <netdev@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Mickey Rachamim <mickeyr@marvell.com>
Subject: Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Date: Tue, 8 Sep 2020 12:38:04 +0300	[thread overview]
Message-ID: <CAHp75VdyahsNyOK9_7mFGHFg_O47jVQWro-mhU0n=1K17Eeg8Q@mail.gmail.com> (raw)
In-Reply-To: <20200908083514.GB3562@plvision.eu>

On Tue, Sep 8, 2020 at 11:35 AM Vadym Kochan <vadym.kochan@plvision.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@plvision.eu> wrote:

...

> > > +       words[3] |= FIELD_PREP(PRESTERA_W3_HW_DEV_NUM, (dsa->hw_dev_num >> 5));
> >
> > Ditto.
> >
> I am not sure 5 needs to be defined as macro as it just moves
> hw_dev_num's higher bits into the last word.

And why 5? I want 6, for example!

...

> > > +       err = prestera_switch_init(sw);
> > > +       if (err) {
> > > +               kfree(sw);
> >
> > > +               return err;
> > > +       }
> > > +
> > > +       return 0;
> >
> > return err;
> >
> why not keep 'return 0' as indication of success point ?

Simple longer, but I'm not insisting. Your choice.

...

> > > +                       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)

-- 
With Best Regards,
Andy Shevchenko