netdev - Re: [PATCH net-next] net: phy: Have __phy

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

Message-ID: <20180112135251.GB29734@lunn.ch>
Date:   Fri, 12 Jan 2018 14:52:51 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     geert@...ux-m68k.org, niklas.cassel@...s.com,
        David Miller <davem@...emloft.net>,
        Russell King <rmk+kernel@....linux.org.uk>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: phy: Have __phy_modify return 0 on success

On Thu, Jan 11, 2018 at 05:29:22PM -0800, Florian Fainelli wrote:
> On 01/11/2018 12:55 PM, Andrew Lunn wrote:
> > __phy_modify would return the old value of the register before it was
> > modified. Thus on success, it does not return 0, but a positive value.
> > Thus functions using phy_modify, which is a wrapper around
> > __phy_modify, can start returning > 0 on success, rather than 0. As a
> > result, breakage has been noticed in various places, where 0 was
> > assumed.
> > 
> > Code inspection does not find any current location where the return of
> > the old value is currently used. 
> 
> phy_restore_page() does actually use the old value returned by
> __phy_modify(),

Hi Florian

int phy_restore_page(struct phy_device *phydev, int oldpage, int ret)
{
        int r;

        if (oldpage >= 0) {
                r = __phy_write_page(phydev, oldpage);
                /* Propagate the operation return code if the page write        
                 * was successful.                                              
                 */
                if (ret >= 0 && r < 0)
                        ret = r;
        } else {
		
                /* Propagate the phy page selection error code */
                ret = oldpage;
        }

        mutex_unlock(&phydev->mdio.bus->mdio_lock);

        return ret;
}

Ah! I see it now. The value of ret parameter can be what phy_modify()
returned.  If ret is not an error, and __phy_write_page() returned an
error, use r, the error from __phy_write_page().

As you say, the actual value is not used, just an indication of if it
represents an error. So this change is O.K.

	Andrew