netdev - Re: [PATCH net-next v20 02/13] rtase: Implement the .ndo

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1d01ece4-bf4e-4266-942c-289c032bf44d@web.de>
Date: Thu, 13 Jun 2024 09:50:21 +0200
From: Markus Elfring <Markus.Elfring@....de>
To: Justin Lai <justinlai0215@...ltek.com>, netdev@...r.kernel.org,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
 Hariprasad Kelam <hkelam@...vell.com>, Jiri Pirko <jiri@...nulli.us>,
 Larry Chiu <larry.chiu@...ltek.com>, Ping-Ke Shih <pkshih@...ltek.com>,
 Ratheesh Kannoth <rkannoth@...vell.com>
Subject: Re: [PATCH net-next v20 02/13] rtase: Implement the .ndo_open
 function

…
> 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?

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

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/Documentation/process/coding-style.rst?h=v6.10-rc3#n116

…
> +static int rtase_alloc_rx_skb(const struct rtase_ring *ring,
…
> +{
…
> +	struct sk_buff *skb = NULL;
…
> +	int ret = 0;
…
> +	if (!page) {
> +		netdev_err(tp->dev, "failed to alloc page\n");
> +		goto err_out;
…
> +	if (!skb) {
…
> +		netdev_err(tp->dev, "failed to build skb\n");
> +		goto err_out;
> +	}
…
> +	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 = -ENOMEM;
> +	rtase_make_unusable_by_asic(desc);
> +
> +	return ret;
> +}
…

It seems that the following statement can be more appropriate.

	return -ENOMEM;

May the local variable “ret” be omitted here?

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

I suggest to return directly after such a resource allocation failure.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.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