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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZIy5jBAS66rqXjAd@corigine.com>
Date:   Fri, 16 Jun 2023 21:35:40 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     "Radu Pirea (NXP OSS)" <radu-nicolae.pirea@....nxp.com>
Cc:     andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, richardcochran@...il.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        sebastian.tobuschat@....com
Subject: Re: [PATCH net-next v1 05/14] net: phy: nxp-c45-tja11xx: prepare the
 ground for TJA1120

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@....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.

> +}
> +

...