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:   Thu, 27 Jan 2022 17:57: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 v14, 2/2] net: Add dm9051 driver
Message-ID: <20220127175751.7ef239c1@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
In-Reply-To: <20220127032701.23056-3-josright123@gmail.com>
References: <20220127032701.23056-1-josright123@gmail.com>
        <20220127032701.23056-3-josright123@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 27 Jan 2022 11:27:01 +0800 Joseph CHAMG wrote:
> Add davicom dm9051 spi ethernet driver, The driver work for the
> device platform which has the spi master
> 
> Signed-off-by: Joseph CHAMG <josright123@gmail.com>

> +/* event: write into the mac registers and eeprom directly
> + */
> +static int dm9051_set_mac_address(struct net_device *ndev, void *p)
> +{
> +	struct board_info *db = to_dm9051_board(ndev);
> +	int ret;
> +
> +	ret = eth_mac_addr(ndev, p);

You should not be using this helper if the write can fail. See what
this function does internally and:

 - put the eth_prepare_mac_addr_change() call here

> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_bulk_write(db->regmap_dmbulk, DM9051_PAR, ndev->dev_addr, ETH_ALEN);
> +	if (ret < 0)
> +		netif_err(db, drv, ndev, "%s: error %d bulk writing reg %02x, len %d\n",
> +			  __func__, ret, DM9051_PAR, ETH_ALEN);

 - put the eth_commit_mac_addr_change() call here

> +	return ret;
> +}

> +static void dm9051_netdev(struct net_device *ndev)
> +{
> +	ndev->mtu = 1500;

Unnecessary, ether_setup() does this already.

> +	ndev->if_port = IF_PORT_100BASET;

Why set this? The if_port API is a leftover from very old 10Mbit
Ethernet days, we have ethtool link APIs now.

> +	ndev->netdev_ops = &dm9051_netdev_ops;
> +	ndev->ethtool_ops = &dm9051_ethtool_ops;

Just inline there two lines into the caller and remove the helper.
dm9051_netdev() does not sound like a function that does setup.

> +}
> +
> +static int dm9051_map_init(struct spi_device *spi, struct board_info *db)
> +{
> +	/* create two regmap instances,
> +	 * run read/write and bulk_read/bulk_write individually,
> +	 * to resolve regmap execution confliction problem
> +	 */
> +	regconfigdm.lock_arg = db;
> +	db->regmap_dm = devm_regmap_init_spi(db->spidev, &regconfigdm);
> +
> +	if (IS_ERR(db->regmap_dm))
> +		return PTR_ERR_OR_ZERO(db->regmap_dm);
> +
> +	regconfigdmbulk.lock_arg = db;
> +	db->regmap_dmbulk = devm_regmap_init_spi(db->spidev, &regconfigdmbulk);
> +

Please remove all the empty lines between function call and error
checking the result.

> +	if (IS_ERR(db->regmap_dmbulk))
> +		return PTR_ERR_OR_ZERO(db->regmap_dmbulk);

Why _OR_ZERO() when you're in a IS_ERR() condition already?

> +	return 0;

> +	ret = devm_register_netdev(dev, ndev);
> +	if (ret) {
> +		dev_err(dev, "failed to register network device\n");
> +		kthread_stop(db->kwr_task_kw);
> +		phy_disconnect(db->phydev);
> +		return ret;
> +	}
> +
> +	skb_queue_head_init(&db->txq);

All the state must be initialized before netdev is registered,
otherwise another thread may immediately open the device and
start to transmit.

> +	return 0;
> +
> +err_stopthread:
> +	kthread_stop(db->kwr_task_kw);
> +	return ret;
> +}