RE: [PATCH net-next v17 02/13] rtase: Implement the .ndo_open function

[Justin Lai <justinlai0215@realtek.com>]

> 
> 
> On Thu, May 02, 2024 at 05:18:36PM +0800, Justin Lai wrote:
> > Implement the .ndo_open function to set default hardware settings and
> > initialize the descriptor ring and interrupts. Among them, when
> > requesting irq, because the first group of interrupts needs to process
> > more events, the overall structure will be different from other groups
> > of interrupts, so it needs to be processed separately.
> >
> > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> 
> Hi Justin,
> 
> some minor feedback from my side.
> 
> > ---
> >  .../net/ethernet/realtek/rtase/rtase_main.c   | 419 ++++++++++++++++++
> >  1 file changed, 419 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > index 5ddb5f7abfe9..b286aac1eedc 100644
> > --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > @@ -130,6 +130,293 @@ static u32 rtase_r32(const struct rtase_private *tp,
> u16 reg)
> >       return readl(tp->mmio_addr + reg);  }
> >
> > +static void rtase_set_rxbufsize(struct rtase_private *tp) {
> > +     tp->rx_buf_sz = RTASE_RX_BUF_SIZE; }
> 
> I'm a big fan of helpers, but maybe it's better to just open-code this one as it is
> trivial and seems to only be used once.

OK, I understand what you mean, I will modify it.
> 
> > +
> > +     rtase_set_rxbufsize(tp);
> > +
> > +     ret = rtase_alloc_desc(tp);
> > +     if (ret)
> > +             goto err_free_all_allocated_mem;
> > +
> > +     ret = rtase_init_ring(dev);
> > +     if (ret)
> > +             goto err_free_all_allocated_mem;
> > +
> > +     rtase_hw_config(dev);
> > +
> > +     if (tp->sw_flag & RTASE_SWF_MSIX_ENABLED) {
> > +             ret = request_irq(ivec->irq, rtase_interrupt, 0,
> > +                               dev->name, ivec);
> > +             if (ret)
> > +                     goto err_free_all_allocated_irq;
> 
> This goto jumps to code that relies on i to set the bounds on a loop.
> However, i is not initialised here. Perhaps it should be set to 1?
> 
> Flagged by Smatch, and clang-18 W=1 builds.

Thank you for telling me the problem here, I will modify it.
> 
> > +
> > +             /* request other interrupts to handle multiqueue */
> > +             for (i = 1; i < tp->int_nums; i++) {
> > +                     ivec = &tp->int_vector[i];
> > +                     snprintf(ivec->name, sizeof(ivec->name),
> > + "%s_int%i", tp->dev->name, i);
> 
> nit: This line could trivially be split into two lines,
>      each less than 80 columns wide.
> 
I will check if there are other similar issues and make corrections.