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)
