Received-SPF: pass (google.com: domain of linux-kernel+bounces-167469-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 3 May 2024 10:33:31 +0100
From: Simon Horman <horms@kernel.org>
To: Justin Lai <justinlai0215@realtek.com>
Cc: kuba@kernel.org, davem@davemloft.net, edumazet@google.com,
	pabeni@redhat.com, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, andrew@lunn.ch, jiri@resnulli.us,
	pkshih@realtek.com, larry.chiu@realtek.com
Subject: Re: [PATCH net-next v17 01/13] rtase: Add pci table supported in
 this module
Message-ID: <20240503093331.GN2821784@kernel.org>
References: <20240502091847.65181-1-justinlai0215@realtek.com>
 <20240502091847.65181-2-justinlai0215@realtek.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240502091847.65181-2-justinlai0215@realtek.com>

On Thu, May 02, 2024 at 05:18:35PM +0800, Justin Lai wrote:
> Add pci table supported in this module, and implement pci_driver function
> to initialize this driver, remove this driver, or shutdown this driver.
> 
> Signed-off-by: Justin Lai <justinlai0215@realtek.com>

..

> diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> new file mode 100644
> index 000000000000..5ddb5f7abfe9
> --- /dev/null
> +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> @@ -0,0 +1,618 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/*
> + *  rtase is the Linux device driver released for Realtek Automotive Switch
> + *  controllers with PCI-Express interface.
> + *
> + *  Copyright(c) 2023 Realtek Semiconductor Corp.
> + *
> + *  Below is a simplified block diagram of the chip and its relevant interfaces.
> + *
> + *               *************************
> + *               *                       *
> + *               *  CPU network device   *
> + *               *                       *
> + *               *   +-------------+     *
> + *               *   |  PCIE Host  |     *
> + *               ***********++************
> + *                          ||
> + *                         PCIE
> + *                          ||
> + *      ********************++**********************
> + *      *            | PCIE Endpoint |             *
> + *      *            +---------------+             *
> + *      *                | GMAC |                  *
> + *      *                +--++--+  Realtek         *
> + *      *                   ||     RTL90xx Series  *
> + *      *                   ||                     *
> + *      *     +-------------++----------------+    *
> + *      *     |           | MAC |             |    *
> + *      *     |           +-----+             |    *
> + *      *     |                               |    *
> + *      *     |     Ethernet Switch Core      |    *
> + *      *     |                               |    *
> + *      *     |   +-----+           +-----+   |    *
> + *      *     |   | MAC |...........| MAC |   |    *
> + *      *     +---+-----+-----------+-----+---+    *
> + *      *         | PHY |...........| PHY |        *
> + *      *         +--++-+           +--++-+        *
> + *      *************||****************||***********

Thanks for the diagram, I like it a lot :)

> + *
> + *  The block of the Realtek RTL90xx series is our entire chip architecture,
> + *  the GMAC is connected to the switch core, and there is no PHY in between.
> + *  In addition, this driver is mainly used to control GMAC, but does not
> + *  control the switch core, so it is not the same as DSA.
> + */

..

> +static int rtase_alloc_msix(struct pci_dev *pdev, struct rtase_private *tp)
> +{
> +	int ret;
> +	u16 i;
> +
> +	memset(tp->msix_entry, 0x0, RTASE_NUM_MSIX * sizeof(struct msix_entry));
> +
> +	for (i = 0; i < RTASE_NUM_MSIX; i++)
> +		tp->msix_entry[i].entry = i;
> +
> +	ret = pci_enable_msix_exact(pdev, tp->msix_entry, tp->int_nums);
> +	if (!ret) {

In Linux Networking code it is an idiomatic practice to keep
handle errors in branches and use the main path of execution
for the non error path.

In this case I think that would look a bit like this:

	ret = pci_enable_msix_exact(pdev, tp->msix_entry, tp->int_nums);
	if (ret)
		return ret;

	...

	return 0;

> +
> +		for (i = 0; i < tp->int_nums; i++)
> +			tp->int_vector[i].irq = pci_irq_vector(pdev, i);

pci_irq_vector() can fail, should that be handled here?

> +	}
> +
> +	return ret;
> +}
> +
> +static int rtase_alloc_interrupt(struct pci_dev *pdev,
> +				 struct rtase_private *tp)
> +{
> +	int ret;
> +
> +	ret = rtase_alloc_msix(pdev, tp);
> +	if (ret) {
> +		ret = pci_enable_msi(pdev);
> +		if (ret)
> +			dev_err(&pdev->dev,
> +				"unable to alloc interrupt.(MSI)\n");

If an error occurs then it is a good practice to unwind resource
allocations made within the context of this function call, as this
leads to more symmetric unwind paths in callers.

In this case I think any resources consumed by rtase_alloc_msix()
should be released if pci_enable_msi fails. Probably using
a goto label is appropriate here.

Likewise, I suggest that similar logic applies to errors within
rtase_alloc_msix().

> +		else
> +			tp->sw_flag |= RTASE_SWF_MSI_ENABLED;
> +	} else {
> +		tp->sw_flag |= RTASE_SWF_MSIX_ENABLED;
> +	}
> +
> +	return ret;
> +}

..