netdev - 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: <20230619075325.ywg4jv6h2hifqjx4@soft-dev3-1>
Date: Mon, 19 Jun 2023 09:53:25 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.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

The 06/16/2023 16:53, Radu Pirea (NXP OSS) wrote:

Hi Radu,

> 
> 
> -struct nxp_c45_phy_stats {
> -       const char      *name;
> -       u8              mmd;
> -       u16             reg;
> -       u8              off;
> -       u16             mask;
> -};
> +static inline

It is recommended not to use inline inside .c files.

> +const struct nxp_c45_phy_data *nxp_c45_get_data(struct phy_device *phydev)
> +{
> +       return phydev->drv->driver_data;
> +}
> +
> +static inline
> +const struct nxp_c45_regmap *nxp_c45_get_regmap(struct phy_device *phydev)
> +{
> +       const struct nxp_c45_phy_data *phy_data = nxp_c45_get_data(phydev);
> +
> +       return phy_data ? phy_data->regmap : NULL;

>From what I can see nxp_c45_get_data(phydev) can't return a NULL pointer
so then I don't think you need the above check. And then maybe you can
remove more of the checks in the code where you check for data to not be
NULL.
The reason why I say nxp_c45_get_data(phydev) can't return a NULL is
because few lines bellow you have:

.driver_data            = &tja1103_phy_data,

...

> +}
> @@ -806,6 +835,7 @@ static int nxp_c45_hwtstamp(struct mii_timestamper *mii_ts,
>         struct nxp_c45_phy *priv = container_of(mii_ts, struct nxp_c45_phy,
>                                                 mii_ts);
>         struct phy_device *phydev = priv->phydev;
> +       const struct nxp_c45_phy_data *data;
>         struct hwtstamp_config cfg;
> 
>         if (copy_from_user(&cfg, ifreq->ifr_data, sizeof(cfg)))
> @@ -814,6 +844,7 @@ static int nxp_c45_hwtstamp(struct mii_timestamper *mii_ts,
>         if (cfg.tx_type < 0 || cfg.tx_type > HWTSTAMP_TX_ON)
>                 return -ERANGE;
> 
> +       data = nxp_c45_get_data(phydev);
>         priv->hwts_tx = cfg.tx_type;
> 
>         switch (cfg.rx_filter) {
> @@ -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)

Like here

> +                       data->ptp_enable(phydev, true);
>         } 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)

And here and few other places bellow:

> +                       data->ptp_enable(phydev, false);
>         }
> 
>         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);
> 

...

> 
> +static const struct nxp_c45_phy_data tja1103_phy_data = {
> +       .regmap = &tja1103_regmap,
> +       .stats = tja1103_hw_stats,
> +       .n_stats = ARRAY_SIZE(tja1103_hw_stats),
> +       .ptp_clk_period = PTP_CLK_PERIOD_100BT1,
> +       .counters_enable = tja1103_counters_enable,
> +       .ptp_init = tja1103_ptp_init,
> +       .ptp_enable = tja1103_ptp_enable,
> +};
> +
>  static struct phy_driver nxp_c45_driver[] = {
>         {
>                 PHY_ID_MATCH_MODEL(PHY_ID_TJA_1103),
>                 .name                   = "NXP C45 TJA1103",
>                 .features               = PHY_BASIC_T1_FEATURES,
> +               .driver_data            = &tja1103_phy_data,
>                 .probe                  = nxp_c45_probe,
>                 .soft_reset             = nxp_c45_soft_reset,
>                 .config_aneg            = genphy_c45_config_aneg,
> --
> 2.34.1
> 
> 

-- 
/Horatiu