netdev - Re: [PATCH net-next] mscc: Add LCPLL Reset to VSC8574 Family of phy drivers

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: <c55019cd-32cf-cc72-1188-3115c547102c@gmail.com>
Date:   Fri, 24 Jul 2020 11:00:57 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Bryan.Whitehead@...rochip.com, davem@...emloft.net
Cc:     netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next] mscc: Add LCPLL Reset to VSC8574 Family of phy
 drivers

On 7/24/20 9:29 AM, Bryan.Whitehead@...rochip.com wrote:
> Hi Florian, see below.
> 
>>>  /* bus->mdio_lock should be locked when using this function */
>>> +/* Page should already be set to MSCC_PHY_PAGE_EXTENDED_GPIO */
>>> +static int vsc8574_wait_for_micro_complete(struct phy_device *phydev)
>>> +{
>>> +     u16 timeout = 500;
>>> +     u16 reg18g = 0;
>>> +
>>> +     reg18g = phy_base_read(phydev, 18);
>>> +     while (reg18g & 0x8000) {
>>> +             timeout--;
>>> +             if (timeout == 0)
>>> +                     return -1;
>>> +             usleep_range(1000, 2000);
>>> +             reg18g = phy_base_read(phydev, 18);
>>
>> Please consider using phy_read_poll_timeout() instead of open coding this busy
>> waiting loop.
> 
> There are a couple issues with the use of phy_read_poll_timeout
> 1) It requires the use of phy_read, which acquires bus->mdio_lock.
> But this function is run with the assumption that, that lock is already acquired.
> There for I presume it will deadlock> 2) The implementation of phy_base_read uses __phy_package_read which
uses a shared phy addr, rather than the addr associated with the phydev.
> 
> These issues could be eliminated if I used read_poll_timeout directly.
> Does that seem reasonable to you?

Certainly, whatever makes the code more maintainable and makes use of
existing functions is better on all counts.
-- 
Florian