netdev - Re: [PATCH net-next 7/9] net: phy: icplus: select page before writing control register

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]

Date:   Wed, 10 Feb 2021 12:14:35 +0100
From:   Michael Walle <michael@...le.cc>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next 7/9] net: phy: icplus: select page before writing
 control register

Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin:
> On Wed, Feb 10, 2021 at 11:38:18AM +0100, Michael Walle wrote:
>> Am 2021-02-10 11:30, schrieb Russell King - ARM Linux admin:
>> > On Wed, Feb 10, 2021 at 08:03:07AM +0100, Heiner Kallweit wrote:
>> > > On 09.02.2021 17:40, Michael Walle wrote:
>> > > > +out:
>> > > > +	return phy_restore_page(phydev, oldpage, err);
>> > >
>> > > If a random page was set before entering config_init, do we actually
>> > > want
>> > > to restore it? Or wouldn't it be better to set the default page as
>> > > part
>> > > of initialization?
>> >
>> > I think you've missed asking one key question: does the paging on this
>> > PHY affect the standardised registers at 0..15 inclusive, or does it
>> > only affect registers 16..31?
>> 
>> For this PHY it affects only registers >=16. But that doesn't 
>> invaldiate
>> the point that for other PHYs this might affect all regsisters. Eg. 
>> ones
>> where you could select between fiber and copper pages, right?
> 
> You are modifying the code using ip101a_g_* functions, which is only
> used for the IP101A and IP101G PHYs. Do these devices support fiber
> in a way that change the first 16 registers?

The PHY doesn't support fiber and register 0..15 are always available
regardless of the selected page for the IP101G.

genphy_() stuff will work, but the IP101G PHY driver specific functions,
like interrupt and mdix will break if someone is messing with the page
register from userspace.

So Heiner's point was, that there are other PHY drivers which
also break when a user changes registers from userspace and no one
seemed to cared about that for now.

I guess it boils down to: how hard should we try to get the driver
behave correctly if the user is changing registers. Or can we
just make the assumption that if the PHY driver sets the page
selection to its default, all the other callbacks will work
on this page.

-michael