Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Thu, 7 Dec 2023 17:07:24 -0600
From:   Andrew Halaney <ahalaney@redhat.com>
To:     Serge Semin <fancer.lancer@gmail.com>
Cc:     Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Jose Abreu <joabreu@synopsys.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: [PATCH net-next v3] net: stmmac: don't create a MDIO bus if
 unnecessary
Message-ID: <hxds75erxqcfkufxnfbyo2up4b4jeicmi3f5xr6qlb3yf7fe76@4byeq62jhu4o>
References: <20231207-stmmac-no-mdio-node-v3-1-34b870f2bafb@redhat.com>
 <jz6ot44fjkbmwcezi3fkgqd54nurglblbemrchfgxgq6udlhqz@ntepnnzzelta>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <jz6ot44fjkbmwcezi3fkgqd54nurglblbemrchfgxgq6udlhqz@ntepnnzzelta>
Precedence: bulk

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.

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.

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

> 
> > 
> > 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@gmail.com>
> 
> -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@lunn.ch/

Thanks,
Andrew