Re: [PATCH] net:mctp: split mctp hdr version to ver and rsvd

[Jeremy Kerr]

Hi yuanxhaoming,

> from spec dsp0236_1.2.1.pdf page 26, the mctp header contains the
> RSVD(4bit) and Hdr version(4 bit).
> 
> mctp_pkttype_receive invoke mctp_hdr, and get mh->ver whole byte 
> compare the MCTP_VER_MIN, MCTP_VER_MAX. the reserver bits may be
> by misleading used.
> 
> Signed-off-by: yuanzhaoming <yuanzm2@xxxxxxxxxx>
> ---
>  include/net/mctp.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/mctp.h b/include/net/mctp.h
> index e1e0a69afdce..80cc9c63f6ba 100644
> --- a/include/net/mctp.h
> +++ b/include/net/mctp.h
> @@ -14,10 +14,17 @@
>  #include <linux/netdevice.h>
>  #include <net/net_namespace.h>
>  #include <net/sock.h>
> +#include <asm/byteorder.h>
>  
>  /* MCTP packet definitions */
>  struct mctp_hdr {
> -       u8      ver;
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +       u8      ver:4, rsvd: 4;
> +#elif defined(__BIG_ENDIAN_BITFIELD)
> +       u8      rsvd:4, ver: 4;
> +#else
> +#error "Please fix <asm/byteorder.h>"
> +#endif

I would strongly prefer that we do not use C bitfields for a wire
format. The existing flags_seq_tag member contains three fields, which
we use with a couple of helpers to extract the flag, sequence number or
tag values - please follow that convention if we need changes here.

Also, this introduces a few subtle bugs in that we are no longer setting
the reserved bits to zero when preparing an outgoing TX packet header.

What is the underlying issue are you fixing here? Are you seeing a peer
that is sending us packets with bits set in the reserved field?

(if so, that would also be handy information to include in the commit
message)

> From: yuanzhaoming <yuanzm2@xxxxxxxxxx>

Is this the preferred format of your name? These are generally in
a full-name format, or a known identity. There's no particular issue
with what you're using there, if that's what you prefer.

Cheers,


Jeremy