linux-kernel - 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]

Message-ID: <f3f831f453378fe3b55e8fc5606266eb@walle.cc>
Date:   Wed, 10 Feb 2021 10:14:59 +0100
From:   Michael Walle <michael@...le.cc>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        "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

Hi,

Am 2021-02-10 10:03, schrieb Heiner Kallweit:
[..]
>>>> +    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?
>> 
>> First, I want to convert this to the match_phy_device() and while at 
>> it,
>> I noticed that there is this one "problem". Short summary: the IP101A 
>> isn't
>> paged, the IP101G has serveral and if page 16 is selected it is more 
>> or
>> less compatible with the IP101A. My problem here is now how to share 
>> the
>> functions for both PHYs without duplicating all the code. Eg. the 
>> IP101A
>> will phy_read/phy_write/phy_modify(), that is, all the locked 
>> versions.
>> For the IP101G I'd either need the _paged() versions or the __phy ones
>> which don't take the mdio_bus lock.
>> 
>> Here is what I came up with:
>> (1) provide a common function which uses the __phy ones, then the
>>     callback for the A version will take the mdio_bus lock and calls
>>     the common one. The G version will use 
>> phy_{select,restore}_page().
>> (2) the phy_driver ops for A will also provde a .read/write_page()
>>     callback which is just a no-op. So A can just use the G versions.
>> (3) What Heiner mentioned here, just set the default page in
>>     config_init().
>> 
>> (1) will still bloat the code; (3) has the disadvantage, that the
>> userspace might fiddle around with the page register and then the
>> whole PHY driver goes awry. I don't know if we have to respect that
>> use case in general. I know there is an API to read/write the PHY
>> registers and it could happen.
>> 
> 
> The potential issue you mention here we have with all PHY's using
> pages. As one example, the genphy functions rely on the PHY being
> set to the default page. In general userspace can write PHY register
> values that break processing, independent of paging.
> I'm not aware of any complaints regarding this behavior, therefore
> wouldn't be too concerned here.

I'm fine with that, that will make the driver easier.

> Regarding (2) I'd like to come back to my proposal from yesterday,
> implement match_phy_device to completely decouple the A and G versions.
> Did you consider this option?

Yes, that is what I was talking about above: "First, I want to convert
this to the match_phy_device()" ;) And then I stumbled across that 
problem
I was describing above.

It will likely go away if I just set the page to the default page.

>> That being said, I'm either fine with (2) and (3) but I'm preferring
>> (2).
>> 
>> BTW, this patch is still missing read/writes to the interrupt status
>> and control registers which is also paged.

-- 
-michael