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

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

[Jeremy Kerr]

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;
 		}
-- 
2.35.1

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

[Alexander H Duyck]

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>

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

[Jeremy Kerr]

Hi Alexander.

> 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.

Yep, that would make sense if they're commonly used values, but I'm not
sure that would be suitable to include that in this fix, as it would
just add disruption to any backport work.

Cheers,


Jeremy

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

[Alexander Duyck]

On Fri, Dec 16, 2022 at 10:44 PM Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
>
> Hi Alexander.
>
> > 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.
>
> Yep, that would make sense if they're commonly used values, but I'm not
> sure that would be suitable to include that in this fix, as it would
> just add disruption to any backport work.

Sorry, I didn't intend that for this patch. I was suggesting it as
possible follow-on work.

Thanks,

- Alex

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

[Jeremy Kerr]

Hi Alexander,

> 
> > Yep, that would make sense if they're commonly used values, but I'm
> > not sure that would be suitable to include that in this fix, as it
> > would just add disruption to any backport work.
> 
> Sorry, I didn't intend that for this patch. I was suggesting it as
> possible follow-on work.

Ah, cool! Yep, definitely a contender for a future change.

(this might be a good opportunity as a starter item for someone wanting
to get familiar with the development/review process, if you know of
anyone?)

Cheers,


Jeremy

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 16 Dec 2022 11:44:09 +0800 you 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.
> 
> [...]

Here is the summary with links:
  - [net] mctp: serial: Fix starting value for frame check sequence
    https://git.kernel.org/netdev/net/c/2856a62762c8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html