Re: [RFC PATCH] net: phy/mdio: enable mmd indirect access through phy_mii_ioctl()

On 02/11/2021 14:39, Russell King (Oracle) wrote:

On Tue, Nov 02, 2021 at 01:49:42AM +0100, Andrew Lunn wrote:

quoted

quoted

The use of the indirect registers is specific to PHYs, and we already
know that various PHYs don't support indirect access, and some emulate
access to the EEE registers - both of which are handled at the PHY
driver level.

That is actually an interesting point. Should the ioctl call actually
use the PHY driver read_mmd and write_mmd? Or should it go direct to
the bus? realtek uses MII_MMD_DATA for something to do with suspend,
and hence it uses genphy_write_mmd_unsupported(), or it has its own
function emulating MMD operations.

So maybe the ioctl handler actually needs to use __phy_read_mmd() if
there is a phy at the address, rather than go direct to the bus?

Or maybe we should just say no, you should do this all from userspace,
by implementing C45 over C22 in userspace, the ioctl allows that, the
kernel does not need to be involved.

Yes and no. There's a problem accessing anything that involves some kind
of indirect or paged access with the current API - you can only do one
access under the bus lock at a time, which makes the whole thing
unreliable. We've accepted that unreliability on the grounds that this
interface is for debugging only, so if it does go wrong, you get to keep
all the pieces!

Right, MMD indirect access is 4 MDIO bus transactions.

The paged access case is really no different from the indirect C45 case.
They're both exactly the same type of indirect access, just using
different registers.

That said, the MII ioctls are designed to be a bus level thing - you can
address anything on the MII bus with them. Pushing the ioctl up to the
PHY layer means we need to find the right phy device to operate on.

The phy_read_mmd/__phy_read_mmd() was the first thing i considered, but
rejected exactly because of the possibility to access any MDIO device
through this ioctls.

in general, it can be called with check (mii->phy_id = pl->phydev->mdio.addr)

What
if we attempt a C45 access at an address that there isn't a phy device?

For example, what would be the effect of trying a C45 indirect access to
a DSA switch?

in case, C22/C22 MMD It will fail to read, seems no issues, and phytool will
just return 0xfffb.

First, there seems was previous attempt to do the same [1].

Also, there is some historical ... mess in this area :(
There are:

- generic_mii_ioctl() - 33 users (2005, it's older), uses struct mii_if_info

- mdio_mii_ioctl() - 7 users (2009), uses struct mdio_if_info

- phy_mii_ioctl() - 29 users, including phylink (2005), need PHY to get MDIO bus

- phy_do_ioctl()->phy_mii_ioctl() - 10 users (2020)

- phy_do_ioctl_running()->phy_mii_ioctl() - 22 users (2020)

- phylink_mii_ioctl() (also calls phy_mii_ioctl(), but only for SIOCSHWTSTAMP) - 9 users, including DSA (2017)
   need PHY to get MDIO bus, also uses PHY for c45 detection, but any phy_id can be passed.

- SIOCSMIIREG custom implementation - 32 users

Personally, my feeling would be that if we want to solve this, we need
to solve this properly - we need to revise the interface so it's
possible to request the kernel to perform a group of MII operations, so
that userspace can safely access any paged/indirect register. With that
solved, there will be no issue with requiring userspace to know what
it's doing with indirect C45 accesses.

It would require MDIO bus lock, which is not a solution,
or some sort of batch processing, like for mmd:
  w reg1 val1
  w reg2 val2
  w reg1 val3
  r reg2

What Kernel interface do you have in mind?

Sry, but I have to note that demand for this become terribly high, min two pings in months

[1] https://www.spinics.net/lists/netdev/msg653629.html

-- 
Best regards,
grygorii