Received-SPF: pass (google.com: domain of linux-kernel+bounces-212750-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <1d01ece4-bf4e-4266-942c-289c032bf44d@web.de>
Date: Thu, 13 Jun 2024 09:50:21 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Markus Elfring <Markus.Elfring@web.de>
Subject: Re: [PATCH net-next v20 02/13] rtase: Implement the .ndo_open
 function
To: Justin Lai <justinlai0215@realtek.com>, netdev@vger.kernel.org,
 "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>,
 Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
 Simon Horman <horms@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>, Andrew Lunn <andrew@lunn.ch>,
 Hariprasad Kelam <hkelam@marvell.com>, Jiri Pirko <jiri@resnulli.us>,
 Larry Chiu <larry.chiu@realtek.com>, Ping-Ke Shih <pkshih@realtek.com>,
 Ratheesh Kannoth <rkannoth@marvell.com>
References: <20240607084321.7254-3-justinlai0215@realtek.com>
Content-Language: en-GB
In-Reply-To: <20240607084321.7254-3-justinlai0215@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:bPMSKqIDmNg=;1s7bZP01Mg+SoE2pyTmiWtVKX1O
 nih5VoK6GtdGEBqdtN7o6edm7axYQptZvaW1x9iE8JUInpgIqHrZPeUPaP3T59WizG1P5/jtn
 ztwYXz2Y8fLp2iHzKjITY6+jzCcZ7t7ebXD8iVwkWRCqoxyg0yTSXNmkQMuDdpPVzDxq+ofTC
 6O/TXKxm4AFmOh3UWZC7qWoHgtCfysX+od21Icszbn9L0wFRqWLTRVJe2zxV9rVoceoMBMFv0
 rf7e7j86Jh4F5pCqkMTsf6shlZKAHENyJIjlAhPHlv34ynSFPSq599OOpSuFtlurWIjrl7PTm
 3Sf2f19Fvr80QNZaNFIu27sPW+3RDnDpMXgBNz76I/ic7TNVi/g0QeaEry1UAoS72W5KlRVIm
 AoTXqlorS4h7KcyEqTjKfVS2Qjr0htIF3xp8AZvOLul24uqFfYrQ/+zwscIxtW7MNfKyKwEvZ
 90HNZYpxb1VHjESZJu7g68g967R70msUT3N7cNpPBx1kI1mu3cL7jXbhCYut90bc8BFJbohov
 wte0ulagnYPJWBd4rY5dOXChuBOW5/a+j3afdUiRfmetFbMp+mHhBvcHerc2i2bgPClhVo45H
 INuq2Su2qNJUxj/A0QPhbtcaCCqawiNZWbn3JmgdybMWyiL49zesge4452Xi6D695ecmkIyA+
 ngzqkUtXSTAEieh3fkU+fH/VV3eQZcb5e0jKnPqCuGOXXr1fSBqqKis2vXnzXwdgPsK5ShMl6
 O8aC853Sp3eCrS1de4sfYacuA+4Jp8GyBr2KRykd8HCV89ed6n6CDw4oMvbkZkCkYXwMa4IoD
 jJYBWY+ErFMgOOdmSEtjZxPwkz/hucjo9rLTA0sipTAj8=

=E2=80=A6
> 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.

Can such a change description become clearer anyhow?


=E2=80=A6
> +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
=E2=80=A6
> +static int rtase_alloc_desc(struct rtase_private *tp)
> +{
=E2=80=A6
> +			netdev_err(tp->dev, "Failed to allocate dma memory of "
> +					    "tx descriptor.\n");
=E2=80=A6

Would you like to keep the message (from such string literals) in a single=
 line?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Do=
cumentation/process/coding-style.rst?h=3Dv6.10-rc3#n116


=E2=80=A6
> +static int rtase_alloc_rx_skb(const struct rtase_ring *ring,
=E2=80=A6
> +{
=E2=80=A6
> +	struct sk_buff *skb =3D NULL;
=E2=80=A6
> +	int ret =3D 0;
=E2=80=A6
> +	if (!page) {
> +		netdev_err(tp->dev, "failed to alloc page\n");
> +		goto err_out;
=E2=80=A6
> +	if (!skb) {
=E2=80=A6
> +		netdev_err(tp->dev, "failed to build skb\n");
> +		goto err_out;
> +	}
=E2=80=A6
> +	return ret;

I find the following statement more appropriate.

	return 0;


> +
> +err_out:
> +	if (skb)
> +		dev_kfree_skb(skb);

Why would you like to repeat such a check after it can be determined
from the control flow that the used variable contains still a null pointer=
?


> +
> +	ret =3D -ENOMEM;
> +	rtase_make_unusable_by_asic(desc);
> +
> +	return ret;
> +}
=E2=80=A6

It seems that the following statement can be more appropriate.

	return -ENOMEM;


May the local variable =E2=80=9Cret=E2=80=9D be omitted here?


=E2=80=A6
> +static int rtase_open(struct net_device *dev)
> +{
=E2=80=A6
> +	int ret;
> +
> +	ivec =3D &tp->int_vector[0];
> +	tp->rx_buf_sz =3D RTASE_RX_BUF_SIZE;
> +
> +	ret =3D rtase_alloc_desc(tp);
> +	if (ret)
> +		goto err_free_all_allocated_mem;
=E2=80=A6

I suggest to return directly after such a resource allocation failure.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Do=
cumentation/process/coding-style.rst?h=3Dv6.10-rc3#n532


How do you think about to increase the application of scope-based resource=
 management?
https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/cleanup.h#=
L8

Regards,
Markus