Re: [PATCH net-next 1/4] net: ptp: introduce enum ptp_msg_type

From: Richard Cochran
Date: Wed Nov 18 2020 - 08:18:54 EST


On Tue, Nov 17, 2020 at 08:31:21PM +0100, Christian Eggers wrote:
> Using a PTP wide enum will obsolete different driver internal defines
> and uses of magic numbers.

I like the general idea, but ...

> +enum ptp_msg_type {
> + PTP_MSGTYPE_SYNC = 0x0,
> + PTP_MSGTYPE_DELAY_REQ = 0x1,
> + PTP_MSGTYPE_PDELAY_REQ = 0x2,
> + PTP_MSGTYPE_PDELAY_RESP = 0x3,
> +};

I would argue that these should be #defines. After all, they are
protocol data fields and not an abstract enumerated types.

For example ...

> @@ -103,10 +110,10 @@ struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type);
> *
> * Return: The message type
> */
> -static inline u8 ptp_get_msgtype(const struct ptp_header *hdr,
> +static inline enum ptp_msg_type ptp_get_msgtype(const struct ptp_header *hdr,
> unsigned int type)
> {
> - u8 msgtype;
> + enum ptp_msg_type msgtype;
>
> if (unlikely(type & PTP_CLASS_V1)) {
> /* msg type is located at the control field for ptp v1 */

msgtype = hdr->control;
} else {
msgtype = hdr->tsmt & 0x0f;

This assigns directly from protocol data into an enumerated type.

}

return msgtype;

Thanks,
Richard