Re: [PATCH net-next v7 4/7] stmmac: intel: configure SerDes according to the interface mode

Choong Yong Liang
2025-02-07T09:53:49Z

On 6/2/2025 9:31 pm, Ilpo Järvinen wrote:
>> +static int intel_tsn_lane_is_available(struct net_device *ndev,
>> + struct intel_priv_data *intel_priv)
>> +{
>> + struct stmmac_priv *priv = netdev_priv(ndev);
>> + struct pmc_ipc_cmd tmp = {};
>> + u32 rbuf[4] = {};
>> + int ret = 0, i, j;
>> + const int max_fia_regs = 5;
>> +
>> + tmp.cmd = IPC_SOC_REGISTER_ACCESS;
>> + tmp.sub_cmd = IPC_SOC_SUB_CMD_READ;
>> +
>> + for (i = 0; i < max_fia_regs; i++) {
>
> Usually, defines are used for true consts.
>
Hi Ilpo,
Thank you for your feedback. I used const int max_fia_regs = 5; to leverage
type safety and scope control in modern C. However, I understand that
#define is a common practice. Please let me know if you prefer I switch to
#define for consistency.

>> +static int intel_mac_finish(struct net_device *ndev,
>> + void *intel_data,
>> + unsigned int mode,
>> + phy_interface_t interface)
>> +{
>> + struct intel_priv_data *intel_priv = intel_data;
>> + struct stmmac_priv *priv = netdev_priv(ndev);
>> + const struct pmc_serdes_regs *regs;
>> + int max_regs = 0;
>> + int ret = 0;
>> +
>> + ret = intel_tsn_lane_is_available(ndev, intel_priv);
>> + if (ret < 0) {
>> + netdev_info(priv->dev, "No TSN lane available to set the registers.\n");
>> + return ret;
>> + }
>> +
>> + if (interface == PHY_INTERFACE_MODE_2500BASEX) {
>> + regs = intel_priv->pid_2p5g.regs;
>> + max_regs = intel_priv->pid_2p5g.num_regs;
>> + } else {
>> + regs = intel_priv->pid_1g.regs;
>> + max_regs = intel_priv->pid_1g.num_regs;
>> + }
>> +
>> + ret = intel_set_reg_access(regs, max_regs);
>> + if (ret < 0)
>> + return ret;
>
> This looks much cleaner now, thanks the update.
>
> However, the intel_priv fields you introduced are not setup until patch
> 6/7? Will this cause NULL ptr deref issues in between the two changes? By
> introducing the reg arrays in this patch but only use them after patch 6,
> you'll also get unused variable warnings out of them in between the
> changes which is unacceptable.
>
Thank you for pointing out the potential issues with the intel_priv fields.
I will move the changes from patch 6 into this patch to avoid NULL pointer
de-reference issues and unused variable warnings. This will ensure
everything is properly set up and used within the same patch.