Re: [PATCH net-next 06/16] net: pcs: xpcs: Avoid creating dummy XPCS MDIO device

[Serge Semin]

Hi Vladimir,

On Tue, Dec 05, 2023 at 01:52:34PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote:
> > If the DW XPCS MDIO devices are either left unmasked for being auto-probed
> > or explicitly registered in the MDIO subsystem by means of the
> > mdiobus_register_board_info() method
> 

> mdiobus_register_board_info() has exactly one caller, and that is
> dsa_loop. I don't understand the relevance of it w.r.t. Synopsys XPCS.
> I'm reading the patches in order from the beginning.

Well, one user of the DW XPCS driver is updated in this series in the
framework of the patch:
[PATCH net-next 13/16] net: stmmac: intel: Register generic MDIO device
https://lore.kernel.org/netdev/20231205103559.9605-14-fancer.lancer@xxxxxxxxx/

I can convert of them (it's sja1105 and wangxun txgbe) and then just
drop the MDIO-device creation part from xpcs_create_mdiodev(). As I
also described in another emails thread below this patch I used to
think that unmasking non-PHY device is also appropriate to get the
MDIO-device instance. I was wrong in that matter obviously.

Anyway I just realized that my solution of using
mdiobus_register_board_info() is a bit clumsy. Moreover the patch 13
(see the link above) shouldn't have the mdio_board_info instance
allocation (it can be defined on stack) and most importantly is wrong
in using the device-managed resources for it. The problem is that
mdiobus_register_board_info() registers an MDIO-device once for entire
system lifetime. It isn't that suitable for the hot-swappable devices
and for drivers bind/unbind cases. Since there is no
mdio_board_info-deregistration method, at the simplest case the no
longer used board-info descriptors might be left registered if a
device or driver are unloaded. That's why the device-managed
allocation is harmful in such scenario. At the very least I'll need to
convert the allocations to being non-managed.

> 
> > there is no point in creating the dummy MDIO device instance in order
> 

> Why dummy? There's nothing dummy about the mdio_device. It's how the PCS
> code accesses the hardware.

I call it 'dummy' because no actual device is registered (though
'redundant' or similar definition might sound more appropriate). The
entire structure is used as a communication layer between the XPCS
driver and MDIO device, where the device address is the only info
needed. Basically nothing prevents us from converting the current DW
XPCS driver to using the mdiobus_c45_read()/mdiobus_c45_write()
methods. Though in that case I wouldn't be able to easily add the
fwnode-based MDIO-devices support.

> 
> > to get the DW XPCS handler since the MDIO core subsystem will create
> > the device during the MDIO bus registration procedure.
> 

> It won't, though? Unless someone is using mdiobus_register_board_info()
> possibly, but who does that?

As I said above I wrongly assumed that unmasking non-PHY device was
ok. But mdiobus_register_board_info() could be used for that as I (a
bit clumsy) demonstrated in the patch 13.

> 
> > All what needs to be done is to just reuse the MDIO-device instance
> > available in the mii_bus.mdio_map array (using some getter for it
> > would look better though). It shall prevent the XPCS devices been
> > accessed over several MDIO-device instances.
> > 
> > Note since the MDIO-device instance might be retrieved from the MDIO-bus
> > map array its reference counter shall be increased. If the MDIO-device
> > instance is created in the xpcs_create_mdiodev() method its reference
> > counter will be already increased. So there is no point in toggling the
> > reference counter in the xpcs_create() function. Just drop it from there.
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > ---
> 

> Sorry, because the commit log lost me at the "context presentation" stage,
> I failed to understand the "what"s and the "why"s.
> 
> Are you basically trying to add xpcs support on top of an mdio_device
> where the mdio_device_create() call was made externally to the xpcs code,
> through mdiobus_register_board_info() and mdiobus_setup_mdiodev_from_board_info()?

Basically yes, but there is more of it. The main idea is to convert
the XPCS driver to using the already created non-PHY MDIO-devices
instead of manually creating a 'dummy'/'redundant' one. From my point
of view there are several reasons of doing so:

1. mdiobus_register_board_info() provides a way to assign the device
platform data to being registered afterwards device. Thus we can pass
some custom data to the XPCS-device driver (whether it's just an
xpcs_create_*() call or a fully functional MDIO-device driver
registered by the mdio_driver_register() method). For instance it can
be utilized to drop the fake PHYSIDs implementation from
drivers/net/dsa/sja1105/sja1105_mdio.c .

2. The MDIO-devices actually registered on the MDIO-bus will be
visible in sysfs with for instance useful IO statistics provided by
the MDIO-bus. Potentially (if it is required) at some point we'll be
able to convert the DW XPCS driver to being true MDIO-device driver
(bindable to the DW XPCS device) with less efforts.

3. Having an MDIO-device registered that way would make the DW XPCS
IO-device implementation unified after the fwnode-based XPCS
descriptor creation support is added in one of the subsequent patches.

So based on the listed above I've got a question. Do you think all of
that is worth to be implemented? Andrew, Russell?

I am asking because the patchset advance depends on your answers. If
you do I'll need to fix the problem described in my first message,
implement some new mdiobus_register_board_info()-like but
MDIO-bus-specific interface function (so MDIO-device infos would be
attached to the allocated MDIO-bus and then used to register the
respective MDIO-devices on the MDIO-bus registration), then convert
the sja1105 and wangxun txgbe drivers to using it. If you don't I'll
get back the xpcs_create_mdiodev() implementation and just provide a
fwnode-based version of one.

Note we already settled that converting DW XPCS driver to being normal
MDIO-device driver is prone to errors at this stage due to a
possibility to have the driver unbindable from user-space. I'll just
move the DT-compatibles check to the xpcs_create_fwnode() method and
drop the rest of the MDIO-device-driver-specific things.

-Serge(y)