| 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: <20250821080205.GA5542@legfed1>
Date: Thu, 21 Aug 2025 10:02:05 +0200
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: "Ilya A. Evenbach" <ievenbach@...ora.tech>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH] [88q2xxx] Add support for handling master/slave in
forced mode
Hi Ilya,
Am Wed, Aug 20, 2025 at 11:11:43AM -0700 schrieb Ilya A. Evenbach:
> 88q2xxx PHYs have non-standard way of setting master/slave in
> forced mode.
> This change adds support for changing and reporting this setting
> correctly through ethtool.
>
> Signed-off-by: Ilya A. Evenbach <ievenbach@...ora.tech>
> ---
> drivers/net/phy/marvell-88q2xxx.c | 106 ++++++++++++++++++++++++++++--
> 1 file changed, 101 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index f3d83b04c953..b94d574fd9b7 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -118,6 +118,11 @@
> #define MV88Q2XXX_LED_INDEX_TX_ENABLE 0
> #define MV88Q2XXX_LED_INDEX_GPIO 1
>
> +/* Marvell vendor PMA/PMD control for forced master/slave when AN is disabled */
> +#define PMAPMD_MVL_PMAPMD_CTL 0x0834
Already defined, see MDIO_PMA_PMD_BT1_CTRL.
> +#define MASTER_MODE BIT(14)
Already defines, see MDIO_PMA_PMD_BT1_CTRL_CFG_MST.
> +#define MODE_MASK BIT(14)
> +
> struct mv88q2xxx_priv {
> bool enable_led0;
> };
> @@ -377,13 +382,57 @@ static int mv88q2xxx_read_link(struct phy_device *phydev)
> static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
> {
> int ret;
> + int adv_l, adv_m, stat, stat2;
> +
> + /* In forced mode, state and config are controlled via PMAPMD 0x834 */
> + if (phydev->autoneg == AUTONEG_DISABLE) {
> + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_MVL_PMAPMD_CTL);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & MASTER_MODE) {
> + phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
> + phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
> + } else {
> + phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
> + phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
> + }
> + return 0;
> + }
>
> - phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> - ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
> - if (ret < 0)
> - return ret;
>
> - if (ret & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
> + adv_l = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L);
> + if (adv_l < 0)
> + return adv_l;
> + adv_m = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M);
> + if (adv_m < 0)
> + return adv_m;
> +
> + if (adv_l & MDIO_AN_T1_ADV_L_FORCE_MS)
> + phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
> + else if (adv_m & MDIO_AN_T1_ADV_M_MST)
> + phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_PREFERRED;
> + else
> + phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_PREFERRED;
> +
> + stat = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT);
> + if (stat < 0)
> + return stat;
> +
> + if (stat & MDIO_MMD_AN_MV_STAT_MS_CONF_FAULT) {
> + phydev->master_slave_state = MASTER_SLAVE_STATE_ERR;
> + return 0;
> + }
> +
> + stat2 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STAT2);
> + if (stat2 < 0)
> + return stat2;
> + if (!(stat2 & MDIO_MMD_AN_MV_STAT2_AN_RESOLVED)) {
> + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> + return 0;
> + }
> +
> + if (stat & MDIO_MMD_AN_MV_STAT_LOCAL_MASTER)
> phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
> else
> phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
> @@ -391,6 +440,34 @@ static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
> return 0;
> }
>
Is there a issue with the function you are trying to fix ? Seems that
you copied some generic functions into it.
> +static int mv88q2xxx_setup_master_slave_forced(struct phy_device *phydev)
> +{
> + int ret = 0;
> +
> + switch (phydev->master_slave_set) {
> + case MASTER_SLAVE_CFG_MASTER_FORCE:
> + case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> + ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
> + PMAPMD_MVL_PMAPMD_CTL,
> + MODE_MASK, MASTER_MODE);
> + break;
> + case MASTER_SLAVE_CFG_SLAVE_FORCE:
> + case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> + ret = phy_modify_mmd_changed(phydev, MDIO_MMD_PMAPMD,
> + PMAPMD_MVL_PMAPMD_CTL,
> + MODE_MASK, 0);
> + break;
> + case MASTER_SLAVE_CFG_UNKNOWN:
> + case MASTER_SLAVE_CFG_UNSUPPORTED:
> + default:
> + phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> + ret = 0;
> + break;
> + }
> +
> + return ret;
> +}
> +
This function does the same as genphy_c45_pma_baset1_setup_master_slave.
Please use the generic function. Besides you are introducing register
PMAPMD_MVL_PMAPMD_CTL which is MDIO_PMA_PMD_BT1_CTRL.
> static int mv88q2xxx_read_aneg_speed(struct phy_device *phydev)
> {
> int ret;
> @@ -448,6 +525,11 @@ static int mv88q2xxx_read_status(struct phy_device *phydev)
> if (ret < 0)
> return ret;
>
> + /* Populate master/slave status also for forced modes */
> + ret = mv88q2xxx_read_master_slave_state(phydev);
> + if (ret < 0 && ret != -EOPNOTSUPP)
> + return ret;
> +
> return genphy_c45_read_pma(phydev);
> }
>
Why ? This function is only used in case AUTONEG_ENABLE.
> @@ -478,6 +560,20 @@ static int mv88q2xxx_config_aneg(struct phy_device *phydev)
> if (ret)
> return ret;
>
> + /* Configure Base-T1 master/slave per phydev->master_slave_set.
> + * For AN disabled, program PMAPMD role directly; otherwise rely on
> + * the standard Base-T1 AN advertisement bits.
> + */
> + if (phydev->autoneg == AUTONEG_DISABLE) {
> + ret = mv88q2xxx_setup_master_slave_forced(phydev);
> + if (ret)
> + return ret;
> + } else {
> + ret = genphy_c45_pma_baset1_setup_master_slave(phydev);
> + if (ret)
> + return ret;
> + }
> +
> return phydev->drv->soft_reset(phydev);
> }
>
I don't see any reason why genphy_c45_config_aneg isn't sufficient here.
In case AUTONEG_DISABLE, genphy_c45_pma_setup_forced is called and calls
genphy_c45_pma_baset1_setup_master_slave which is basically the same as
mv88q2xxx_setup_master_slave_forced.
In case AUTONEG_ENABLE, calling genphy_c45_pma_baset1_setup_master_slave is
wrong, please look how genphy_c45_an_config_aneg is implemented.
Please take other users of the driver into CC, they did a lot of
reviewing and testing in the past. If there is some issue with the
driver, they should know:
"Niklas Söderlund" <niklas.soderlund+renesas@...natech.se>
"Gregor Herburger" <gregor.herburger@...tq-group.com>
"Stefan Eichenberger" <eichest@...il.com>
"Geert Uytterhoeven" <geert@...ux-m68k.org>
Which device are you using, and why did you need this patch ? Is there
any issue you are trying to fix ? On my side I did a lot of testing with
the different modes and never experienced any problems so far.
Best regards,
Dimitri Fedrau
Powered by blists - more mailing lists