[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2eecaca2d1066d51d136a8d95b5cd2fd19e5e111.camel@gmail.com>
Date: Fri, 16 Dec 2022 08:41:30 -0800
From: Alexander H Duyck <alexander.duyck@...il.com>
To: Jeremy Kerr <jk@...econstruct.com.au>, netdev@...r.kernel.org
Cc: Matt Johnston <matt@...econstruct.com.au>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>,
Harsh Tyagi <harshtya@...gle.com>
Subject: Re: [PATCH net] mctp: serial: Fix starting value for frame check
sequence
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@...gle.com>
> Tested-by: Harsh Tyagi <harshtya@...gle.com>
> Signed-off-by: Jeremy Kerr <jk@...econstruct.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@...com>
Powered by blists - more mailing lists