Re: [PATCH v2 1/3] net/macb: add new driver

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue,  5 Nov 2024 08:41:38 +0000
liwencheng <liwencheng@phytium.com.cn> wrote:

> +
> +int genphy_read_status(struct phy_device *phydev)
> +{
> +	struct macb *bp = phydev->bp;
> +	uint16_t bmcr, bmsr, ctrl1000 = 0, stat1000 = 0;
> +	uint32_t advertising, lp_advertising;
> +	uint32_t nego;
> +	uint16_t phyad = phydev->phyad;
> +
> +	/* Do a fake read */
> +	bmsr = macb_mdio_read(bp, phyad, GENERIC_PHY_BMSR);
> +
> +	bmsr = macb_mdio_read(bp, phyad, GENERIC_PHY_BMSR);
> +	bmcr = macb_mdio_read(bp, phyad, GENERIC_PHY_BMCR);
> +
> +	if (bmcr & BMCR_ANENABLE) {
> +		ctrl1000 = macb_mdio_read(bp, phyad, GENERIC_PHY_CTRL1000);
> +		stat1000 = macb_mdio_read(bp, phyad, GENERIC_PHY_STAT1000);
> +
> +		advertising = ADVERTISED_Autoneg;
> +		advertising |= genphy_get_an(bp, phyad, GENERIC_PHY_ADVERISE);
> +		advertising |= genphy_ctrl1000_to_ethtool_adv_t(ctrl1000);
> +
> +		if (bmsr & BMSR_ANEGCOMPLETE) {
> +			lp_advertising = genphy_get_an(bp, phyad, GENERIC_PHY_LPA);
> +			lp_advertising |= genphy_stat1000_to_ethtool_lpa_t(stat1000);
> +		} else {
> +			lp_advertising = 0;
> +		}
> +
> +		nego = advertising & lp_advertising;
> +		if (nego & (ADVERTISED_1000baseT_Full | ADVERTISED_1000baseT_Half)) {
> +			phydev->speed = SPEED_1000;
> +			phydev->duplex = !!(nego & ADVERTISED_1000baseT_Full);
> +		} else if (nego &
> +				(ADVERTISED_100baseT_Full | ADVERTISED_100baseT_Half)) {
> +			phydev->speed = SPEED_100;
> +			phydev->duplex = !!(nego & ADVERTISED_100baseT_Full);
> +		} else {
> +			phydev->speed = SPEED_10;
> +			phydev->duplex = !!(nego & ADVERTISED_10baseT_Full);
> +		}
> +	} else {
> +		phydev->speed = ((bmcr & BMCR_SPEED1000 && (bmcr & BMCR_SPEED100) == 0)
> +						 ? SPEED_1000
> +						 : ((bmcr & BMCR_SPEED100) ? SPEED_100 : SPEED_10));
> +		phydev->duplex = (bmcr & BMCR_FULLDPLX) ? DUPLEX_FULL : DUPLEX_HALF;
> +	}
> +
> +	return 0;
> +}

Always returns 0 can be void function?

> +int macb_usxgmii_pcs_resume(struct phy_device *phydev)
> +{
> +	u32 config;
> +	struct macb *bp = phydev->bp;
> +
> +	config = gem_readl(bp, USX_CONTROL);
> +
> +	/* enable signal */
> +	config &= ~(GEM_BIT(RX_SYNC_RESET));
> +	config |= GEM_BIT(SIGNAL_OK) | GEM_BIT(TX_EN);
> +	gem_writel(bp, USX_CONTROL, config);
> +
> +	return 0;
> +}

Always returns 0 can be void function?

> +int macb_usxgmii_pcs_suspend(struct phy_device *phydev)
> +{
> +	uint32_t config;
> +	struct macb *bp = phydev->bp;
> +
> +	config = gem_readl(bp, USX_CONTROL);
> +	config |= GEM_BIT(RX_SYNC_RESET);
> +	/* disable signal */
> +	config &= ~(GEM_BIT(SIGNAL_OK) | GEM_BIT(TX_EN));
> +	gem_writel(bp, USX_CONTROL, config);
> +	rte_delay_ms(1);
> +	return 0;
> +}

Always returns 0 should be void?

> +
> +int macb_usxgmii_pcs_check_for_link(struct phy_device *phydev)
> +{
> +	int value;
> +	int link;
> +	struct macb *bp = phydev->bp;
> +	value = gem_readl(bp, USX_STATUS);
> +	link = GEM_BFEXT(BLOCK_LOCK, value);
> +	return link;
> +}

The driver is sloppy in using int where unsigned value is possible.
You lose precision doing that and are prone to sign extension bugs.

Since gem_readl() is wrapper around macb_reg_readl() and that returns u32;
this function should be returning u32 and value should be u32
The temporary variable value is not needed.

> +int macb_gbe_pcs_check_for_link(struct phy_device *phydev)
> +{
> +	int value;
> +	int link;
> +	struct macb *bp = phydev->bp;
> +
> +	value = macb_readl(bp, NSR);
> +	link = MACB_BFEXT(NSR_LINK, value);
> +	return link;
> +}