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

[Choong Yong Liang <yong.liang.choong@linux.intel.com>]

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.