| Message ID | 20221216034409.27174-1-jk@codeconstruct.com.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2856a62762c8409e360d4fd452194c8e57ba1058 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] mctp: serial: Fix starting value for frame check sequence | expand |
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>
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
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
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
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!
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; }
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(-)