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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 1 Jun 2021 17:14:01 +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 2/6/2021 12:55 am, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
>>>> +     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?

I thought to use get_phy_c45_ids in gpy_config_init to populate the 
c45_ids, but this is a static function inside phy_device.c.

Or maybe in genphy_c45_pma_read_abilities, it reads MII_BMSR register if 
is_c45 not set.

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

I agree, this is issue for mixed C22/45 device.

I saw something similar in BRCM driver (bcm84881_read_status) and 
Marvell driver (mv3310_read_status_copper).

They are C45 device and use vendor specific register in MMD to access 
C22 equivalent registers.

>
>     Andrew
>