After the fixed link support was introduced, it is observed that PHY
no longer attach to the MAC properly.
This patch series fixes the issue and maintains fixed-link capability.
Michael Sit Wei Hong (2):
net: stmmac: fix PHY handle parsing
net: stmmac: move fixed-link support fixup code
.../net/ethernet/stmicro/stmmac/dwmac-intel.c | 11 ---------
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 23 +++++++++++++++++--
2 files changed, 21 insertions(+), 13 deletions(-)
--
2.34.1
xpcs_an_inband value is updated in the speed_mode_2500 function
which turns on the xpcs_an_inband mode.
Moving the fixed-link fixup code to right before phylink setup to
ensure no more fixup will affect the fixed-link mode configurations.
Fixes: 72edaf39fc65 ("stmmac: intel: add phy-mode and fixed-link ACPI _DSD setting support")
Signed-off-by: Michael Sit Wei Hong <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 11 -----------
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15 +++++++++++++++
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 7deb1f817dac..d02db2b529b9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -592,17 +592,6 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
plat->mdio_bus_data->xpcs_an_inband = true;
}
- /* For fixed-link setup, we clear xpcs_an_inband */
- if (fwnode) {
- struct fwnode_handle *fixed_node;
-
- fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link");
- if (fixed_node)
- plat->mdio_bus_data->xpcs_an_inband = false;
-
- fwnode_handle_put(fixed_node);
- }
-
/* Ensure mdio bus scan skips intel serdes and pcs-xpcs */
plat->mdio_bus_data->phy_mask = 1 << INTEL_MGBE_ADHOC_ADDR;
plat->mdio_bus_data->phy_mask |= 1 << INTEL_MGBE_XPCS_ADDR;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 398adcd68ee8..5a9abafba490 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7064,6 +7064,7 @@ int stmmac_dvr_probe(struct device *device,
struct stmmac_resources *res)
{
struct net_device *ndev = NULL;
+ struct fwnode_handle *fwnode;
struct stmmac_priv *priv;
u32 rxq;
int i, ret = 0;
@@ -7306,6 +7307,20 @@ int stmmac_dvr_probe(struct device *device,
goto error_xpcs_setup;
}
+ /* For fixed-link setup, we clear xpcs_an_inband */
+ if (!fwnode)
+ fwnode = dev_fwnode(priv->device);
+
+ if (fwnode) {
+ struct fwnode_handle *fixed_node;
+
+ fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link");
+ if (fixed_node)
+ priv->plat->mdio_bus_data->xpcs_an_inband = false;
+
+ fwnode_handle_put(fixed_node);
+ }
+
ret = stmmac_phy_setup(priv);
if (ret) {
netdev_err(ndev, "failed to setup phy (%d)\n", ret);
--
2.34.1
phylink_fwnode_phy_connect returns 0 when set to MLO_AN_INBAND.
This causes the PHY handle parsing to skip and the PHY will not be attached
to the MAC.
Add additional check for PHY handle parsing when set to MLO_AN_INBAND.
Fixes: ab21cf920928 ("net: stmmac: make mdio register skips PHY scanning for fixed-link")
Signed-off-by: Michael Sit Wei Hong <[email protected]>
Signed-off-by: Lai Peter Jun Ann <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8f543c3ab5c5..398adcd68ee8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1134,6 +1134,7 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv)
static int stmmac_init_phy(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
+ struct fwnode_handle *fixed_node;
struct fwnode_handle *fwnode;
int ret;
@@ -1141,13 +1142,16 @@ static int stmmac_init_phy(struct net_device *dev)
if (!fwnode)
fwnode = dev_fwnode(priv->device);
- if (fwnode)
+ if (fwnode) {
+ fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link");
+ fwnode_handle_put(fixed_node);
ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0);
+ }
/* Some DT bindings do not set-up the PHY handle. Let's try to
* manually parse it
*/
- if (!fwnode || ret) {
+ if (!fwnode || ret || !fixed_node) {
int addr = priv->plat->phy_addr;
struct phy_device *phydev;
--
2.34.1
Hi Michael,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net/master]
url: https://github.com/intel-lab-lkp/linux/commits/Michael-Sit-Wei-Hong/net-stmmac-fix-PHY-handle-parsing/20230313-160802
patch link: https://lore.kernel.org/r/20230313080135.2952774-3-michael.wei.hong.sit%40intel.com
patch subject: [PATCH net 2/2] net: stmmac: move fixed-link support fixup code
config: x86_64-randconfig-a013-20230313 (https://download.01.org/0day-ci/archive/20230313/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/e1abd092fb1e75931e37147152b274e0d3e6e655
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Michael-Sit-Wei-Hong/net-stmmac-fix-PHY-handle-parsing/20230313-160802
git checkout e1abd092fb1e75931e37147152b274e0d3e6e655
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/ethernet/stmicro/stmmac/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:7311:7: warning: variable 'fwnode' is uninitialized when used here [-Wuninitialized]
if (!fwnode)
^~~~~~
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:7067:30: note: initialize the variable 'fwnode' to silence this warning
struct fwnode_handle *fwnode;
^
= NULL
1 warning generated.
vim +/fwnode +7311 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
7051
7052 /**
7053 * stmmac_dvr_probe
7054 * @device: device pointer
7055 * @plat_dat: platform data pointer
7056 * @res: stmmac resource pointer
7057 * Description: this is the main probe function used to
7058 * call the alloc_etherdev, allocate the priv structure.
7059 * Return:
7060 * returns 0 on success, otherwise errno.
7061 */
7062 int stmmac_dvr_probe(struct device *device,
7063 struct plat_stmmacenet_data *plat_dat,
7064 struct stmmac_resources *res)
7065 {
7066 struct net_device *ndev = NULL;
7067 struct fwnode_handle *fwnode;
7068 struct stmmac_priv *priv;
7069 u32 rxq;
7070 int i, ret = 0;
7071
7072 ndev = devm_alloc_etherdev_mqs(device, sizeof(struct stmmac_priv),
7073 MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES);
7074 if (!ndev)
7075 return -ENOMEM;
7076
7077 SET_NETDEV_DEV(ndev, device);
7078
7079 priv = netdev_priv(ndev);
7080 priv->device = device;
7081 priv->dev = ndev;
7082
7083 stmmac_set_ethtool_ops(ndev);
7084 priv->pause = pause;
7085 priv->plat = plat_dat;
7086 priv->ioaddr = res->addr;
7087 priv->dev->base_addr = (unsigned long)res->addr;
7088 priv->plat->dma_cfg->multi_msi_en = priv->plat->multi_msi_en;
7089
7090 priv->dev->irq = res->irq;
7091 priv->wol_irq = res->wol_irq;
7092 priv->lpi_irq = res->lpi_irq;
7093 priv->sfty_ce_irq = res->sfty_ce_irq;
7094 priv->sfty_ue_irq = res->sfty_ue_irq;
7095 for (i = 0; i < MTL_MAX_RX_QUEUES; i++)
7096 priv->rx_irq[i] = res->rx_irq[i];
7097 for (i = 0; i < MTL_MAX_TX_QUEUES; i++)
7098 priv->tx_irq[i] = res->tx_irq[i];
7099
7100 if (!is_zero_ether_addr(res->mac))
7101 eth_hw_addr_set(priv->dev, res->mac);
7102
7103 dev_set_drvdata(device, priv->dev);
7104
7105 /* Verify driver arguments */
7106 stmmac_verify_args();
7107
7108 priv->af_xdp_zc_qps = bitmap_zalloc(MTL_MAX_TX_QUEUES, GFP_KERNEL);
7109 if (!priv->af_xdp_zc_qps)
7110 return -ENOMEM;
7111
7112 /* Allocate workqueue */
7113 priv->wq = create_singlethread_workqueue("stmmac_wq");
7114 if (!priv->wq) {
7115 dev_err(priv->device, "failed to create workqueue\n");
7116 ret = -ENOMEM;
7117 goto error_wq_init;
7118 }
7119
7120 INIT_WORK(&priv->service_task, stmmac_service_task);
7121
7122 /* Initialize Link Partner FPE workqueue */
7123 INIT_WORK(&priv->fpe_task, stmmac_fpe_lp_task);
7124
7125 /* Override with kernel parameters if supplied XXX CRS XXX
7126 * this needs to have multiple instances
7127 */
7128 if ((phyaddr >= 0) && (phyaddr <= 31))
7129 priv->plat->phy_addr = phyaddr;
7130
7131 if (priv->plat->stmmac_rst) {
7132 ret = reset_control_assert(priv->plat->stmmac_rst);
7133 reset_control_deassert(priv->plat->stmmac_rst);
7134 /* Some reset controllers have only reset callback instead of
7135 * assert + deassert callbacks pair.
7136 */
7137 if (ret == -ENOTSUPP)
7138 reset_control_reset(priv->plat->stmmac_rst);
7139 }
7140
7141 ret = reset_control_deassert(priv->plat->stmmac_ahb_rst);
7142 if (ret == -ENOTSUPP)
7143 dev_err(priv->device, "unable to bring out of ahb reset: %pe\n",
7144 ERR_PTR(ret));
7145
7146 /* Init MAC and get the capabilities */
7147 ret = stmmac_hw_init(priv);
7148 if (ret)
7149 goto error_hw_init;
7150
7151 /* Only DWMAC core version 5.20 onwards supports HW descriptor prefetch.
7152 */
7153 if (priv->synopsys_id < DWMAC_CORE_5_20)
7154 priv->plat->dma_cfg->dche = false;
7155
7156 stmmac_check_ether_addr(priv);
7157
7158 ndev->netdev_ops = &stmmac_netdev_ops;
7159
7160 ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
7161 NETIF_F_RXCSUM;
7162 ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
7163 NETDEV_XDP_ACT_XSK_ZEROCOPY |
7164 NETDEV_XDP_ACT_NDO_XMIT;
7165
7166 ret = stmmac_tc_init(priv, priv);
7167 if (!ret) {
7168 ndev->hw_features |= NETIF_F_HW_TC;
7169 }
7170
7171 if ((priv->plat->tso_en) && (priv->dma_cap.tsoen)) {
7172 ndev->hw_features |= NETIF_F_TSO | NETIF_F_TSO6;
7173 if (priv->plat->has_gmac4)
7174 ndev->hw_features |= NETIF_F_GSO_UDP_L4;
7175 priv->tso = true;
7176 dev_info(priv->device, "TSO feature enabled\n");
7177 }
7178
7179 if (priv->dma_cap.sphen && !priv->plat->sph_disable) {
7180 ndev->hw_features |= NETIF_F_GRO;
7181 priv->sph_cap = true;
7182 priv->sph = priv->sph_cap;
7183 dev_info(priv->device, "SPH feature enabled\n");
7184 }
7185
7186 /* The current IP register MAC_HW_Feature1[ADDR64] only define
7187 * 32/40/64 bit width, but some SOC support others like i.MX8MP
7188 * support 34 bits but it map to 40 bits width in MAC_HW_Feature1[ADDR64].
7189 * So overwrite dma_cap.addr64 according to HW real design.
7190 */
7191 if (priv->plat->addr64)
7192 priv->dma_cap.addr64 = priv->plat->addr64;
7193
7194 if (priv->dma_cap.addr64) {
7195 ret = dma_set_mask_and_coherent(device,
7196 DMA_BIT_MASK(priv->dma_cap.addr64));
7197 if (!ret) {
7198 dev_info(priv->device, "Using %d bits DMA width\n",
7199 priv->dma_cap.addr64);
7200
7201 /*
7202 * If more than 32 bits can be addressed, make sure to
7203 * enable enhanced addressing mode.
7204 */
7205 if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT))
7206 priv->plat->dma_cfg->eame = true;
7207 } else {
7208 ret = dma_set_mask_and_coherent(device, DMA_BIT_MASK(32));
7209 if (ret) {
7210 dev_err(priv->device, "Failed to set DMA Mask\n");
7211 goto error_hw_init;
7212 }
7213
7214 priv->dma_cap.addr64 = 32;
7215 }
7216 }
7217
7218 ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
7219 ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
7220 #ifdef STMMAC_VLAN_TAG_USED
7221 /* Both mac100 and gmac support receive VLAN tag detection */
7222 ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX;
7223 if (priv->dma_cap.vlhash) {
7224 ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
7225 ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER;
7226 }
7227 if (priv->dma_cap.vlins) {
7228 ndev->features |= NETIF_F_HW_VLAN_CTAG_TX;
7229 if (priv->dma_cap.dvlan)
7230 ndev->features |= NETIF_F_HW_VLAN_STAG_TX;
7231 }
7232 #endif
7233 priv->msg_enable = netif_msg_init(debug, default_msg_level);
7234
7235 /* Initialize RSS */
7236 rxq = priv->plat->rx_queues_to_use;
7237 netdev_rss_key_fill(priv->rss.key, sizeof(priv->rss.key));
7238 for (i = 0; i < ARRAY_SIZE(priv->rss.table); i++)
7239 priv->rss.table[i] = ethtool_rxfh_indir_default(i, rxq);
7240
7241 if (priv->dma_cap.rssen && priv->plat->rss_en)
7242 ndev->features |= NETIF_F_RXHASH;
7243
7244 /* MTU range: 46 - hw-specific max */
7245 ndev->min_mtu = ETH_ZLEN - ETH_HLEN;
7246 if (priv->plat->has_xgmac)
7247 ndev->max_mtu = XGMAC_JUMBO_LEN;
7248 else if ((priv->plat->enh_desc) || (priv->synopsys_id >= DWMAC_CORE_4_00))
7249 ndev->max_mtu = JUMBO_LEN;
7250 else
7251 ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);
7252 /* Will not overwrite ndev->max_mtu if plat->maxmtu > ndev->max_mtu
7253 * as well as plat->maxmtu < ndev->min_mtu which is a invalid range.
7254 */
7255 if ((priv->plat->maxmtu < ndev->max_mtu) &&
7256 (priv->plat->maxmtu >= ndev->min_mtu))
7257 ndev->max_mtu = priv->plat->maxmtu;
7258 else if (priv->plat->maxmtu < ndev->min_mtu)
7259 dev_warn(priv->device,
7260 "%s: warning: maxmtu having invalid value (%d)\n",
7261 __func__, priv->plat->maxmtu);
7262
7263 if (flow_ctrl)
7264 priv->flow_ctrl = FLOW_AUTO; /* RX/TX pause on */
7265
7266 /* Setup channels NAPI */
7267 stmmac_napi_add(ndev);
7268
7269 mutex_init(&priv->lock);
7270
7271 /* If a specific clk_csr value is passed from the platform
7272 * this means that the CSR Clock Range selection cannot be
7273 * changed at run-time and it is fixed. Viceversa the driver'll try to
7274 * set the MDC clock dynamically according to the csr actual
7275 * clock input.
7276 */
7277 if (priv->plat->clk_csr >= 0)
7278 priv->clk_csr = priv->plat->clk_csr;
7279 else
7280 stmmac_clk_csr_set(priv);
7281
7282 stmmac_check_pcs_mode(priv);
7283
7284 pm_runtime_get_noresume(device);
7285 pm_runtime_set_active(device);
7286 if (!pm_runtime_enabled(device))
7287 pm_runtime_enable(device);
7288
7289 if (priv->hw->pcs != STMMAC_PCS_TBI &&
7290 priv->hw->pcs != STMMAC_PCS_RTBI) {
7291 /* MDIO bus Registration */
7292 ret = stmmac_mdio_register(ndev);
7293 if (ret < 0) {
7294 dev_err_probe(priv->device, ret,
7295 "%s: MDIO bus (id: %d) registration failed\n",
7296 __func__, priv->plat->bus_id);
7297 goto error_mdio_register;
7298 }
7299 }
7300
7301 if (priv->plat->speed_mode_2500)
7302 priv->plat->speed_mode_2500(ndev, priv->plat->bsp_priv);
7303
7304 if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) {
7305 ret = stmmac_xpcs_setup(priv->mii);
7306 if (ret)
7307 goto error_xpcs_setup;
7308 }
7309
7310 /* For fixed-link setup, we clear xpcs_an_inband */
> 7311 if (!fwnode)
7312 fwnode = dev_fwnode(priv->device);
7313
7314 if (fwnode) {
7315 struct fwnode_handle *fixed_node;
7316
7317 fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link");
7318 if (fixed_node)
7319 priv->plat->mdio_bus_data->xpcs_an_inband = false;
7320
7321 fwnode_handle_put(fixed_node);
7322 }
7323
7324 ret = stmmac_phy_setup(priv);
7325 if (ret) {
7326 netdev_err(ndev, "failed to setup phy (%d)\n", ret);
7327 goto error_phy_setup;
7328 }
7329
7330 ret = register_netdev(ndev);
7331 if (ret) {
7332 dev_err(priv->device, "%s: ERROR %i registering the device\n",
7333 __func__, ret);
7334 goto error_netdev_register;
7335 }
7336
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Mon, Mar 13, 2023 at 04:01:34PM +0800, Michael Sit Wei Hong wrote:
> phylink_fwnode_phy_connect returns 0 when set to MLO_AN_INBAND.
> This causes the PHY handle parsing to skip and the PHY will not be attached
> to the MAC.
>
> Add additional check for PHY handle parsing when set to MLO_AN_INBAND.
>
> Fixes: ab21cf920928 ("net: stmmac: make mdio register skips PHY scanning for fixed-link")
> Signed-off-by: Michael Sit Wei Hong <[email protected]>
> Signed-off-by: Lai Peter Jun Ann <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8f543c3ab5c5..398adcd68ee8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1134,6 +1134,7 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv)
> static int stmmac_init_phy(struct net_device *dev)
> {
> struct stmmac_priv *priv = netdev_priv(dev);
> + struct fwnode_handle *fixed_node;
> struct fwnode_handle *fwnode;
> int ret;
>
> @@ -1141,13 +1142,16 @@ static int stmmac_init_phy(struct net_device *dev)
> if (!fwnode)
> fwnode = dev_fwnode(priv->device);
>
> - if (fwnode)
> + if (fwnode) {
> + fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link");
> + fwnode_handle_put(fixed_node);
> ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0);
> + }
>
On the occasion, why not rewrite above to:
if (!fwnode)
...
else
...
or:
if(fwnode)
...
else
?
> /* Some DT bindings do not set-up the PHY handle. Let's try to
> * manually parse it
> */
> - if (!fwnode || ret) {
> + if (!fwnode || ret || !fixed_node) {
> int addr = priv->plat->phy_addr;
> struct phy_device *phydev;
>
> --
> 2.34.1
>
> -----Original Message-----
> From: Raczynski, Piotr <[email protected]>
> Sent: Tuesday, March 14, 2023 4:39 PM
> To: Sit, Michael Wei Hong <[email protected]>
> Cc: Giuseppe Cavallaro <[email protected]>; Alexandre
> Torgue <[email protected]>; Jose Abreu
> <[email protected]>; David S . Miller <[email protected]>;
> Eric Dumazet <[email protected]>; Jakub Kicinski
> <[email protected]>; Paolo Abeni <[email protected]>; Maxime
> Coquelin <[email protected]>; Ong, Boon Leong
> <[email protected]>; [email protected]; linux-
> [email protected]; linux-arm-
> [email protected]; [email protected]; Looi,
> Hong Aun <[email protected]>; Voon, Weifeng
> <[email protected]>; Lai, Peter Jun Ann
> <[email protected]>
> Subject: Re: [PATCH net 1/2] net: stmmac: fix PHY handle parsing
>
> On Mon, Mar 13, 2023 at 04:01:34PM +0800, Michael Sit Wei Hong
> wrote:
> > phylink_fwnode_phy_connect returns 0 when set to
> MLO_AN_INBAND.
> > This causes the PHY handle parsing to skip and the PHY will not be
> > attached to the MAC.
> >
> > Add additional check for PHY handle parsing when set to
> MLO_AN_INBAND.
> >
> > Fixes: ab21cf920928 ("net: stmmac: make mdio register skips PHY
> > scanning for fixed-link")
> > Signed-off-by: Michael Sit Wei Hong
> <[email protected]>
> > Signed-off-by: Lai Peter Jun Ann <[email protected]>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8
> ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 8f543c3ab5c5..398adcd68ee8 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1134,6 +1134,7 @@ static void
> stmmac_check_pcs_mode(struct
> > stmmac_priv *priv) static int stmmac_init_phy(struct net_device
> *dev)
> > {
> > struct stmmac_priv *priv = netdev_priv(dev);
> > + struct fwnode_handle *fixed_node;
> > struct fwnode_handle *fwnode;
> > int ret;
> >
> > @@ -1141,13 +1142,16 @@ static int stmmac_init_phy(struct
> net_device *dev)
> > if (!fwnode)
> > fwnode = dev_fwnode(priv->device);
> >
> > - if (fwnode)
> > + if (fwnode) {
> > + fixed_node =
> fwnode_get_named_child_node(fwnode, "fixed-link");
> > + fwnode_handle_put(fixed_node);
> > ret = phylink_fwnode_phy_connect(priv->phylink,
> fwnode, 0);
> > + }
> >
>
> On the occasion, why not rewrite above to:
> if (!fwnode)
> ...
> else
> ...
>
> or:
> if(fwnode)
> ...
> else
> ?
>
The (!fwnode) serves as a NULL check, and if the fwnode is NULL, we try to populate it using dev_fwnode.
If fwnode is then populated, then the if(fwnode) code will run.
If fwnode is still NULL after dev_fwnode, fwnode_get_named_child_node will generate a kernel panic if no
NULL check is present.
> > /* Some DT bindings do not set-up the PHY handle. Let's try
> to
> > * manually parse it
> > */
> > - if (!fwnode || ret) {
> > + if (!fwnode || ret || !fixed_node) {
> > int addr = priv->plat->phy_addr;
> > struct phy_device *phydev;
> >
> > --
> > 2.34.1
> >
On Mon, Mar 13, 2023 at 04:01:35PM +0800, Michael Sit Wei Hong wrote:
> xpcs_an_inband value is updated in the speed_mode_2500 function
> which turns on the xpcs_an_inband mode.
>
> Moving the fixed-link fixup code to right before phylink setup to
> ensure no more fixup will affect the fixed-link mode configurations.
>
> Fixes: 72edaf39fc65 ("stmmac: intel: add phy-mode and fixed-link ACPI _DSD setting support")
> Signed-off-by: Michael Sit Wei Hong <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 11 -----------
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15 +++++++++++++++
> 2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 7deb1f817dac..d02db2b529b9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -592,17 +592,6 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
> plat->mdio_bus_data->xpcs_an_inband = true;
> }
>
> - /* For fixed-link setup, we clear xpcs_an_inband */
> - if (fwnode) {
> - struct fwnode_handle *fixed_node;
> -
> - fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link");
> - if (fixed_node)
> - plat->mdio_bus_data->xpcs_an_inband = false;
> -
> - fwnode_handle_put(fixed_node);
> - }
> -
> /* Ensure mdio bus scan skips intel serdes and pcs-xpcs */
> plat->mdio_bus_data->phy_mask = 1 << INTEL_MGBE_ADHOC_ADDR;
> plat->mdio_bus_data->phy_mask |= 1 << INTEL_MGBE_XPCS_ADDR;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 398adcd68ee8..5a9abafba490 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7064,6 +7064,7 @@ int stmmac_dvr_probe(struct device *device,
> struct stmmac_resources *res)
> {
> struct net_device *ndev = NULL;
> + struct fwnode_handle *fwnode;
> struct stmmac_priv *priv;
> u32 rxq;
> int i, ret = 0;
> @@ -7306,6 +7307,20 @@ int stmmac_dvr_probe(struct device *device,
> goto error_xpcs_setup;
> }
>
> + /* For fixed-link setup, we clear xpcs_an_inband */
> + if (!fwnode)
> + fwnode = dev_fwnode(priv->device);
> +
> + if (fwnode) {
> + struct fwnode_handle *fixed_node;
> +
> + fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link");
> + if (fixed_node)
> + priv->plat->mdio_bus_data->xpcs_an_inband = false;
> +
> + fwnode_handle_put(fixed_node);
> + }
> +
Now you're doing similar checks here and inside stmmac_init_phy. Maybe
you could combined this to some function?
Piotr
> ret = stmmac_phy_setup(priv);
> if (ret) {
> netdev_err(ndev, "failed to setup phy (%d)\n", ret);
> --
> 2.34.1
>
> -----Original Message-----
> From: Raczynski, Piotr <[email protected]>
> Sent: Tuesday, March 14, 2023 5:02 PM
> To: Sit, Michael Wei Hong <[email protected]>
> Cc: Giuseppe Cavallaro <[email protected]>; Alexandre
> Torgue <[email protected]>; Jose Abreu
> <[email protected]>; David S . Miller <[email protected]>;
> Eric Dumazet <[email protected]>; Jakub Kicinski
> <[email protected]>; Paolo Abeni <[email protected]>; Maxime
> Coquelin <[email protected]>; Ong, Boon Leong
> <[email protected]>; [email protected]; linux-
> [email protected]; linux-arm-
> [email protected]; [email protected]; Looi,
> Hong Aun <[email protected]>; Voon, Weifeng
> <[email protected]>; Lai, Peter Jun Ann
> <[email protected]>
> Subject: Re: [PATCH net 2/2] net: stmmac: move fixed-link support
> fixup code
>
> On Mon, Mar 13, 2023 at 04:01:35PM +0800, Michael Sit Wei Hong
> wrote:
> > xpcs_an_inband value is updated in the speed_mode_2500
> function which
> > turns on the xpcs_an_inband mode.
> >
> > Moving the fixed-link fixup code to right before phylink setup to
> > ensure no more fixup will affect the fixed-link mode configurations.
> >
> > Fixes: 72edaf39fc65 ("stmmac: intel: add phy-mode and fixed-link
> ACPI
> > _DSD setting support")
> > Signed-off-by: Michael Sit Wei Hong
> <[email protected]>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 11 ---------
> --
> > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15
> +++++++++++++++
> > 2 files changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > index 7deb1f817dac..d02db2b529b9 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > @@ -592,17 +592,6 @@ static int
> intel_mgbe_common_data(struct pci_dev *pdev,
> > plat->mdio_bus_data->xpcs_an_inband = true;
> > }
> >
> > - /* For fixed-link setup, we clear xpcs_an_inband */
> > - if (fwnode) {
> > - struct fwnode_handle *fixed_node;
> > -
> > - fixed_node =
> fwnode_get_named_child_node(fwnode, "fixed-link");
> > - if (fixed_node)
> > - plat->mdio_bus_data->xpcs_an_inband =
> false;
> > -
> > - fwnode_handle_put(fixed_node);
> > - }
> > -
> > /* Ensure mdio bus scan skips intel serdes and pcs-xpcs */
> > plat->mdio_bus_data->phy_mask = 1 <<
> INTEL_MGBE_ADHOC_ADDR;
> > plat->mdio_bus_data->phy_mask |= 1 <<
> INTEL_MGBE_XPCS_ADDR; diff
> > --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 398adcd68ee8..5a9abafba490 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -7064,6 +7064,7 @@ int stmmac_dvr_probe(struct device
> *device,
> > struct stmmac_resources *res)
> > {
> > struct net_device *ndev = NULL;
> > + struct fwnode_handle *fwnode;
> > struct stmmac_priv *priv;
> > u32 rxq;
> > int i, ret = 0;
> > @@ -7306,6 +7307,20 @@ int stmmac_dvr_probe(struct device
> *device,
> > goto error_xpcs_setup;
> > }
> >
> > + /* For fixed-link setup, we clear xpcs_an_inband */
> > + if (!fwnode)
> > + fwnode = dev_fwnode(priv->device);
> > +
> > + if (fwnode) {
> > + struct fwnode_handle *fixed_node;
> > +
> > + fixed_node =
> fwnode_get_named_child_node(fwnode, "fixed-link");
> > + if (fixed_node)
> > + priv->plat->mdio_bus_data-
> >xpcs_an_inband = false;
> > +
> > + fwnode_handle_put(fixed_node);
> > + }
> > +
>
> Now you're doing similar checks here and inside stmmac_init_phy.
> Maybe you could combined this to some function?
>
> Piotr
In stmmac_dvr_probe, the check for fixed-link is to fixup the xpcs_an_inband
data. Whereas in stmmac_init_phy, we store the fixed_node info to
determine if we need to setup the PHY handle manually in the later code,
and will run phylink_fwnode_phy_connect which uses the fwnode variable.
The NULL check on fwnode is to ensure the functions that uses the fwnode
does not access NULL pointers. It might be difficult to combine it into a
function due to the way fixed_node is used.
>
> > ret = stmmac_phy_setup(priv);
> > if (ret) {
> > netdev_err(ndev, "failed to setup phy (%d)\n", ret);
> > --
> > 2.34.1
> >