Messages in this thread | | | Date | Fri, 16 Jun 2023 21:35:40 +0200 | From | Simon Horman <> | 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@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.
> +} > +
...
| |