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

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!