Re: [PATCH net-next v2 2/5] net: phy: microchip_ptp : Add ptp library for Microchip phys

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 11/11/2024 12:58, Divya Koppera wrote:
> Add ptp library for Microchip phys
> 1-step and 2-step modes are supported, over Ethernet and UDP(ipv4, ipv6)
> 
> Signed-off-by: Divya Koppera <divya.koppera@microchip.com>
> ---
> v1 -> v2
> - Removed redundant memsets
> - Moved to standard comparision than memcmp for u16
> - Fixed sparse/smatch warnings reported by kernel test robot
> - Added spinlock to shared code
> - Moved redundant part of code out of spinlock protected area
> ---
>   drivers/net/phy/microchip_ptp.c | 998 ++++++++++++++++++++++++++++++++
>   1 file changed, 998 insertions(+)
>   create mode 100644 drivers/net/phy/microchip_ptp.c

[..snip..]

> +static struct mchp_ptp_rx_ts *mchp_ptp_get_rx_ts(struct mchp_ptp_clock *ptp_clock)
> +{
> +	struct phy_device *phydev = ptp_clock->phydev;
> +	struct mchp_ptp_rx_ts *rx_ts = NULL;
> +	u32 sec, nsec;
> +	int rc;
> +
> +	rc = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			  MCHP_PTP_RX_INGRESS_NS_HI(BASE_PORT(ptp_clock)));
> +	if (rc < 0)
> +		goto error;
> +	if (!(rc & MCHP_PTP_RX_INGRESS_NS_HI_TS_VALID)) {
> +		phydev_err(phydev, "RX Timestamp is not valid!\n");
> +		goto error;
> +	}
> +	nsec = (rc & GENMASK(13, 0)) << 16;
> +
> +	rc = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			  MCHP_PTP_RX_INGRESS_NS_LO(BASE_PORT(ptp_clock)));
> +	if (rc < 0)
> +		goto error;
> +	nsec |= rc;
> +
> +	rc = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			  MCHP_PTP_RX_INGRESS_SEC_HI(BASE_PORT(ptp_clock)));
> +	if (rc < 0)
> +		goto error;
> +	sec = rc << 16;
> +
> +	rc = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			  MCHP_PTP_RX_INGRESS_SEC_LO(BASE_PORT(ptp_clock)));
> +	if (rc < 0)
> +		goto error;
> +	sec |= rc;
> +
> +	rc = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			  MCHP_PTP_RX_MSG_HEADER2(BASE_PORT(ptp_clock)));
> +	if (rc < 0)
> +		goto error;
> +
> +	rx_ts = kzalloc(sizeof(*rx_ts), GFP_KERNEL);

I think I've asked it already, but why zero out new allocation, which
will be fully re-written by the next instructions? Did you find any
problems?

> +	if (!rx_ts)
> +		return NULL;
> +
> +	rx_ts->seconds = sec;
> +	rx_ts->nsec = nsec;
> +	rx_ts->seq_id = rc;
> +
> +error:
> +	return rx_ts;
> +}
> +
> +static void mchp_ptp_process_rx_ts(struct mchp_ptp_clock *ptp_clock)
> +{
> +	struct phy_device *phydev = ptp_clock->phydev;
> +	int caps;
> +
> +	do {
> +		struct mchp_ptp_rx_ts *rx_ts;
> +
> +		rx_ts = mchp_ptp_get_rx_ts(ptp_clock);
> +		if (rx_ts)
> +			mchp_ptp_match_rx_ts(ptp_clock, rx_ts);
> +
> +		caps = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +				    MCHP_PTP_CAP_INFO(BASE_PORT(ptp_clock)));
> +		if (caps < 0)
> +			return;
> +	} while (MCHP_PTP_RX_TS_CNT(caps) > 0);
> +}
> +