netdev - Re: [PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

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]

Message-ID: <CAHwZ4N3dn+jWG0Hbz2ptPRyA3i1SwCq1F7ipgMdwBaahntqkjA@mail.gmail.com>
Date: Tue, 3 Dec 2024 11:08:22 +0800
From: 万致远 <kmlinuxm@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: kuba@...nel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	willy.liu@...ltek.com, Yuki Lee <febrieac@...look.com>
Subject: Re: [PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

On 2024/12/3 7:52, Andrew Lunn wrote:
>> +static int rtl8211f_config_aneg(struct phy_device *phydev)
>> +{
>> +    int ret;
>> +
>> +    struct rtl821x_priv *priv = phydev->priv;
>> +
>> +    ret = genphy_read_abilities(phydev);
>> +    if (ret < 0)
>> +            return ret;
>> +
>> +    linkmode_copy(phydev->advertising, phydev->supported);
>
> This is all very unusual for config_aneg(). genphy_read_abilities()
> will have been done very early on during phy_probe(). So why do it
> now? And why overwrite how the user might of configured what is to be
> advertised?
>

These codes are migrated from Rockchip SDK and I'm not familiar with this part.

I will use `linkmode_and` instead of `linkmode_copy` in my next
version of patch like Marvell does.

>> +static int rtl8211f_read_status(struct phy_device *phydev)
>> +{
>> +    int ret;
>> +    struct rtl821x_priv *priv = phydev->priv;
>> +    bool changed = false;
>> +
>> +    if (rtl8211f_mode(phydev) != priv->lastmode) {
>> +            changed = true;
>> +            ret = rtl8211f_config_aneg(phydev);
>> +            if (ret < 0)
>> +                    return ret;
>> +
>> +            ret = genphy_restart_aneg(phydev);
>> +            if (ret < 0)
>> +                    return ret;
>> +    }
>> +
>> +    return genphy_c37_read_status(phydev, &changed);
>> +}
>
> So you are assuming read_status() is called once per second? But what
> about when interrupts are used?
>
> You might want to look at how the marvell driver does this. It is not
> great, but better than this.

I'm not familiar with that either, could you please tell me how to do
it properly to automatically switch between copper and fiber mode?

>
>     Andrew
>
> ---
> pw-bot: cr
Sincerely,

Zhiyuan Wan