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

[Vadym Kochan <vadym.kochan@plvision.eu>]

Hi Andy,

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@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!
> 
> ...

Actually this shifting is conceptually not right and should be mirrored
with PRESTERA_DSA_DEV_NUM genmask from prestera_dsa_parse(), so instead I
did this:

...
u32 dev_num = dsa->hw_dev_num;
...
dev_num = FIELD_GET(PRESTERA_DSA_DEV_NUM, dev_num);
words[3] |= FIELD_PREP(PRESTERA_DSA_W3_DEV_NUM, dev_num);
...

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

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.

> -- 
> With Best Regards,
> Andy Shevchenko

Thanks!