RE: [PATCH net-next v2 02/16] net: txgbe: Reset hardware

On Wednesday, August 31, 2022 8:39 AM, Andrew Lunn wrote:

n Tue, Aug 30, 2022 at 03:04:40PM +0800, Jiawen Wu wrote:

quoted

 /* struct txgbe_mac_operations */
+static int txgbe_stop_adapter_dummy(struct txgbe_hw *TUP0) {
+	return -EPERM;

This is a bit of an odd error code. -EOPNOTSUPP would be more normal.

I do wonder what all this dummy stuff is for...

Okay, I just think that this way I don't need to determine whether the
function pointer is NULL every time it is called.

quoted

+/**
+ *  txgbe_stop_adapter - Generic stop Tx/Rx units
+ *  @hw: pointer to hardware structure
+ *
+ *  Sets the adapter_stopped flag within txgbe_hw struct. Clears
+interrupts,
+ *  disables transmit and receive units. The adapter_stopped flag is
+used by
+ *  the shared code and drivers to determine if the adapter is in a
+stopped
+ *  state and should not touch the hardware.
+ **/
+int txgbe_stop_adapter(struct txgbe_hw *hw) {
+	u16 i;
+
+	/* Set the adapter_stopped flag so other driver functions stop

touching

quoted

+	 * the hardware
+	 */
+	hw->adapter_stopped = true;
+
+	/* Disable the receive unit */
+	hw->mac.ops.disable_rx(hw);
+
+	/* Set interrupt mask to stop interrupts from being generated */
+	txgbe_intr_disable(hw, TXGBE_INTR_ALL);
+
+	/* Clear any pending interrupts, flush previous writes */
+	wr32(hw, TXGBE_PX_MISC_IC, 0xffffffff);
+	wr32(hw, TXGBE_BME_CTL, 0x3);
+
+	/* Disable the transmit unit.  Each queue must be disabled. */
+	for (i = 0; i < hw->mac.max_tx_queues; i++) {
+		wr32m(hw, TXGBE_PX_TR_CFG(i),
+		      TXGBE_PX_TR_CFG_SWFLSH | TXGBE_PX_TR_CFG_ENABLE,
+		      TXGBE_PX_TR_CFG_SWFLSH);
+	}
+
+	/* Disable the receive unit by stopping each queue */
+	for (i = 0; i < hw->mac.max_rx_queues; i++) {
+		wr32m(hw, TXGBE_PX_RR_CFG(i),
+		      TXGBE_PX_RR_CFG_RR_EN, 0);
+	}
+
+	/* flush all queues disables */
+	TXGBE_WRITE_FLUSH(hw);
+
+	/* Prevent the PCI-E bus from hanging by disabling PCI-E master
+	 * access and verify no pending requests
+	 */
+	return txgbe_disable_pcie_master(hw);

Interesting. You use it here....

quoted

+}
+
+/**
+ *  txgbe_disable_pcie_master - Disable PCI-express master access
+ *  @hw: pointer to hardware structure
+ *
+ *  Disables PCI-Express master access and verifies there are no
+pending
+ *  requests. TXGBE_ERR_MASTER_REQUESTS_PENDING is returned if

master

quoted

+disable
+ *  bit hasn't caused the master requests to be disabled, else 0
+ *  is returned signifying master requests disabled.
+ **/
+int txgbe_disable_pcie_master(struct txgbe_hw *hw) {
+	struct txgbe_adapter *adapter = container_of(hw, struct

txgbe_adapter,

hw);

quoted

+	int status = 0;
+	u32 val;

But define it here, afterwards. Wrong order. Swap this around, and remove

the

forward reference. And should this also be static? Is it used in any other

object

file?

Because you have it in the wrong order, the compiler cannot easily inline

it. The

optimised won't do as good a job as if it had seen it first before it was

also used.

Also, because it is not static, the compiler needs to keep a copy around

for the

linker to use with another object file.

So....

Define functions before you use them.

Make them static if possible.

Header files should only contain definitions of functions which are used
between object files, not within an object file.

There are indeed a lot of order errors in the code, I'll try to fix it.
When making the patch, I cut out every piece of functionality from the full
driver.
So there are some functions called later in other files, which currently
seems like a silly design.