linux-kernel - Re: [PATCH v12, 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] [day] [month] [year] [list]

Message-ID: <YeripbSOeq1s2U3u@lunn.ch>
Date:   Fri, 21 Jan 2022 17:43:17 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Joseph CHAMG <josright123@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        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,
        Leon Romanovsky <leon@...nel.org>
Subject: Re: [PATCH v12, 2/2] net: Add dm9051 driver

> +static int ctrl_dm9051_phywrite(void *context, unsigned int reg, unsigned int val)
> +{
> +	/* chip internal operation need wait 1 ms for if power-up phy
> +	 */

> +	if (reg == MII_BMCR && !(val & BMCR_PDOWN))
> +		mdelay(1);

What PHY driver are you using? It would be much better to have this in
the PHY driver. The MAC driver should not be touching the PHY.

> +static int dm9051_phy_connect(struct board_info *db)
> +{
> +	char phy_id[MII_BUS_ID_SIZE + 3];
> +
> +	snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT,
> +		 db->mdiobus->id, DM9051_PHY_ID);
> +
> +	db->phydev = phy_connect(db->ndev, phy_id, dm9051_handle_link_change,
> +				 PHY_INTERFACE_MODE_MII);
> +	if (IS_ERR(db->phydev))
> +		return PTR_ERR_OR_ZERO(db->phydev);

Why PTR_ERR_OR_ZERO()

> +static int dm9051_direct_fifo_reset(struct board_info *db)
> +{
> +	struct net_device *ndev = db->ndev;
> +	int rxlen = le16_to_cpu(db->eth_rxhdr.rxlen);

reverse christmas tree. There are a few more cases. Please review the
whole driver.

> +/* transmit a packet,
> + * return value,
> + *   0 - succeed
> + *  -ETIMEDOUT - timeout error
> + */
> +static int dm9051_single_tx(struct board_info *db, u8 *buff, unsigned int len)
> +{
> +	int ret;
> +
> +	ret = dm9051_map_xmitpoll(db);
> +	if (ret)
> +		return -ETIMEDOUT;
> +

If dm9051_map_xmitpoll() returns an error code, use it. There needs to
be a good reason to change the error code, and if you have such a good
reason, please add a comment about it.

> +static irqreturn_t dm9051_rx_threaded_irq(int irq, void *pw)
> +{
> +	struct board_info *db = pw;
> +	int result, resul_tx;
> +
> +	mutex_lock(&db->spi_lockm); /* mutex essential */

When are mutex's not essential? This commit seems to be
meaningless. It gives the impression you don't understand mutex's and
locking in general. You have just added mutex until it seems to work,
not that you have a locking design.

> +	if (netif_carrier_ok(db->ndev)) {
> +		result = regmap_write(db->regmap_dm, DM9051_IMR, IMR_PAR); /* disable imr */
> +		if (unlikely(result))
> +			goto spi_err;
> +
> +		do {
> +			result = dm9051_loop_rx(db); /* threaded irq rx */
> +			if (result < 0)
> +				goto spi_err;
> +			resul_tx = dm9051_loop_tx(db); /* more tx better performance */
> +			if (resul_tx < 0)

result_tx
     ^
> +				goto spi_err;
> +		} while (result > 0);
> +
> +		result = regmap_write(db->regmap_dm, DM9051_IMR, db->imr_all); /* enable imr */
> +		if (unlikely(result))
> +			goto spi_err;
> +	}
> +spi_err:
> +	mutex_unlock(&db->spi_lockm); /* mutex essential */
> +	return IRQ_HANDLED;
> +}


> +static int dm9051_map_phyup(struct board_info *db)
> +{
> +	int ret;
> +
> +	/* ~BMCR_PDOWN to power-up phyxcer
> +	 */
> +	ret = mdiobus_modify(db->mdiobus, DM9051_PHY_ID, MII_BMCR, BMCR_PDOWN, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* chip internal operation need wait 1 ms for if GPR power-up phy
> +	 */
> +	ret = regmap_write(db->regmap_dm, DM9051_GPR, 0);
> +	if (unlikely(ret))
> +		return ret;
> +	mdelay(1);

The phy driver should do this. Again, what PHY driver are you using?

> +static int dm9051_map_phydown(struct board_info *db)
> +{
> +	int ret;
> +
> +	ret = regmap_write(db->regmap_dm, DM9051_GPR, GPR_PHY_ON); /* Power-Down PHY */
> +	if (unlikely(ret))
> +		return ret;
> +	return ret;
> +}

Cam you still access the PHY after this? Does it loose its
configuration?

> +	/* We may have start with auto negotiation */
> +	db->phydev->autoneg = AUTONEG_ENABLE;
> +	db->phydev->speed = 0;
> +	db->phydev->duplex = 0;

If you have to touch these, something is wrong. Please explain.

   Andrew