Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 28 Jan 2022 13:35:51 -0800
From:   Jakub Kicinski <kuba@kernel.org>
To:     Joseph CHAMG <josright123@gmail.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Rob Herring <robh+dt@kernel.org>, joseph_chang@davicom.com.tw,
        netdev@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
        andrew@lunn.ch, leon@kernel.org
Subject: Re: [PATCH v15, 2/2] net: Add dm9051 driver
Message-ID: <20220128133551.55c09fca@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
In-Reply-To: <20220128064532.2654-3-josright123@gmail.com>
References: <20220128064532.2654-1-josright123@gmail.com>
        <20220128064532.2654-3-josright123@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 28 Jan 2022 14:45:32 +0800 Joseph CHAMG wrote:
> +/* Open network device
> + * Called when the network device is marked active, such as a user executing
> + * 'ifconfig up' on the device
> + */
> +static int dm9051_open(struct net_device *ndev)
> +{
> +	struct board_info *db = to_dm9051_board(ndev);
> +	struct spi_device *spi = db->spidev;
> +	int ret;
> +
> +	db->imr_all = IMR_PAR | IMR_PRM;
> +	db->rcr_all = RCR_DIS_LONG | RCR_DIS_CRC | RCR_RXEN;
> +	db->lcr_all = LMCR_MODE1;
> +
> +	netif_wake_queue(ndev);

This should be last, after the device is actually ready to transmit.

> +	ndev->irq = spi->irq; /* by dts */
> +	ret = request_threaded_irq(spi->irq, NULL, dm9051_rx_threaded_irq,
> +				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +				   ndev->name, db);
> +	if (ret < 0) {
> +		netdev_err(ndev, "failed to get irq\n");
> +		return ret;
> +	}
> +
> +	phy_support_sym_pause(db->phydev); /* Enable support of sym pause */
> +	phy_start(db->phydev); /* it enclose with mutex_lock/mutex_unlock */
> +
> +	ret = dm9051_all_start(db);
> +	if (ret) {
> +		phy_stop(db->phydev);
> +		free_irq(spi->irq, db);
> +		netif_stop_queue(ndev);
> +		return ret;
> +	}
> +
> +	/* init pause param FlowCtrl */
> +	db->eth_pause.rx_pause = true;
> +	db->eth_pause.tx_pause = true;
> +	db->eth_pause.autoneg = AUTONEG_DISABLE;
> +
> +	if (db->phydev->autoneg)
> +		db->eth_pause.autoneg = AUTONEG_ENABLE;
> +
> +	return 0;
> +}
> +
> +/* Close network device
> + * Called to close down a network device which has been active. Cancel any
> + * work, shutdown the RX and TX process and then place the chip into a low
> + * power state while it is not being used
> + */
> +static int dm9051_stop(struct net_device *ndev)
> +{
> +	struct board_info *db = to_dm9051_board(ndev);
> +
> +	phy_stop(db->phydev);
> +	free_irq(db->spidev->irq, db);
> +	netif_stop_queue(ndev);

I don't see anything draining &db->txq when the worker is stopped.
It should probably be drained here, after the queue is stopped.

> +	return dm9051_all_stop(db);
> +}