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: <c435e879-33c2-4cee-a2b1-56e82a0e9281@wanadoo.fr>
Date: Mon, 6 May 2024 22:14:20 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Kamil Horák - 2N <kamilh@...s.com>,
 florian.fainelli@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
 andrew@...n.ch, hkallweit1@...il.com
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach link
 modes

Le 06/05/2024 à 16:40, Kamil Horák - 2N a écrit :
> Implement single-pair BroadR-Reach modes on bcm5481x PHY by Broadcom.
> Create set of functions alternative to IEEE 802.3 to handle configuration
> of these modes on compatible Broadcom PHYs.
> 
> Signed-off-by: Kamil Horák - 2N <kamilh@...s.com>
> ---
>   drivers/net/phy/bcm-phy-lib.c | 122 ++++++++++++
>   drivers/net/phy/bcm-phy-lib.h |   4 +
>   drivers/net/phy/broadcom.c    | 338 ++++++++++++++++++++++++++++++++--
>   3 files changed, 449 insertions(+), 15 deletions(-)

Hi,

a few nitpicks below, should it help.

...

> +int bcm_config_aneg(struct phy_device *phydev, bool changed)
> +{
> +	int err;
> +
> +	if (genphy_config_eee_advert(phydev))
> +		changed = true;
> +
> +	err = bcm_setup_master_slave(phydev);
> +	if (err < 0)
> +		return err;
> +	else if (err)
> +		changed = true;
> +
> +	if (phydev->autoneg != AUTONEG_ENABLE)
> +		return bcm_setup_forced(phydev);
> +
> +	err = bcm_config_advert(phydev);
> +	if (err < 0) /* error */

Nitpick: the comment could be removed, IMHO (otherwise maybe it should 
be added a few lines above too)

> +		return err;
> +	else if (err)
> +		changed = true;
> +
> +	return genphy_check_and_restart_aneg(phydev, changed);
> +}
> +EXPORT_SYMBOL_GPL(bcm_config_aneg);
> +
> +/**
> + * bcm_config_advert - sanitize and advertise auto-negotiation parameters
> + * @phydev: target phy_device struct
> + *
> + * Description: Writes MII_BCM54XX_LREANAA with the appropriate values,
> + *   after sanitizing the values to make sure we only advertise
> + *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
> + *   hasn't changed, and > 0 if it has changed.
> + */
> +int bcm_config_advert(struct phy_device *phydev)
> +{
> +	int err;
> +	u32 adv;
> +
> +	/* Only allow advertising what this PHY supports */
> +	linkmode_and(phydev->advertising, phydev->advertising,
> +		     phydev->supported);
> +
> +	adv = bcm_linkmode_adv_to_mii_adv_t(phydev->advertising);
> +
> +	/* Setup BroadR-Reach mode advertisement */
> +	err = phy_modify_changed(phydev, MII_BCM54XX_LREANAA,
> +				 LRE_ADVERTISE_ALL | LREANAA_PAUSE |
> +				 LREANAA_PAUSE_ASYM, adv);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return err > 0 ? 1 : 0;

Nitpick: Could be: return err;
(at this point it can be only 0 or 1 anyway)

> +}
> +EXPORT_SYMBOL_GPL(bcm_config_advert);

...

> @@ -576,18 +604,16 @@ static int bcm54811_config_init(struct phy_device *phydev)
>   			return err;
>   	}
>   
> -	return err;
> +	/* Configure BroadR-Reach function. */
> +	return  bcm5481x_set_brrmode(phydev, ETHTOOL_PHY_BRR_MODE_OFF);

Nitpick: 2 spaces before "bcm5481x_set_brrmode"

