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

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Wednesday, August 31, 2022 8:39 AM, Andrew Lunn wrote:
> n Tue, Aug 30, 2022 at 03:04:40PM +0800, Jiawen Wu wrote:
> >  /* 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.

> > +/**
> > + *  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
> > +	 * 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....
> 
> > +}
> > +
> > +/**
> > + *  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
> > +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);
> > +	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.