Re: [PATCH net-next v3] net: stmmac: don't create a MDIO bus if unnecessary

[Serge Semin]

On Thu, Dec 07, 2023 at 05:07:24PM -0600, Andrew Halaney wrote:
> On Fri, Dec 08, 2023 at 01:16:12AM +0300, Serge Semin wrote:
> > On Thu, Dec 07, 2023 at 03:12:40PM -0600, Andrew Halaney wrote:
> > > The stmmac_dt_phy() function, which parses the devicetree node of the
> > > MAC and ultimately causes MDIO bus allocation, misinterprets what
> > > fixed-link means in relation to the MAC's MDIO bus. This results in
> > > a MDIO bus being created in situations it need not be.
> > > 
> > > Currently a MDIO bus is created if the description is either:
> > > 
> > >     1. Not fixed-link
> > >     2. fixed-link but contains a MDIO bus as well
> > > 
> > > The "1" case above isn't always accurate. If there's a phy-handle,
> > > it could be referencing a phy on another MDIO controller's bus[1]. In
> > > this case currently the MAC will make a MDIO bus and scan it all
> > > anyways unnecessarily.
> > > 
> > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to
> > > be created and scanned for a phy. This case can also be inferred from
> > > the platform description by not having a phy-handle && not being
> > > fixed-link. This hits case "1" in the current driver's logic.
> > > 
> > > Let's improve the logic to create a MDIO bus if either:
> > > 
> > 
> > >     - Devicetree contains a MDIO bus
> > >     - !fixed-link && !phy-handle (legacy handling)
> > 
> > If what you suggest here is a free from regressions semantics change
> > (really hope it is) I will be with both my hands for it. This will
> > solve the problem we have with one of our device which doesn't have
> > SMA interface (hardware designers decided to save ~4K gates of the
> > chip area) but has a PHY externally attached to the DW XGMAC<->XPCS
> > interface. PHY is accessible via a GPIO-based MDIO bus. BTW having no
> > SMA interface available on a DW *MAC device but creating the MDIO-bus
> > on top of the non-existent SMA CSRs anyway causes having _32_ dummy
> > PHYs created with zero IDs.
> 

> I hope it is regression free! I have tested both the [1] and [2] cases
> (I hacked up the devicetree for [1] to make it look like [2]) without
> any issue.
> 

I doubt you could have tested it on all the possible hardware the
STMMAC driver supports. The problem is that the DT-bindings thing is a
kind of contract which can't be changed that easily. It's like ABI but
for the hardware description so the kernel would bootup correctly on
the platforms with the old DT blobs. But if the change isn't that
critical, if the device-tree sources in the kernel fit to the updated
semantics, if the networking subsystem maintainers aren't against it
and I guess with the Rob, Krzysztof or Conor blessing (at least it
won't hurt to add them to the Cc-list together with the devicetree
mailing-list), then it will likely be accepted.

> Sorry, I don't have any docs for stmmac hardware so this might be
> answered in there (or just common net knowledge that I can't find
> online)... what's SMA stand for? I assume it's the MDIO interface.

Right. Synopsys names the MDIO-bus interface as Station Management
Agent MDIO module.

> 
> I agree though, if you have a phy-handle and no mdio node in your
> devicetree this patch series should bail out without registering a bus
> in stmmac_mdio_register().

On the other hand why would the MDIO-bus needed in such case? If the
phy-handle property is specified with no MDIO-bus DT-subnode, then it
will point out to a PHY residing an external bus. The only case I can
imagine though is that the DW XPCS device could be still auto-detected
on the internal SMA-MDIO-bus. But the only driver which currently has
XPCS auto-detection activated is the Intel glue layer (see
dwmac-intel.c and has_xpcs flag), but it doesn't use DT interface
since it handles a PCIe-based device.  So this case is out of
brackets.

> 
> > 
> > > 
> > > Below upstream devicetree snippets can be found that explain some of
> > > the cases above more concretely.
> 
> <snip>
> 
> > > -	if (mdio) {
> > > -		plat->mdio_bus_data =
> > > -			devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data),
> > > -				     GFP_KERNEL);
> > 
> > > +	/* Legacy devicetrees allowed for no MDIO bus description and expect
> > > +	 * the bus to be scanned for devices. If there's no phy or fixed-link
> > > +	 * described assume this is the case since there must be something
> > > +	 * connected to the MAC.
> > > +	 */
> > > +	legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node;
> > > +	if (legacy_mdio)
> > > +		dev_info(dev, "Deprecated MDIO bus assumption used\n");
> > > +
> > > +	if (plat->mdio_node || legacy_mdio) {
> > > +		plat->mdio_bus_data = devm_kzalloc(dev,
> > 
> > Special thanks for adding the comment above this code. It will really
> > save time of figuring out why MDIO-bus needs to be created anyway.
> > 
> > > +						   sizeof(struct stmmac_mdio_bus_data),
> > 
> > Should v4 is required I would suggest to change this to
> > sizeof(*plat->mdio_bus_data).
> > 
> > Anyway feel free to add:
> > Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > 
> > -Serge(y)
> 

> Sure I will spin v4 to pick that up, thanks for catching it. I'll also
> improve the motivation in the commit message a hair more per Andrew
> Lunn's request over here on v2 (and will hold off a little bit just to
> make sure reviews come in before a respin):
> 
>     https://lore.kernel.org/netdev/e64b14c3-4b80-4120-8cc4-9baa40cdcb75@xxxxxxx/

Ok. Thanks.

-Serge(y)

> 
> Thanks,
> Andrew
>