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

From: Serge Semin <fancer.lancer@gmail.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	 Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Jose Abreu <joabreu@synopsys.com>,
	 Jose Abreu <Jose.Abreu@synopsys.com>,
	Maxime Chevallier <maxime.chevallier@bootlin.com>,
	 Tomer Maimon <tmaimon77@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	 "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	 Jakub Kicinski <kuba@kernel.org>,
	Paolo Abeni <pabeni@redhat.com>,
	openbmc@lists.ozlabs.org,  netdev@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 06/16] net: pcs: xpcs: Avoid creating dummy XPCS MDIO device
Date: Tue, 5 Dec 2023 14:31:41 +0300	[thread overview]
Message-ID: <rgp33mm4spbpm5tmgxurkhy4is3lz3z62rz64rni2pygteyrit@zwflw2ejdkn7> (raw)
In-Reply-To: <ZW8ASzkC9IFFlxkV@shell.armlinux.org.uk>

On Tue, Dec 05, 2023 at 10:49:47AM +0000, Russell King (Oracle) 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 there is no point in creating the
> > dummy MDIO device instance in order to get the DW XPCS handler since the
> > MDIO core subsystem will create the device during the MDIO bus
> > registration procedure.
> 

> Please reword this overly long sentence.

Ok.

> 
> If they're left unmasked, what prevents them being created as PHY
> devices?

Not sure I fully get what you meant. If they are left unmasked the
MDIO-device descriptor will be created by the MDIO subsystem anyway.
What the point in creating another one?

> 
> > @@ -1437,19 +1435,21 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
> >  	struct mdio_device *mdiodev;
> >  	struct dw_xpcs *xpcs;
> >  
> > -	mdiodev = mdio_device_create(bus, addr);
> > -	if (IS_ERR(mdiodev))
> > -		return ERR_CAST(mdiodev);
> > +	if (addr >= PHY_MAX_ADDR)
> > +		return ERR_PTR(-EINVAL);
> >  
> > -	xpcs = xpcs_create(mdiodev, interface);
> > +	if (mdiobus_is_registered_device(bus, addr)) {
> > +		mdiodev = bus->mdio_map[addr];
> > +		mdio_device_get(mdiodev);
> 

> No, this makes no sense now. This function is called
> xpcs_create_mdiodev() - note the "create_mdiodev" part. If it's getting
> the mdiodev from what is already there then it isn't creating it, so
> it's no longer doing what it says in its function name. If you want to
> add this functionality, create a new function to do it.

AFAICS the method semantics is a bit different. It's responsibility is to
create the DW XPCS descriptor. MDIO-device is utilized internally by
the DW XPCS driver. The function callers don't access the created MDIO
device directly (at least since some recent commit). So AFAIU "create"
means creating the XPCS descriptor irrespective from the internal
communication layer. So IMO the suffix is a bit misleading. I'll
change it in one of the next commit anyway. Should I just merge that
patch back in this one?

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!