| 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: Tue, 1 Jun 2021 15:53:36 +0000
From: Liang Xu <lxu@...linear.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "hkallweit1@...il.com" <hkallweit1@...il.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"linux@...linux.org.uk" <linux@...linux.org.uk>,
Hauke Mehrtens <hmehrtens@...linear.com>,
Thomas Mohren <tmohren@...linear.com>
Subject: Re: [PATCH] phy: maxlinear: add Maxlinear GPY115/21x/24x driver
On 1/6/2021 8:41 pm, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
> On Tue, Jun 01, 2021 at 03:44:27PM +0800, Xu Liang wrote:
>> ---
>> drivers/net/phy/Kconfig | 5 +
>> drivers/net/phy/Makefile | 1 +
>> drivers/net/phy/mxl-gpy.c | 537 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 543 insertions(+)
>> create mode 100644 drivers/net/phy/mxl-gpy.c
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 288bf405ebdb..7f1a0d62d83a 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -186,6 +186,11 @@ config INTEL_XWAY_PHY
>> PEF 7061, PEF 7071 and PEF 7072 or integrated into the Intel
>> SoCs xRX200, xRX300, xRX330, xRX350 and xRX550.
>>
>> +config MXL_GPHY
>> + tristate "Maxlinear 2.5G PHYs"
>> + help
>> + Support for the Maxlinear 2.5G PHYs.
>> +
> This file is sorted based on the tristate string. So this entry should
> come after the "Marvell 88X2222 PHY"
Sorry for the mistake, I will fix in v2 patch.
>
>> config LSI_ET1011C_PHY
>> tristate "LSI ET1011C PHY"
>> help
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index bcda7ed2455d..28aa2a198d00 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -59,6 +59,7 @@ obj-$(CONFIG_DP83TC811_PHY) += dp83tc811.o
>> obj-$(CONFIG_FIXED_PHY) += fixed_phy.o
>> obj-$(CONFIG_ICPLUS_PHY) += icplus.o
>> obj-$(CONFIG_INTEL_XWAY_PHY) += intel-xway.o
>> +obj-$(CONFIG_MXL_GPHY) += mxl-gpy.o
>> obj-$(CONFIG_LSI_ET1011C_PHY) += et1011c.o
>> obj-$(CONFIG_LXT_PHY) += lxt.o
>> obj-$(CONFIG_MARVELL_10G_PHY) += marvell10g.o
> And this file is sorted on the CONFIG_XXX value.
I will fix in v2 patch.
>> +static int gpy_read_abilities(struct phy_device *phydev)
>> +{
>> + int ret;
>> +
>> + ret = genphy_read_abilities(phydev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Detect 2.5G/5G support. */
>> + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
>> + if (ret < 0)
>> + return ret;
>> + if (!(ret & MDIO_PMA_STAT2_EXTABLE))
>> + return 0;
>> +
>> + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
>> + if (ret < 0)
>> + return ret;
>> + if (!(ret & MDIO_PMA_EXTABLE_NBT))
>> + return 0;
>> +
>> + ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
>> + if (ret < 0)
>> + return ret;
>> +
>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>> + phydev->supported,
>> + ret & MDIO_PMA_NG_EXTABLE_2_5GBT);
>> +
>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>> + phydev->supported,
>> + ret & MDIO_PMA_NG_EXTABLE_5GBT);
>> +
> Does genphy_c45_pma_read_abilities() do the wrong thing here? What
> does it get wrong?
The problem comes from condition "phydev->c45_ids.mmds_present &
MDIO_DEVS_AN".
Our product supports both C22 and C45.
In the real system, we found C22 was used by customers (with indirect
access to C45 registers when necessary).
Then during probe, in API "get_phy_device", it skips reading C45 IDs.
So that genphy_c45_pma_read_abilities skip the supported flag
ETHTOOL_LINK_MODE_Autoneg_BIT.
>> + return 0;
>> +}
>> +
>> +static int gpy_config_init(struct phy_device *phydev)
>> +{
>> + int ret, fw_ver;
>> +
>> + /* Show GPY PHY FW version in dmesg */
>> + fw_ver = phy_read(phydev, PHY_FWV);
>> + if (fw_ver < 0)
>> + return fw_ver;
>> +
>> + phydev_info(phydev, "Firmware Version: 0x%04X (%s)\n", fw_ver,
>> + (fw_ver & BIT(15)) ? "release" : "test");
> You define PHY_FWV_TYPE_MASK and PHY_FWV_MINOR_MASK. Does it make
> sense to break this up a bit more?
Will fix in v2 patch.
>
>> +static bool gpy_sgmii_need_reaneg(struct phy_device *phydev)
>> +{
>> + struct {
>> + int type;
>> + int minor;
>> + } table[] = {
>> + {7, 0x6D},
>> + {8, 0x6D},
>> + {9, 0x73},
>> + };
> Please make this const. And to fix reverse christmas tree, you might
> want to move it out of the function.
Will fix in v2 patch.
>> +
>> + int fw_ver, fw_type, fw_minor;
>> + size_t i;
>> +
>> + fw_ver = phy_read(phydev, PHY_FWV);
>> + if (fw_ver < 0)
>> + return true;
>> +
>> + fw_type = FIELD_GET(PHY_FWV_TYPE_MASK, fw_ver);
>> + fw_minor = FIELD_GET(PHY_FWV_MINOR_MASK, fw_ver);
>> +
>> + for (i = 0; i < ARRAY_SIZE(table); i++) {
>> + if (fw_type != table[i].type)
>> + continue;
>> + if (fw_minor < table[i].minor)
>> + return true;
>> + break;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static bool gpy_sgmii_2p5g_chk(struct phy_device *phydev)
> This name got me for a while. p meaning point? I would prefer
> gpy_sgmii_2500_chk.
Will fix in v2 patch.
>> + int ret;
>> +
>> + ret = phy_read(phydev, PHY_MIISTAT);
>> + if (ret < 0) {
>> + phydev_err(phydev, "Error: MDIO register access failed: %d\n",
>> + ret);
>> + return false;
>> + }
>> +
>> + if (!(ret & PHY_MIISTAT_LS)
>> + || FIELD_GET(PHY_MIISTAT_SPD_MASK, ret) != PHY_MIISTAT_SPD_2500)
>> + return false;
>> +
>> + phydev->speed = SPEED_2500;
>> + phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
> Then function is called SGMII but here you use 2500BaseX? Which is the
> hardware doing?
I will change it gpy_2500basex_chk in v2 patch.
>> +static bool gpy_sgmii_aneg_en(struct phy_device *phydev)
>> +{
>> + int ret;
>> +
>> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL);
>> + if (ret < 0) {
>> + phydev_err(phydev, "Error: MMD register access failed: %d\n",
>> + ret);
> You seem to like bool functions meaning you cannot return an error
> code. You might want to consider using int.
This is quite low chance error as MDIO bus is pretty reliable.
And in the logic here, even it has error, it won't break the function
and can continue to the next step.
>> + return true;
>> + }
>> +
>> + return (ret & VSPEC1_SGMII_CTRL_ANEN) ? true : false;
>> +}
>> +
>> +static int gpy_config_aneg(struct phy_device *phydev)
>> +{
>> + bool changed = false;
>> + u32 adv;
>> + int ret;
>> +
>> + if (phydev->autoneg == AUTONEG_DISABLE) {
>> + return phydev->duplex != DUPLEX_FULL
>> + ? genphy_setup_forced(phydev)
>> + : genphy_c45_pma_setup_forced(phydev);
>> + }
>> +
>> + ret = genphy_c45_an_config_aneg(phydev);
>> + if (ret < 0)
>> + return ret;
>> + if (ret)
>> + changed = true;
>> +
>> + adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising);
>> + ret = phy_modify_changed(phydev, MII_CTRL1000,
>> + ADVERTISE_1000FULL | ADVERTISE_1000HALF,
>> + adv);
>> + if (ret < 0)
>> + return ret;
>> + if (ret > 0)
>> + changed = true;
>> +
>> + ret = genphy_c45_check_and_restart_aneg(phydev, changed);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (phydev->interface == PHY_INTERFACE_MODE_USXGMII
>> + || phydev->interface == PHY_INTERFACE_MODE_INTERNAL)
> It is normal to put the || at the end of the line.
I will fix in v2 patch.
>
>> + return 0;
>> +
>> + /* No need to trigger re-ANEG if SGMII link speed is 2.5G
>> + * or SGMII ANEG is disabled.
>> + */
>> + if (!gpy_sgmii_need_reaneg(phydev) || gpy_sgmii_2p5g_chk(phydev)
>> + || !gpy_sgmii_aneg_en(phydev))
>> + return 0;
>> +
>> + /* There is a design constraint in GPY2xx device where SGMII AN is
>> + * only triggered when there is change of speed. If, PHY link
>> + * partner`s speed is still same even after PHY TPI is down and up
>> + * again, SGMII AN is not triggered and hence no new in-band message
>> + * from GPY to MAC side SGMII.
>> + * This could cause an issue during power up, when PHY is up prior to
>> + * MAC. At this condition, once MAC side SGMII is up, MAC side SGMII
>> + * wouldn`t receive new in-band message from GPY with correct link
>> + * status, speed and duplex info.
>> + *
>> + * 1) If PHY is already up and TPI link status is still down (such as
>> + * hard reboot), TPI link status is polled for 4 seconds before
>> + * retriggerring SGMII AN.
>> + * 2) If PHY is already up and TPI link status is also up (such as soft
>> + * reboot), polling of TPI link status is not needed and SGMII AN is
>> + * immediately retriggered.
>> + * 3) Other conditions such as PHY is down, speed change etc, skip
>> + * retriggering SGMII AN. Note: in case of speed change, GPY FW will
>> + * initiate SGMII AN.
>> + */
>> +
>> + if (phydev->state != PHY_UP)
>> + return 0;
>> +
>> + ret = phy_read_poll_timeout(phydev, MII_BMSR, ret, ret & BMSR_LSTATUS,
>> + 20000, 4000000, false);
>> + if (ret == -ETIMEDOUT)
>> + return 0;
>> + else if (ret < 0)
>> + return ret;
>> +
>> + /* Trigger SGMII AN. */
>> + return phy_modify_mmd_changed(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
>> + VSPEC1_SGMII_CTRL_ANRS,
>> + VSPEC1_SGMII_CTRL_ANRS);
>> +}
>> +
>> +static void gpy_update_interface(struct phy_device *phydev)
>> +{
>> + int ret;
>> +
>> + /* Interface mode is fixed for USXGMII and integrated PHY */
>> + if (phydev->interface == PHY_INTERFACE_MODE_USXGMII
>> + || phydev->interface == PHY_INTERFACE_MODE_INTERNAL)
> Please move the ||
will fix in v2 patch.
>
>> +static int gpy_read_status(struct phy_device *phydev)
>> +{
>> + int ret;
>> +
>> + ret = genphy_update_link(phydev);
>> + if (ret)
>> + return ret;
>> +
>> + phydev->speed = SPEED_UNKNOWN;
>> + phydev->duplex = DUPLEX_UNKNOWN;
>> + phydev->pause = 0;
>> + phydev->asym_pause = 0;
>> +
>> + if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>> + ret = genphy_c45_read_lpa(phydev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Read the link partner's 1G advertisement */
>> + ret = phy_read(phydev, MII_STAT1000);
>> + if (ret < 0)
>> + return ret;
>> + mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, ret);
> can genphy_read_lpa() be used here?
2.5G is not covered in genphy_read_lpa.
If I use genphy_c45_read_lpa first then genphy_read_lpa after, it seems
a bit redundant.
>> +static int gpy_set_wol(struct phy_device *phydev,
>> + struct ethtool_wolinfo *wol)
>> +{
>> + struct net_device *attach_dev = phydev->attached_dev;
>> + int ret;
>> +
>> + if (wol->wolopts & WAKE_MAGIC) {
>> + /* MAC address - Byte0:Byte1:Byte2:Byte3:Byte4:Byte5
>> + * VPSPEC2_WOL_AD45 = Byte0:Byte1
>> + * VPSPEC2_WOL_AD23 = Byte2:Byte3
>> + * VPSPEC2_WOL_AD01 = Byte4:Byte5
>> + */
>> + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
>> + VPSPEC2_WOL_AD45,
>> + ((attach_dev->dev_addr[0] << 8) |
>> + attach_dev->dev_addr[1]));
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
>> + VPSPEC2_WOL_AD23,
>> + ((attach_dev->dev_addr[2] << 8) |
>> + attach_dev->dev_addr[3]));
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
>> + VPSPEC2_WOL_AD01,
>> + ((attach_dev->dev_addr[4] << 8) |
>> + attach_dev->dev_addr[5]));
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Enable the WOL interrupt */
>> + ret = phy_write(phydev, PHY_IMASK, PHY_IMASK_WOL);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Enable magic packet matching */
>> + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
>> + VPSPEC2_WOL_CTL,
>> + WOL_EN);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Clear the interrupt status register */
>> + ret = phy_read(phydev, PHY_ISTAT);
>> + if (ret < 0)
>> + return ret;
> It seems like there is the potential to loose other interrupts here?
> I wonder if you should call phy_trigger_machine(phydev) if other bits
> are set?
You are right. I will fix in v2 patch.
>> +
>> + } else {
>> + /* Disable magic packet matching */
>> + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
>> + VPSPEC2_WOL_CTL,
>> + WOL_EN);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> + if (wol->wolopts & WAKE_PHY) {
>> + /* Enable the link state change interrupt */
>> + ret = phy_set_bits(phydev, PHY_IMASK, PHY_IMASK_LSTC);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Clear the interrupt status register */
>> + ret = phy_read(phydev, PHY_ISTAT);
>> + } else {
>> + /* Disable the link state change interrupt */
>> + ret = phy_clear_bits(phydev, PHY_IMASK, PHY_IMASK_LSTC);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static struct phy_driver gpy_drivers[] = {
>> + {
>> + .phy_id = PHY_ID_GPY,
>> + .phy_id_mask = PHY_ID_MASK,
>> +#define PHY_ID_MASK GENMASK(31, 10)
> That is a rather large mask? Can you split this up into individual
> devices?
I will change to smaller mask GENMASK(31, 4) in v2 patch.
>
> Andrew
>
Powered by blists - more mailing lists