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

[Heiner Kallweit]

On 10.02.2021 13:17, Michael Walle wrote:
> Am 2021-02-10 12:48, schrieb Russell King - ARM Linux admin:
>> On Wed, Feb 10, 2021 at 12:14:35PM +0100, Michael Walle wrote:
>>> Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin:
>>> 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.
>>
>> Provided the PHY driver uses the paged accessors for all paged
>> registers, userspace can't break the PHY driver because we have proper
>> locking in the paged accessors (I wrote them.) Userspace can access the
>> paged registers too, but there will be no locking other than on each
>> individual access. This can't be fixed without augmenting the kernel/
>> user API, and in any case is not a matter for the PHY driver.
>>
>> So, let's stop worrying about the userspace paged access problem for
>> driver reviews; that's a core phylib and userspace API issue.
>>
>> The paged accessor API is designed to allow the driver author to access
>> registers in the most efficient manner. There are two parts to it.
>>
>> 1) the phy_*_paged() accessors switch the page before accessing the
>>    register, and restore it afterwards. If you need to access a lot
>>    of paged registers, this can be inefficient.
>>
>> 2) phy_save_page()..phy_restore_page() allows wrapping of __phy_*
>>    accessors to access paged registers.
>>
>> 3) phy_select_page()..phy_restore_page() also allows wrapping of
>>    __phy_* accessors to access paged registers.
>>
>> phy_save_page() and phy_select_page() must /always/ be paired with
>> a call to phy_restore_page(), since the former pair takes the bus lock
>> and the latter releases it.
>>
>> (2) and (3) allow multiple accesses to either a single page without
>> constantly saving and restoring the page, and can also be used to
>> select other pages without the save/restore and locking steps. We
>> could export __phy_read_page() and __phy_write_page() if required.
>>
>> While the bus lock is taken, userspace can't access any PHY on the bus.
> 
> That was how the v1 of this series was written. But Heiner's review
> comment was, what if we just set the default page and don't
> use phy_save_page()..phy_restore_page() for the registers which are
> behind the default page. And in this case, userspace _can_ break
> access to the paged registers.
> 

The comment was assuming that paging also applies to register 0..15,
like it is the case for Realtek PHY's. That's not the case for your
PHY, therefore the situation is slightly different.


> -michael