netdev - Re: [RFC PATCH] net: phy/mdio: enable mmd indirect access through phy_mii

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: <bc9df441-49bf-5c8a-891c-cc3f0db00aba@ti.com>
Date:   Tue, 2 Nov 2021 19:19:46 +0200
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>
CC:     "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        <linux-kernel@...r.kernel.org>,
        Vignesh Raghavendra <vigneshr@...com>
Subject: 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:
>>> 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