[bug report] net: stmmac: configure SerDes according to the interface mode

[bug report] net: stmmac: configure SerDes according to the interface mode

[Dan Carpenter]

Hello Choong Yong Liang,

Commit a42f6b3f1cc1 ("net: stmmac: configure SerDes according to the
interface mode") from Feb 27, 2025 (linux-next), leads to the
following Smatch static checker warning:

	drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c:497 intel_tsn_lane_is_available()
	warn: missing error code? 'ret'

drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
    472 static int intel_tsn_lane_is_available(struct net_device *ndev,
    473                                        struct intel_priv_data *intel_priv)
    474 {
    475         struct stmmac_priv *priv = netdev_priv(ndev);
    476         struct pmc_ipc_cmd tmp = {};
    477         struct pmc_ipc_rbuf rbuf = {};
    478         int ret = 0, i, j;

Better to avoid initializing ret so that bug show up as uninitialized
variables.

    479         const int max_fia_regs = 5;
    480 
    481         tmp.cmd = IPC_SOC_REGISTER_ACCESS;
    482         tmp.sub_cmd = IPC_SOC_SUB_CMD_READ;
    483 
    484         for (i = 0; i < max_fia_regs; i++) {
    485                 tmp.wbuf[0] = R_PCH_FIA_15_PCR_LOS1_REG_BASE + i;
    486 
    487                 ret = intel_pmc_ipc(&tmp, &rbuf);
    488                 if (ret < 0) {
    489                         netdev_info(priv->dev, "Failed to read from PMC.\n");
    490                         return ret;
    491                 }
    492 
    493                 for (j = 0; j <= intel_priv->max_tsn_lane_regs; j++)
    494                         if ((rbuf.buf[0] >>
    495                                 (4 * (intel_priv->tsn_lane_regs[j] % 8)) &
    496                                         B_PCH_FIA_PCR_L0O) == 0xB)
--> 497                                 return ret;

This should probably be return -EINVAL, right?

    498         }
    499 
    500         return ret;

It's more clear to just return 0 for the success path.  Otherwise you
have to read the code a bit.

    501 }

regards,
dan carpenter

Re: [bug report] net: stmmac: configure SerDes according to the interface mode

[Choong Yong Liang]

On 8/3/2025 7:17 pm, Dan Carpenter wrote:
> Hello Choong Yong Liang,
> 
> Commit a42f6b3f1cc1 ("net: stmmac: configure SerDes according to the
> interface mode") from Feb 27, 2025 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c:497 intel_tsn_lane_is_available()
> 	warn: missing error code? 'ret'
> 
> drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
>      472 static int intel_tsn_lane_is_available(struct net_device *ndev,
>      473                                        struct intel_priv_data *intel_priv)
>      474 {
>      475         struct stmmac_priv *priv = netdev_priv(ndev);
>      476         struct pmc_ipc_cmd tmp = {};
>      477         struct pmc_ipc_rbuf rbuf = {};
>      478         int ret = 0, i, j;
> 
> Better to avoid initializing ret so that bug show up as uninitialized
> variables.
> 
>      479         const int max_fia_regs = 5;
>      480
>      481         tmp.cmd = IPC_SOC_REGISTER_ACCESS;
>      482         tmp.sub_cmd = IPC_SOC_SUB_CMD_READ;
>      483
>      484         for (i = 0; i < max_fia_regs; i++) {
>      485                 tmp.wbuf[0] = R_PCH_FIA_15_PCR_LOS1_REG_BASE + i;
>      486
>      487                 ret = intel_pmc_ipc(&tmp, &rbuf);
>      488                 if (ret < 0) {
>      489                         netdev_info(priv->dev, "Failed to read from PMC.\n");
>      490                         return ret;
>      491                 }
>      492
>      493                 for (j = 0; j <= intel_priv->max_tsn_lane_regs; j++)
>      494                         if ((rbuf.buf[0] >>
>      495                                 (4 * (intel_priv->tsn_lane_regs[j] % 8)) &
>      496                                         B_PCH_FIA_PCR_L0O) == 0xB)
> --> 497                                 return ret;
> 
> This should probably be return -EINVAL, right?
> 
>      498         }
>      499
>      500         return ret;
> 
> It's more clear to just return 0 for the success path.  Otherwise you
> have to read the code a bit.
> 
>      501 }
> 
> regards,
> dan carpenter

Hello Dan,

Thank you for bringing this to my attention.

The code at line 497 should return 0 to indicate success when the condition 
is met. Additionally, I will modify line 500 to return -EINVAL to clearly 
signify an error when no valid lane is found.

I will prepare and submit a patch to implement these changes. If you have 
any further feedback or suggestions, please feel free to let me know.

Thanks again for your valuable input.

Best regards,
Yong Liang