netdev - Re: [PATCH net-next v4] net: phy: mxl-gpy: enhance delay time required by loopback disable function

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] [day] [month] [year] [list]

Message-ID: <4219e4e0-dc44-1c23-8ae2-05cafee54bc6@maxlinear.com>
Date:   Tue, 14 Mar 2023 14:43:08 +0000
From:   Liang Xu <lxu@...linear.com>
To:     Michael Walle <michael@...le.cc>, 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>,
        Rahul Tanwar <rtanwar@...linear.com>,
        "Ismail, Mohammad Athari" <mohammad.athari.ismail@...el.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>
Subject: Re: [PATCH net-next v4] net: phy: mxl-gpy: enhance delay time
 required by loopback disable function

On 14/3/2023 10:04 pm, Michael Walle wrote:
> This email was sent from outside of MaxLinear.
>
>
> Am 2023-03-14 14:50, schrieb Andrew Lunn:
>>> +    /* It takes 3 seconds to fully switch out of loopback mode before
>>> +     * it can safely re-enter loopback mode. Record the time when
>>> +     * loopback is disabled. Check and wait if necessary before 
>>> loopback
>>> +     * is enabled.
>>> +     */
>>
>> Is there are restriction about entering loopback mode within the first
>> 3 seconds after power on?
>>
>>> +    bool lb_dis_chk;
>>> +    u64 lb_dis_to;
>>>  };
>>>
>>>  static const struct {
>>> @@ -769,18 +777,34 @@ static void gpy_get_wol(struct phy_device
>>> *phydev,
>>>
>>>  static int gpy_loopback(struct phy_device *phydev, bool enable)
>>>  {
>>> +    struct gpy_priv *priv = phydev->priv;
>>> +    u16 set = 0;
>>>      int ret;
>>>
>>> -    ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
>>> -                     enable ? BMCR_LOOPBACK : 0);
>>> -    if (!ret) {
>>> -            /* It takes some time for PHY device to switch
>>> -             * into/out-of loopback mode.
>>> +    if (enable) {
>>> +            /* wait until 3 seconds from last disable */
>>> +            if (priv->lb_dis_chk && 
>>> time_is_after_jiffies64(priv->lb_dis_to))
>>> + msleep(jiffies64_to_msecs(priv->lb_dis_to - get_jiffies_64()));
>>> +
>>> +            priv->lb_dis_chk = false;
>>> +            set = BMCR_LOOPBACK;
>>
>> Maybe this can be simplified by setting priv->lb_dis_to =
>> get_jiffies_64() + HZ * 3 in _probe(). Then you don't need
>> priv->lb_dis_chk.
>
> First, I wonder if this is worth the effort and code complications.
> phy_loopback() seem to be used very seldom. Anyway.
>
> Can't we just save the jiffies on last enable as kind of a timestamp.
> If it's 0 you know it wasn't called yet and if it's set, you have to at
> least wait for until it is after "jiffies + HZ*3".
>
> Also isn't that racy right now? "priv->lb_dis_to - get_jiffies_64())"
> can
> get negative, no?
>
Yes, this is a bug. I will fix.

Also use jiffies only.

There is no restriction to enter loopback mode for first 3 seconds.

> -michael
>