linux-kernel - Re: [PATCH v14, 2/2] net: Add dm9051 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220127175751.7ef239c1@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 27 Jan 2022 17:57:51 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Joseph CHAMG <josright123@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>, joseph_chang@...icom.com.tw,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, andy.shevchenko@...il.com,
        andrew@...n.ch, leon@...nel.org
Subject: Re: [PATCH v14, 2/2] net: Add dm9051 driver

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@...il.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;
> +}