One more driver (my Apple SPI-HID driver) will need a crc16 implementation.
Rather than continue the spread of duplicate crc16() functions, let's extract this one into a common header?
Details
Diff Detail
- Repository
- R10 FreeBSD src repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
I think it will be better to commit another crc16 implementation and then apply 'code duplication fix' by moving function used in both places to independent header. The reason is, that I cannot confirm that it is 'generic' and 'right' crc16 implementation. I can only confirm, that this implementation is suitable for ext2fs.
Yeah, it looks like this corresponds with what Linux calls "standard CRC16". My reference above calls it CRC16/UMTS.
Do we have other equivalent in-tree copies?
The polynomial 0x8005 is CRC-16-IBM or CRC-16-ANSI.
Linux has a separate header for crc-16-ansi: https://elixir.bootlin.com/linux/latest/source/include/linux/crc16.h
It's used for ext4: https://elixir.bootlin.com/linux/latest/source/fs/ext4/super.c#L2825
And for Apple: https://elixir.bootlin.com/linux/latest/source/drivers/input/keyboard/applespi.c#L868
And elsewhere:
https://elixir.bootlin.com/linux/latest/C/ident/crc16
I see no reason not to put this implementation in a separate header file.
https://reveng.sourceforge.io/crc-catalogue/all.htm has 6 different named poly 0x8005 CRC16s.
Regardless of what we name it I believe this change is good.
There is another one user of this crc16(): usr.sbin/bhyve/pci_nvme.c
May be we should move table to sys/libkern/crc16.c to use single copy across the users?
It would be slightly annoying to apply both changes — we'll have to build a libkern file in a usr.sbin program…
It is easy. See e.g. lib/libufs/Makefile which imports crc32 from libkern.
From the other side there are no crc16 users in default kernel, so it will be unused most of time.
One small thing that can be done here is moving crc16_table out of crc16() and making latter inlined.
Full git patch with metadata for someone to commit: P528 (I'm still not exactly sure why sometimes arc can apply with metadata and sometimes it can't…)
I'm still not exactly sure why sometimes arc can apply with metadata and sometimes it can't…
afaik Phabricator can (usually) handle git metadata on supplied patches but does not store it and doesn't have a way to return it again when downloading a patch
P528 yes, but Greg had to just create that outside of the normal arc workflow I think, it's no different than attaching a git patch to a Bugzilla PR
Yes. It was also created with -U9999999 or whatever to get the entire files... That's great for reviews, terrible for applying the patch in general because patch is unhappy with mismatches in different ways...
Both the bugzilla work flow and doing it this way require special, weird names / URLS to be fetched to the raw file that can be fed into git am. bz at least has a helper tool to download and apply.