Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
CC:     "Jose.Abreu@synopsys.com" <Jose.Abreu@synopsys.com>,
        "andrew@lunn.ch" <andrew@lunn.ch>,
        "hkallweit1@gmail.com" <hkallweit1@gmail.com>,
        "linux@armlinux.org.uk" <linux@armlinux.org.uk>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "peppe.cavallaro@st.com" <peppe.cavallaro@st.com>,
        "alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
        "weifeng.voon@intel.com" <weifeng.voon@intel.com>,
        "boon.leong.ong@intel.com" <boon.leong.ong@intel.com>,
        "tee.min.tan@intel.com" <tee.min.tan@intel.com>,
        "vee.khee.wong@linux.intel.com" <vee.khee.wong@linux.intel.com>,
        "vee.khee.wong@intel.com" <vee.khee.wong@intel.com>,
        "linux-stm32@st-md-mailman.stormreply.com" 
        <linux-stm32@st-md-mailman.stormreply.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RESEND PATCH net-next v5 1/3] net: stmmac: split xPCS setup from
 mdio register
Thread-Topic: [RESEND PATCH net-next v5 1/3] net: stmmac: split xPCS setup
 from mdio register
Thread-Index: AQHXWTENJC5Z8so33US6hBvpd1LPX6sDvaOA
Date:   Fri, 4 Jun 2021 11:51:54 +0000
Message-ID: <20210604115153.pux7qbrybs4ckg4j@skbuf>
References: <20210604105733.31092-1-michael.wei.hong.sit@intel.com>
 <20210604105733.31092-2-michael.wei.hong.sit@intel.com>
In-Reply-To: <20210604105733.31092-2-michael.wei.hong.sit@intel.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <214EC5EA82E94F4EB763362AE59AC680@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 3ce3843e-57f6-4f96-0640-08d9274f2664
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Jun 2021 11:51:54.8735
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 7bw812ORGrLxsd0FyYmf1GHYeNT9Unzrn5MG5omGcZUjImDDF0yy4Q0OpkRUclsSyDDxSfzcig6dyS9XiHcwiA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB2798
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Jun 04, 2021 at 06:57:31PM +0800, Michael Sit Wei Hong wrote:
> From: Voon Weifeng <weifeng.voon@intel.com>
>=20
> This patch is a preparation patch for the enabling of Intel mGbE 2.5Gbps
> link speed. The Intel mGbR link speed configuration (1G/2.5G) is depends =
on
> a mdio ADHOC register which can be configured in the bios menu.
> As PHY interface might be different for 1G and 2.5G, the mdio bus need be
> ready to check the link speed and select the PHY interface before probing
> the xPCS.
>=20
> Signed-off-by: Voon Weifeng <weifeng.voon@intel.com>
> Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 +
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  7 ++
>  .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 64 ++++++++++---------
>  3 files changed, 43 insertions(+), 29 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/e=
thernet/stmicro/stmmac/stmmac.h
> index b6cd43eda7ac..fd7212afc543 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -311,6 +311,7 @@ enum stmmac_state {
>  int stmmac_mdio_unregister(struct net_device *ndev);
>  int stmmac_mdio_register(struct net_device *ndev);
>  int stmmac_mdio_reset(struct mii_bus *mii);
> +int stmmac_xpcs_setup(struct mii_bus *mii);
>  void stmmac_set_ethtool_ops(struct net_device *netdev);
> =20
>  void stmmac_ptp_register(struct stmmac_priv *priv);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/=
net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6d41dd6f9f7a..c1331c07623d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6991,6 +6991,12 @@ int stmmac_dvr_probe(struct device *device,
>  		}
>  	}
> =20
> +	if (priv->plat->mdio_bus_data->has_xpcs) {

stmmac_mdio_register has:

	if (!mdio_bus_data)
		return 0;

which suggests that some platforms might not populate priv->plat->mdio_bus_=
data.

Are you sure it is safe to go straight to dereferencing mdio_bus_data->has_=
xpcs
in the common driver probe function?

> +		ret =3D stmmac_xpcs_setup(priv->mii);
> +		if (ret)
> +			goto error_xpcs_setup;
> +	}
> +
>  	ret =3D stmmac_phy_setup(priv);
>  	if (ret) {
>  		netdev_err(ndev, "failed to setup phy (%d)\n", ret);
> @@ -7027,6 +7033,7 @@ int stmmac_dvr_probe(struct device *device,
>  	unregister_netdev(ndev);
>  error_netdev_register:
>  	phylink_destroy(priv->phylink);
> +error_xpcs_setup:
>  error_phy_setup:
>  	if (priv->hw->pcs !=3D STMMAC_PCS_TBI &&
>  	    priv->hw->pcs !=3D STMMAC_PCS_RTBI)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/=
net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index 6312a152c8ad..bc900e240da2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -397,6 +397,41 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  	return 0;
>  }
> =20
> +int stmmac_xpcs_setup(struct mii_bus *bus)
> +{
> +	int mode, addr;

Can you please sort the variables in decreasing order of line length?
Also, "mode" can be of the phy_interface_t type.

> +	struct net_device *ndev =3D bus->priv;
> +	struct mdio_xpcs_args *xpcs;
> +	struct stmmac_priv *priv;
> +	struct mdio_device *mdiodev;
> +
> +	priv =3D netdev_priv(ndev);
> +	mode =3D priv->plat->phy_interface;
> +
> +	/* Try to probe the XPCS by scanning all addresses. */
> +	for (addr =3D 0; addr < PHY_MAX_ADDR; addr++) {
> +		mdiodev =3D mdio_device_create(bus, addr);
> +		if (IS_ERR(mdiodev))
> +			continue;
> +
> +		xpcs =3D xpcs_create(mdiodev, mode);
> +		if (IS_ERR_OR_NULL(xpcs)) {
> +			mdio_device_free(mdiodev);
> +			continue;
> +		}
> +
> +		priv->hw->xpcs =3D xpcs;
> +		break;
> +	}
> +
> +	if (!priv->hw->xpcs) {
> +		dev_warn(priv->device, "No xPCS found\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * stmmac_mdio_register
>   * @ndev: net device structure
> @@ -501,40 +536,11 @@ int stmmac_mdio_register(struct net_device *ndev)
>  		goto no_phy_found;
>  	}
> =20
> -	/* Try to probe the XPCS by scanning all addresses. */
> -	if (mdio_bus_data->has_xpcs) {
> -		int mode =3D priv->plat->phy_interface;
> -		struct mdio_device *mdiodev;
> -		struct mdio_xpcs_args *xpcs;
> -
> -		for (addr =3D 0; addr < PHY_MAX_ADDR; addr++) {
> -			mdiodev =3D mdio_device_create(new_bus, addr);
> -			if (IS_ERR(mdiodev))
> -				continue;
> -
> -			xpcs =3D xpcs_create(mdiodev, mode);
> -			if (IS_ERR_OR_NULL(xpcs)) {
> -				mdio_device_free(mdiodev);
> -				continue;
> -			}
> -
> -			priv->hw->xpcs =3D xpcs;
> -			break;
> -		}
> -
> -		if (!priv->hw->xpcs) {
> -			dev_warn(dev, "No XPCS found\n");
> -			err =3D -ENODEV;
> -			goto no_xpcs_found;
> -		}
> -	}
> -
>  bus_register_done:
>  	priv->mii =3D new_bus;
> =20
>  	return 0;
> =20
> -no_xpcs_found:
>  no_phy_found:
>  	mdiobus_unregister(new_bus);
>  bus_register_fail:
> --=20
> 2.17.1
> =