Re: [PATCH net-next v1 05/14] net: phy: nxp-c45-tja11xx: prepare the ground for TJA1120

[Simon Horman <simon.horman@corigine.com>]

On Fri, Jun 16, 2023 at 04:53:14PM +0300, Radu Pirea (NXP OSS) wrote:
> Remove the defined bits and register addresses that are not common
> between TJA1103 and TJA1120 and replace them with reg_fields and
> register addresses from phydev->drv->driver_data.
> 
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>

Hi Radu,

thanks for your patch-set.

Some minor points from my side.

> @@ -831,27 +862,26 @@ static int nxp_c45_hwtstamp(struct mii_timestamper *mii_ts,
>  	}
>  
>  	if (priv->hwts_rx || priv->hwts_tx) {
> -		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_EVENT_MSG_FILT,
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			      data->regmap->vend1_event_msg_filt,
>  			      EVENT_MSG_FILT_ALL);
> -		phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> -				   VEND1_PORT_PTP_CONTROL,
> -				   PORT_PTP_CONTROL_BYPASS);
> +		if (data && data->ptp_enable)
> +			data->ptp_enable(phydev, true);

A check for data being null is made here before dereferencing.
But a few lines above data is dereferenced without such a guard.

This is flagged by Smatch as:

 .../nxp-c45-tja11xx.c:868 nxp_c45_hwtstamp() warn: variable dereferenced before check 'data' (see line 866)`

>  	} else {
> -		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_EVENT_MSG_FILT,
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			      data->regmap->vend1_event_msg_filt,
>  			      EVENT_MSG_FILT_NONE);
> -		phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PORT_PTP_CONTROL,
> -				 PORT_PTP_CONTROL_BYPASS);
> +		if (data && data->ptp_enable)
> +			data->ptp_enable(phydev, false);

Likewise here.

>  	}
>  
>  	if (nxp_c45_poll_txts(priv->phydev))
>  		goto nxp_c45_no_ptp_irq;
>  
>  	if (priv->hwts_tx)
> -		phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> -				 VEND1_PTP_IRQ_EN, PTP_IRQ_EGR_TS);
> +		nxp_c45_set_reg_field(phydev, &data->regmap->irq_egr_ts_en);
>  	else
> -		phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> -				   VEND1_PTP_IRQ_EN, PTP_IRQ_EGR_TS);
> +		nxp_c45_clear_reg_field(phydev, &data->regmap->irq_egr_ts_en);
>  
>  nxp_c45_no_ptp_irq:
>  	return copy_to_user(ifreq->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;

...

> +static void nxp_c45_ptp_init(struct phy_device *phydev)
> +{
> +	const struct nxp_c45_phy_data *data = nxp_c45_get_data(phydev);
> +
> +	phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +		      data->regmap->vend1_ptp_clk_period,
> +		      data->ptp_clk_period);
> +	nxp_c45_clear_reg_field(phydev, &data->regmap->ltc_lock_ctrl);
> +
> +	if (data && data->ptp_init)
> +		data->ptp_init(phydev);

And here.

> +}
> +

...