Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses

[Andrew Lunn <andrew@lunn.ch>]

On Sun, Apr 04, 2021 at 09:23:55PM +0200, Danilo Krummrich wrote:
> On Fri, Apr 02, 2021 at 01:58:58PM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Apr 02, 2021 at 03:10:49AM +0200, Danilo Krummrich wrote:
> > > On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin wrote:
> > > > One could also argue this is a feature, and it allows userspace to
> > > > know whether C45 cycles are supported or not.
> > > >
> > > No, if the userspace requests such a transfer although the MDIO controller
> > > does not support real c45 framing the kernel will call mdiobus_c45_addr() to
> > > join the devaddr and  and regaddr in one parameter and pass it to
> > > mdiobus_read() or mdiobus_write(). A bus driver not supporting c45 framing
> > > will not care and just mask/shift the joined value and write it to the
> > > particular register. Obviously, this will result into complete garbage being
> > > read or (even worse) written.
> > 
> > 
> > We have established that MDIO drivers need to reject accesses for
> > reads/writes that they do not support - this isn't something that
> > they have historically checked for because it is only recent that
> > phylib has really started to support clause 45 PHYs.
> > 
> I see, that's why you consider it a feature - because it is.
> What do you think about adding a flag MDIO_PHY_ID_MMD (or similar) analog to
> MDIO_PHY_ID_C45 for phy_mii_ioctl() to check for, such that userspace can ask
> for an indirect access in order to save userspace doing the indirect access
> itself. A nice side effect would be saving 3 syscalls per request.

We don't care about the performance of this IOCTL interface. It is for
debug only, and you need to be very careful how you use it, because
you can very easily shoot yourself in the foot.

> So currently every driver should check for the flag MII_ADDR_C45 and report an
> error in case it's unsupported.
> 
> What do you think about checking the bus' capabilities instead in
> mdiobus_c45_*()? This way the check if C45 is supported can even happen before
> calling the driver at all. I think that would be a little cleaner than having
> two places where information of the bus' capabilities are stored (return value
> of read/write functions and the capabilities field).
> 
> I think there are not too many drivers setting their capabilities though, but
> it should be easy to derive this information from how and if they handle the
> MII_ADDR_C45 flag.

I actually don't think anything needs to change. The Marvell PHY
probably probes due to its C22 IDs. The driver will then requests C45
access which automagically get converted into C45 over C22 for your
hardware, but remain C45 access for bus drivers which support C45.

	  Andrew