| 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
| ||
|
Message-ID: <885eec03-b4d0-4bd1-869f-c334bb22888c@lunn.ch>
Date: Thu, 11 Jul 2024 21:01:56 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Kamil Horák (2N) <kamilh@...s.com>
Cc: florian.fainelli@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 4/4] net: phy: bcm-phy-lib: Implement BroadR-Reach
link modes
> +static int bcm5481x_get_brrmode(struct phy_device *phydev, u8 *data)
> {
> - int err, reg;
> + int reg;
>
> - /* Disable BroadR-Reach function. */
> reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
> - reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
> - err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL,
> - reg);
> - if (err < 0)
bcm_phy_read_exp() could fail. So you should keep the test. Also, the
caller of this function does look at the return value.
> +/**
> + * bcm5481x_read_abilities - read PHY abilities from LRESR or Clause 22
> + * (BMSR) registers, based on whether the PHY is in BroadR-Reach or IEEE mode
> + * @phydev: target phy_device struct
> + *
> + * Description: Reads the PHY's abilities and populates
> + * phydev->supported accordingly. The register to read the abilities from is
> + * determined by current brr mode setting of the PHY.
> + * Note that the LRE and IEEE sets of abilities are disjunct.
> + *
> + * Returns: 0 on success, < 0 on failure
> + */
> +static int bcm5481x_read_abilities(struct phy_device *phydev)
> +{
> + int i, val, err;
> + u8 brr_mode;
> +
> + for (i = 0; i < ARRAY_SIZE(bcm54811_linkmodes); i++)
> + linkmode_clear_bit(bcm54811_linkmodes[i], phydev->supported);
> +
> + err = bcm5481x_get_brrmode(phydev, &brr_mode);
> +static int bcm5481x_set_brrmode(struct phy_device *phydev, bool on)
> +{
> + int reg;
> + int err;
> + u16 val;
> +
> + reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
> +
> + if (on)
> + reg |= BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
> + else
> + reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
> +
> +static int bcm54811_config_init(struct phy_device *phydev)
> +{
> + struct device_node *np = phydev->mdio.dev.of_node;
> + bool brr = false;
> + int err, reg;
> +
> err = bcm54xx_config_init(phydev);
>
> /* Enable CLK125 MUX on LED4 if ref clock is enabled. */
> @@ -576,29 +687,80 @@ static int bcm54811_config_init(struct phy_device *phydev)
> return err;
> }
>
> - return err;
> + /* Configure BroadR-Reach function. */
> + brr = of_property_read_bool(np, "brr-mode");
> +
> + /* With BCM54811, BroadR-Reach implies no autoneg */
> + if (brr)
> + phydev->autoneg = 0;
> +
> + return bcm5481x_set_brrmode(phydev, brr);
> }
The ordering seems a bit strange here.
phy_probe() will call phydrv->get_features. At this point, the PHY is
in whatever mode it resets to, or maybe what it is strapped
to. phydev->supported could thus be set to standard IEEE modes,
despite the board design is actually for BroadR-Reach.
Sometime later, when the MAC is connected to the PHY config_init() is
called. At that point, you poke around in DT and find how the PHY is
connected to the cable. At that point, you set the PHY mode, and
change phydev->supported to reflect reality.
I really think that reading DT should be done much earlier, maybe in
the driver probe function, or maybe get_features. get_features should
always return the correct values from the board.
> +static int bcm5481_config_aneg(struct phy_device *phydev)
> +{
> + u8 brr_mode;
> + int ret;
> +
> + ret = bcm5481x_get_brrmode(phydev, &brr_mode);
Rather than read it from the hardware every single time, could you
store the DT value in bcm54xx_phy_priv ?
> +/* Read LDS Link Partner Ability in BroadR-Reach mode */
> +static int bcm_read_lpa(struct phy_device *phydev)
This function seems to be missing an lds or lre prefix.
> +static int bcm_read_status_fixed(struct phy_device *phydev)
and here. Please make sure the naming is consistent. Anything which
only accesses lre or lds registers should make that clear in its name.
> +static int bcm54811_read_status(struct phy_device *phydev)
> +{
> + u8 brr_mode;
> + int err;
> +
> + err = bcm5481x_get_brrmode(phydev, &brr_mode);
> +
> + if (err)
> + return err;
> +
> + if (brr_mode) {
> + /* Get the status in BroadRReach mode just like
> + * genphy_read_status does in normal mode
> + */
> +
> + int err, old_link = phydev->link;
> +
> + /* Update the link, but return if there was an error */
> +
> + err = lre_update_link(phydev);
> + if (err)
> + return err;
> +
> + /* why bother the PHY if nothing can have changed */
> + if (phydev->autoneg ==
> + AUTONEG_ENABLE && old_link && phydev->link)
> + return 0;
> +
> + phydev->speed = SPEED_UNKNOWN;
> + phydev->duplex = DUPLEX_UNKNOWN;
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> +
> + err = bcm_read_master_slave(phydev);
> + if (err < 0)
> + return err;
> +
> + /* Read LDS Link Partner Ability */
> + err = bcm_read_lpa(phydev);
> + if (err < 0)
> + return err;
> +
> + if (phydev->autoneg ==
> + AUTONEG_ENABLE && phydev->autoneg_complete) {
> + phy_resolve_aneg_linkmode(phydev);
> + } else if (phydev->autoneg == AUTONEG_DISABLE) {
> + err = bcm_read_status_fixed(phydev);
> + if (err < 0)
> + return err;
> + }
This would probably look better if you pulled this code out into a
helper bcm54811_lre_read_status().
Andrew
---
pw-bot: cr
Powered by blists - more mailing lists