Page MenuHomeFreeBSD

ext2fs: extract crc16 into sys/crc16.h
AcceptedPublic

Authored by greg_unrelenting.technology on Nov 7 2021, 12:07 PM.

Details

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?

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.

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

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?

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…

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.

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

chuck added a subscriber: chuck.

LGTM. Thanks for fixing NVMe!

This revision is now accepted and ready to land.Nov 13 2021, 5:49 PM

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

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

In D32879#744906, @imp 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

In D32879#744906, @imp 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.

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

Is anyone going to merge this?