netdev - Re: [PATCH] phy: maxlinear: add Maxlinear GPY115/21x/24x driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Jun 2021 18:55:03 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Liang Xu <lxu@...linear.com>
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

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

This sounds like a generic problem, which will affect any PHY which
has both C22 and C45. I wounder if it makes sense to add a helper
function which a PHY driver can call to get the
phydev->c45_ids.mmds_present populated?

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

I'm just trying to avoid repeating code which is in helpers. I think
this is the first PHY driver which uses a mixture of C22 and C45 like
this. So it could be the helpers need small modifications to make them
work. We should make those modifications, since your PHY is not likely
to be the only mixed C22 and C45 device.

   Andrew