| 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
| ||
|
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