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

From: Justin Lai
Date: Sun May 05 2024 - 22:45:41 EST


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