netdev - Re: [PATCH] net: phy: Fix phy_modify() semantic difference fallout

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: <20180111.104835.381039264896352436.davem@davemloft.net>
Date:   Thu, 11 Jan 2018 10:48:35 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     geert+renesas@...der.be
Cc:     rmk+kernel@...linux.org.uk, andrew@...n.ch, f.fainelli@...il.com,
        netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: Fix phy_modify() semantic difference fallout

From: Geert Uytterhoeven <geert+renesas@...der.be>
Date: Tue,  9 Jan 2018 12:11:21 +0100

> In case of success, the return values of (__)phy_write() and
> (__)phy_modify() are not compatible: (__)phy_write() returns 0, while
> (__)phy_modify() returns the old PHY register value.
> 
> Apparently this change was catered for in drivers/net/phy/marvell.c, but
> not in other source files.
> 
> Hence genphy_restart_aneg() now returns 4416 instead zero, which is
> considered an error:
> 
>     ravb e6800000.ethernet eth0: failed to connect PHY
>     IP-Config: Failed to open eth0
>     IP-Config: No network devices available
> 
> Fix this by converting positive values to zero in all callers of
> phy_modify().
> 
> Fixes: fea23fb591cce995 ("net: phy: convert read-modify-write to phy_modify()")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> ---
> Alternatively, __phy_modify() could be changed to follow __phy_write()
> semantics?

I really want a resolution to this quickly, this broke lots of stuff
for people.

__phy_modify() wants to return multiple values, so it should be coded
up to do so explicitly rather than trying to encode two values from
overlapping value spaces in one return value.

That means the original value should be returned by-reference.  And
this will make the error/no-error return value unambiguous.

int __phy_modify(struct phy_device *phydev, u32 regnum, u16 mask, u16 set,
		 u16 *orig_val);

Thank you.