Re: [PATCH net] mctp: serial: Fix starting value for frame check sequence

[Alexander H Duyck <alexander.duyck@gmail.com>]

On Fri, 2022-12-16 at 11:44 +0800, Jeremy Kerr wrote:
> RFC1662 defines the start state for the crc16 FCS to be 0xffff, but
> we're currently starting at zero.
> 
> This change uses the correct start state. We're only early in the
> adoption for the serial binding, so there aren't yet any other users to
> interface to.
> 
> Fixes: a0c2ccd9b5ad ("mctp: Add MCTP-over-serial transport binding")
> 
> Reported-by: Harsh Tyagi <harshtya@google.com>
> Tested-by: Harsh Tyagi <harshtya@google.com>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---
>  drivers/net/mctp/mctp-serial.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
> index 7cd103fd34ef..9f9eaf896047 100644
> --- a/drivers/net/mctp/mctp-serial.c
> +++ b/drivers/net/mctp/mctp-serial.c
> @@ -35,6 +35,8 @@
>  #define BYTE_FRAME		0x7e
>  #define BYTE_ESC		0x7d
>  
> +#define FCS_INIT		0xffff
> +
>  static DEFINE_IDA(mctp_serial_ida);
>  
>  enum mctp_serial_state {
> @@ -123,7 +125,7 @@ static void mctp_serial_tx_work(struct work_struct *work)
>  		buf[2] = dev->txlen;
>  
>  		if (!dev->txpos)
> -			dev->txfcs = crc_ccitt(0, buf + 1, 2);
> +			dev->txfcs = crc_ccitt(FCS_INIT, buf + 1, 2);
>  
>  		txlen = write_chunk(dev, buf + dev->txpos, 3 - dev->txpos);
>  		if (txlen <= 0) {
> @@ -303,7 +305,7 @@ static void mctp_serial_push_header(struct mctp_serial *dev, unsigned char c)
>  	case 1:
>  		if (c == MCTP_SERIAL_VERSION) {
>  			dev->rxpos++;
> -			dev->rxfcs = crc_ccitt_byte(0, c);
> +			dev->rxfcs = crc_ccitt_byte(FCS_INIT, c);
>  		} else {
>  			dev->rxstate = STATE_ERR;
>  		}

Since the starting value isn't unique would it possibly be worthwhile
to look at adding a define to include/linux/crc-ccitt.h to be used to
handle the cases where the initial value is 0xffff? I notice there
seems to only be two starting values 0 and 0xffff for all callers so it
might make sense to centralize it in one place.

Otherwise the code itself looks good.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>