>   }
>   
> -static int bcm5481_config_aneg(struct phy_device *phydev)
> +static int bcm5481x_config_delay_swap(struct phy_device *phydev)
>   {
>   	struct device_node *np = phydev->mdio.dev.of_node;
> -	int ret;
> -
> -	/* Aneg firstly. */
> -	ret = genphy_config_aneg(phydev);
> +	int ret = 0;

I think that ret can be left un-initialized and use return 0; at the end 
of the function.

>   
> -	/* Then we can set up the delay. */
> +	/* Set up the delay. */
>   	bcm54xx_config_clock_delay(phydev);
>   
>   	if (of_property_read_bool(np, "enet-phy-lane-swap")) {
> @@ -601,6 +627,56 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
>   	return ret;
>   }

...

> +static int bcm54811_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +	u8 brr_mode;
> +
> +	/* Aneg firstly. */
> +	ret = bcm5481x_get_brrmode(phydev, &brr_mode);
> +	if (ret)
> +		return ret;
> +
> +	if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
> +		/* BCM54811 is only capable of autonegotiation in IEEE mode */
> +		if (phydev->autoneg)
> +			return -EOPNOTSUPP;
> +
> +		ret = bcm_config_aneg(phydev, false);
> +

Nitpick: unneeded new line

> +	} else {
> +		ret = genphy_config_aneg(phydev);
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Then we can set up the delay and swap. */
> +	return bcm5481x_config_delay_swap(phydev);
> +}
> +
>   struct bcm54616s_phy_priv {
>   	bool mode_1000bx_en;
>   };
> @@ -1062,6 +1138,234 @@ static void bcm54xx_link_change_notify(struct phy_device *phydev)
>   	bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_EXP08, ret);
>   }
>   
> +static int bcm54811_read_abilities(struct phy_device *phydev)
> +{
> +	int val, err;
> +	int i;
> +	static const int modes_array[] = {ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> +					  ETHTOOL_LINK_MODE_1BR10_BIT,
> +					  ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +					  ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +					  ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> +					  ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> +					  ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> +					  ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> +					  ETHTOOL_LINK_MODE_10baseT_Half_BIT};
> +

Nitpick: unneeded new line. Or maybe brr_mode could be defined above 
this struct?

Should there be an extra space after { and before }?
(see br_bits below)

> +	u8 brr_mode;
> +
> +	for (i = 0; i < ARRAY_SIZE(modes_array); i++)
> +		linkmode_clear_bit(modes_array[i], phydev->supported);
> +
> +	err = bcm5481x_get_brrmode(phydev, &brr_mode);
> +

Nitpick: unneeded new line

> +	if (err)
> +		return err;
> +
> +	if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
> +		linkmode_set_bit_array(phy_basic_ports_array,
> +				       ARRAY_SIZE(phy_basic_ports_array),
> +				       phydev->supported);
> +
> +		val = phy_read(phydev, MII_BCM54XX_LRESR);
> +		if (val < 0)
> +			return val;
> +
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +				 phydev->supported, 1);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> +				 phydev->supported,
> +				 val & LRESR_100_1PAIR);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_1BR10_BIT,
> +				 phydev->supported,
> +				 val & LRESR_10_1PAIR);
> +	} else {
> +		return genphy_read_abilities(phydev);
> +	}
> +
> +	return err;

Nitpick: return 0; ?

> +}

...

> +/* Read LDS Link Partner Ability in BroadR-Reach mode */
> +static int bcm_read_lpa(struct phy_device *phydev)
> +{
> +	int i, lrelpa;
> +
> +	if (phydev->autoneg != AUTONEG_ENABLE) {
> +		if (!phydev->autoneg_complete) {
> +			/* aneg not yet done, reset all relevant bits */
> +			static int br_bits[] = { ETHTOOL_LINK_MODE_Autoneg_BIT,

Nitpick: add const? (but maybe the line would get too long)

> +						 ETHTOOL_LINK_MODE_Pause_BIT,
> +						 ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +						 ETHTOOL_LINK_MODE_1BR10_BIT,
> +						 ETHTOOL_LINK_MODE_100baseT1_Full_BIT };
> +			for (i = 0; i < ARRAY_SIZE(br_bits); i++)
> +				linkmode_clear_bit(br_bits[i], phydev->lp_advertising);
> +
> +			return 0;
> +		}
> +
> +		/* Long-Distance-Signalling Link Partner Ability */

Typo? Signaling?

> +		lrelpa = phy_read(phydev, MII_BCM54XX_LRELPA);
> +		if (lrelpa < 0)
> +			return lrelpa;
> +
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_PAUSE_ASYM);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_PAUSE);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_100_1PAIR);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_1BR10_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_10_1PAIR);
> +	} else {
> +		linkmode_zero(phydev->lp_advertising);
> +	}
> +
> +	return 0;
> +}

...

CJ


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