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

[Divya.Koppera]

Hi Simon,

Thanks for your comments. I will take care all in next revision.

Thanks and Regards,
Divya

> -----Original Message-----
> From: Simon Horman <horms@xxxxxxxxxx>
> Sent: Thursday, November 7, 2024 8:04 PM
> To: Divya Koppera - I30481 <Divya.Koppera@xxxxxxxxxxxxx>
> Cc: andrew@xxxxxxx; Arun Ramadoss - I17769
> <Arun.Ramadoss@xxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>; hkallweit1@xxxxxxxxx;
> linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; richardcochran@xxxxxxxxx
> Subject: Re: [PATCH net-next 2/5] net: phy: microchip_ptp : Add ptp library for
> Microchip phys
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> 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@xxxxxxxxxxxxx>
> > ---
> >  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;
> > +}
> 
> ...