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

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] [day] [month] [year] [list]

Message-ID: <20241107143404.GV4507@kernel.org>
Date: Thu, 7 Nov 2024 14:34:04 +0000
From: Simon Horman <horms@...nel.org>
To: Divya Koppera <divya.koppera@...rochip.com>
Cc: andrew@...n.ch, arun.ramadoss@...rochip.com,
	UNGLinuxDriver@...rochip.com, hkallweit1@...il.com,
	linux@...linux.org.uk, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, richardcochran@...il.com
Subject: Re: [PATCH net-next 2/5] net: phy: microchip_ptp : Add ptp library
 for Microchip phys

On Mon, Nov 04, 2024 at 02:37:47PM +0530, 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@...rochip.com>
> ---
>  drivers/net/phy/microchip_ptp.c | 990 ++++++++++++++++++++++++++++++++
>  1 file changed, 990 insertions(+)
>  create mode 100644 drivers/net/phy/microchip_ptp.c
> 
> diff --git a/drivers/net/phy/microchip_ptp.c b/drivers/net/phy/microchip_ptp.c

...

> +static bool mchp_ptp_get_sig_tx(struct sk_buff *skb, u16 *sig)
> +{
> +	struct ptp_header *ptp_header;
> +	int type;
> +
> +	type = ptp_classify_raw(skb);
> +	if (type == PTP_CLASS_NONE)
> +		return false;
> +
> +	ptp_header = ptp_parse_header(skb, type);
> +	if (!ptp_header)
> +		return false;
> +
> +	*sig = htons(ptp_header->sequence_id);

Hi Divya,

The type of *sig is u16, a host-byte order integer.
But htons() returns __be16, a big-endian integer.
This does not seem right.

Likewise, in the caller, and beyond, if these are big-endian integers
then appropriate types - probably __be16 - should be used.

Flagged by Sparse.

> +
> +	return true;
> +}

...

> +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;
> +	u16 seq;
> +	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;
> +
> +	seq = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			   MCHP_PTP_RX_MSG_HEADER2(BASE_PORT(ptp_clock)));
> +	if (seq < 0)

seq is unsigned; it can never be less than 0.

Flagged by Smatch.

> +		goto error;
> +
> +	rx_ts = kzalloc(sizeof(*rx_ts), GFP_KERNEL);
> +	if (!rx_ts)
> +		return NULL;
> +
> +	rx_ts->seconds = sec;
> +	rx_ts->nsec = nsec;
> +	rx_ts->seq_id = seq;
> +
> +error:
> +	return rx_ts;
> +}

...

> +static bool mchp_ptp_get_tx_ts(struct mchp_ptp_clock *ptp_clock,
> +			       u32 *sec, u32 *nsec, u16 *seq)
> +{
> +	struct phy_device *phydev = ptp_clock->phydev;
> +	int rc;
> +
> +	rc = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			  MCHP_PTP_TX_EGRESS_NS_HI(BASE_PORT(ptp_clock)));
> +	if (rc < 0)
> +		return false;
> +	if (!(rc & MCHP_PTP_TX_EGRESS_NS_HI_TS_VALID))
> +		return false;
> +	*nsec = (rc & GENMASK(13, 0)) << 16;
> +
> +	rc = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			  MCHP_PTP_TX_EGRESS_NS_LO(BASE_PORT(ptp_clock)));
> +	if (rc < 0)
> +		return false;
> +	*nsec = *nsec | rc;
> +
> +	rc = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			  MCHP_PTP_TX_EGRESS_SEC_HI(BASE_PORT(ptp_clock)));
> +	if (rc < 0)
> +		return false;
> +	*sec = rc << 16;
> +
> +	rc = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			  MCHP_PTP_TX_EGRESS_SEC_LO(BASE_PORT(ptp_clock)));
> +	if (rc < 0)
> +		return false;
> +	*sec = *sec | rc;
> +
> +	*seq = phy_read_mmd(phydev, PTP_MMD(ptp_clock),
> +			    MCHP_PTP_TX_MSG_HEADER2(BASE_PORT(ptp_clock)));
> +	if (*seq < 0)

Likewise, *seq is unsigned; it can never be less than 0.

> +		return false;
> +
> +	return true;
> +}

...