| 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: <20250822065817.GA4907@legfed1>
Date: Fri, 22 Aug 2025 08:58:17 +0200
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Ilya Evenbach <ievenbach@...ora.tech>, Jakub Kicinski <kuba@...nel.org>,
Andrew Lunn <andrew@...n.ch>,
Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
Gregor Herburger <gregor.herburger@...tq-group.com>,
Stefan Eichenberger <eichest@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH] [88q2xxx] Add support for handling master/slave in
forced mode
Hi Ilya,
Am Thu, Aug 21, 2025 at 01:53:04PM -0700 schrieb Ilya Evenbach:
> I am using 88Q2221, This PHY has a bug - MDIO_PMA_EXTABLE is not properly
> implemented (reads as 0x20).
> As a result, genphy_c45_baset1_able() returns false, and master/slave is
> not set up or read properly in forced mode.
That is not 88Q2221 specific, please have a look at function
mv88q2xxx_config_init which solves this issue:
/* The 88Q2XXX PHYs do have the extended ability register available, but
* register MDIO_PMA_EXTABLE where they should signalize it does not
* work according to specification. Therefore, we force it here.
*/
phydev->pma_extable = MDIO_PMA_EXTABLE_BT1;
> I will try to clean up this patch to better utilize generic functions.
>
What you are trying to do is adding support for 88Q2221 which has the
same phy_id as the 88Q2220 but needs a different(additional) setup to
work properly(init sequence, ...).
Please change your commit message then.
> On Thu, Aug 21, 2025 at 1:02 AM Dimitri Fedrau <dima.fedrau@...il.com>
> wrote:
> >
> > 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