linux-kernel - Re: [PATCH v16, 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]
Date:   Thu, 3 Feb 2022 01:36:26 +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@...nel.org
Subject: Re: [PATCH v16, 2/2] net: Add dm9051 driver

> +static int dm9051_update_fcr(struct board_info *db)
> +{
> +	u8 fcr = 0;
> +	int ret;
> +
> +	if (db->pause.rx_pause)
> +		fcr |= FCR_BKPM | FCR_FLCE;
> +	if (db->pause.tx_pause)
> +		fcr |= FCR_TXPEN;
> +
> +	ret = regmap_update_bits(db->regmap_dm, DM9051_FCR, 0xff, fcr);

Is 0xff correct here? You only seem interested in FCR_BKPM, FCR_FLCE,
FCR_TXPEN so i would of expected a value based around those.

> +	if (ret)
> +		netif_err(db, drv, db->ndev, "%s: error %d update bits reg %02x\n",
> +			  __func__, ret, DM9051_FCR);
> +	return ret;
> +}
> +
> +static int dm9051_set_fcr(struct board_info *db)
> +{
> +	u8 fcr = 0;
> +	int ret;
> +
> +	if (db->pause.rx_pause)
> +		fcr |= FCR_BKPM | FCR_FLCE;
> +	if (db->pause.tx_pause)
> +		fcr |= FCR_TXPEN;
> +
> +	ret = regmap_write(db->regmap_dm, DM9051_FCR, fcr);
> +	if (ret)
> +		netif_err(db, drv, db->ndev, "%s: error %d write reg %02x\n",
> +			  __func__, ret, DM9051_FCR);
> +	return ret;

I guess you can combine this code somehow, make one call the other?

> +static int dm9051_mdio_register(struct board_info *db)
> +{
> +	struct spi_device *spi = db->spidev;
> +	int ret;
> +
> +	db->mdiobus = devm_mdiobus_alloc(&spi->dev);
> +	if (!db->mdiobus)
> +		return -ENOMEM;
> +
> +	db->mdiobus->priv = db;
> +	db->mdiobus->read = dm9051_mdiobus_read;
> +	db->mdiobus->write = dm9051_mdiobus_write;
> +	db->mdiobus->name = "dm9051-mdiobus";
> +	db->mdiobus->phy_mask = (u32)~GENMASK(1, 1);
> +	db->mdiobus->parent = &spi->dev;
> +	snprintf(db->mdiobus->id, MII_BUS_ID_SIZE,
> +		 "dm9051-%s.%u", dev_name(&spi->dev), spi->chip_select);
> +
> +	ret = devm_mdiobus_register(&spi->dev, db->mdiobus);
> +	if (ret)
> +		dev_err(&spi->dev, "Could not register MDIO bus\n");
> +
> +	return 0;

You should return ret here, since an error might of occurred.

> +static void dm9051_handle_link_change(struct net_device *ndev)
> +{
> +	struct board_info *db = to_dm9051_board(ndev);
> +	int lcl_adv, rmt_adv;
> +
> +	phy_print_status(db->phydev);
> +
> +	/* only write pause settings to mac. since mac and phy are integrated
> +	 * together, such as link state, speed and duplex are sync already
> +	 */
> +	if (db->phydev->link) {
> +		if (db->pause.autoneg == AUTONEG_ENABLE) {
> +			lcl_adv = linkmode_adv_to_mii_adv_t(db->phydev->advertising);
> +			rmt_adv = linkmode_adv_to_mii_adv_t(db->phydev->lp_advertising);
> +
> +			if (lcl_adv & rmt_adv & ADVERTISE_PAUSE_CAP) {
> +				db->pause.rx_pause = true;
> +				db->pause.tx_pause = true;
> +			}

Please look at phydev->pause. It gives you the resolved value, you
don't need to work it out for yourself.  phydev->asym_pause tells you
about asymmetric pause, but you hardware does not support that, so you
don't need it.


> +static int dm9051_set_pauseparam(struct net_device *ndev,
> +				 struct ethtool_pauseparam *pause)
> +{
> +	struct board_info *db = to_dm9051_board(ndev);
> +
> +	db->pause = *pause;
> +
> +	if (pause->autoneg == AUTONEG_DISABLE) {
> +		db->phydev->autoneg = AUTONEG_DISABLE;

As i said before, ksetting is used to change this, not pause. Please
don't set phydev->autoneg like this.

> +		return dm9051_update_fcr(db);
> +	}
> +
> +	db->phydev->autoneg = AUTONEG_ENABLE;

Nor here.

> +	phy_set_sym_pause(db->phydev, pause->rx_pause, pause->tx_pause,
> +			  pause->autoneg);
> +	phy_start_aneg(db->phydev);
> +	return 0;
> +}

> +static irqreturn_t dm9051_rx_threaded_irq(int irq, void *pw)
> +{
> +	struct board_info *db = pw;
> +	int result, result_tx;
> +
> +	mutex_lock(&db->spi_lockm);
> +	if (netif_carrier_ok(db->ndev)) {

Why is carrier relevant here? Maybe the device is trying to give you
the last packets before the carrier went down?

It is also interesting that you don't look at the interrupt service
register. Often you need to clear the interrupt by reading the
interrupt service register.

> +		result = regmap_write(db->regmap_dm, DM9051_IMR, IMR_PAR); /* disable int */
> +		if (result)
> +			goto spi_err;
> +
> +		do {
> +			result = dm9051_loop_rx(db); /* threaded irq rx */
> +			if (result < 0)
> +				goto spi_err;
> +			result_tx = dm9051_loop_tx(db); /* more tx better performance */
> +			if (result_tx < 0)
> +				goto spi_err;
> +		} while (result > 0);
> +
> +		result = regmap_write(db->regmap_dm, DM9051_IMR, db->imr_all); /* enable int */
> +		if (result)
> +			goto spi_err;
> +	}
> +spi_err:
> +	mutex_unlock(&db->spi_lockm);
> +
> +	return IRQ_HANDLED;
> +}
> +

  Andrew