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

[Markus Elfring]

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