Received-SPF: pass (google.com: domain of linux-kernel+bounces-169265-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Justin Lai <justinlai0215@realtek.com>
To: Simon Horman <horms@kernel.org>
CC: "kuba@kernel.org" <kuba@kernel.org>,
        "davem@davemloft.net"
	<davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org"
	<netdev@vger.kernel.org>,
        "andrew@lunn.ch" <andrew@lunn.ch>,
        "jiri@resnulli.us" <jiri@resnulli.us>,
        Ping-Ke Shih <pkshih@realtek.com>, Larry Chiu <larry.chiu@realtek.com>
Subject: RE: [PATCH net-next v17 02/13] rtase: Implement the .ndo_open function
Thread-Topic: [PATCH net-next v17 02/13] rtase: Implement the .ndo_open
 function
Thread-Index: AQHanHHZpNHx0voP+0CDD26jysJVP7GEr5aAgATU6AA=
Date: Mon, 6 May 2024 02:45:11 +0000
Message-ID: <745b2ee9e81f4904920e0e4fe6e4df89@realtek.com>
References: <20240502091847.65181-1-justinlai0215@realtek.com>
 <20240502091847.65181-3-justinlai0215@realtek.com>
 <20240503085257.GM2821784@kernel.org>
In-Reply-To: <20240503085257.GM2821784@kernel.org>
Accept-Language: zh-TW, en-US
Content-Language: zh-TW
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0

>=20
>=20
> 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>
>=20
> Hi Justin,
>=20
> some minor feedback from my side.
>=20
> > ---
> >  .../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 =3D RTASE_RX_BUF_SIZE; }
>=20
> I'm a big fan of helpers, but maybe it's better to just open-code this on=
e as it is
> trivial and seems to only be used once.

OK, I understand what you mean, I will modify it.
>=20
> > +
> > +     rtase_set_rxbufsize(tp);
> > +
> > +     ret =3D rtase_alloc_desc(tp);
> > +     if (ret)
> > +             goto err_free_all_allocated_mem;
> > +
> > +     ret =3D 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 =3D request_irq(ivec->irq, rtase_interrupt, 0,
> > +                               dev->name, ivec);
> > +             if (ret)
> > +                     goto err_free_all_allocated_irq;
>=20
> 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?
>=20
> Flagged by Smatch, and clang-18 W=3D1 builds.

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