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

Powered by blists - more mailing lists