ext2fs: extract crc16 into sys/crc16.h

[greg_unrelenting.technology]

Summary

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?

[fsu]

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.

[emaste]

https://github.com/madler/crcany is a useful reference for all things CRC

[greg_unrelenting.technology]

Well, this one does match the Apple SPI input controller too.

[emaste]

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?

[afedorov]

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.

[emaste]

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.

[wulf]

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?

[greg_unrelenting.technology]

In D32879#742594, @wulf wrote:

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…

[wulf]

In D32879#742790, @greg_unrelenting.technology wrote:

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.

[greg_unrelenting.technology]

Move the table into a .c file, use in bhyve too

[chuck]

LGTM. Thanks for fixing NVMe!

[greg_unrelenting.technology]

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…)

[emaste]

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

[imp]

In D32879#744901, @emaste wrote:

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

The link sure looked like it had a git am-able patch....

[emaste]

In D32879#744906, @imp wrote:

In D32879#744901, @emaste wrote:

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

The link sure looked like it had a git am-able 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

[imp]

In D32879#744917, @emaste wrote:

In D32879#744906, @imp wrote:

In D32879#744901, @emaste wrote:

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

The link sure looked like it had a git am-able 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.

[greg_unrelenting.technology]

Updated P528 to rebase after the whitespace cleanup; did the diff without -U9999

[khng]

Is anyone going to merge this?