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

[Serge Semin]

Hi Vladimir, Russell, Andrew

On Wed, Dec 13, 2023 at 06:27:57PM +0300, Serge Semin wrote:
> 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.

Folks, this is the only issue left to settle so I could move on with
the series fixing up. So the question is: taking my comment above into
account is it worth to convert the xpcs_create_mdiodev() method to
re-using the already registered MDIO-device instance instead of
always creating a stub-like MDIO-device?

-Serge(y)

> 
> 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)