linuxkpi: firmware
AbandonedPublic
Actions

Authored by bz on Sep 1 2020, 3:43 PM.

Details

Reviewers

• hselasky
manu

Summary

Introduce a linux compatibility interface to firmware(9) to handle
linux firmware functions, which we seem to rewrite in multiple
(work-in-progress) drivers. Due to the equal naming of
struct firmware the "compat structure" is duplicated between header
and implementation with a "linux_" prefix in the latter and needs
to be kept in sync. This allows us to hide the FreeBSD implementation
entirely.

Reviewed by:   ..
Sponsored by:  The FreeBSD Foundation
MFC after:	5 days

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 33297
Build 30618: arc lint + arc unit

Event Timeline

bz requested review of this revision.Sep 1 2020, 3:43 PM

bz created this revision.

Harbormaster completed remote builds in B33297: Diff 76493.Sep 1 2020, 3:43 PM

manu added inline comments.Sep 1 2020, 3:50 PM

sys/compat/linuxkpi/common/include/linux/firmware.h
53	Function should be named linux_ in linuxkpi.
sys/compat/linuxkpi/common/src/linux_firmware.c
39	What's the point of having the structure in both files ?
66	Use bsd function inside linuxkpi, so this can be malloc with M_WAITOK etc ...
79	Not sure this is enough for all the case, please try with drm-devel or drm-current-kmod.

bz added inline comments.Sep 1 2020, 4:00 PM

sys/compat/linuxkpi/common/include/linux/firmware.h
53	Why should it? Linux calls it this way. the entire purpose of linuxkpi is to not touch upstream code where avoidable.
sys/compat/linuxkpi/common/src/linux_firmware.c
39	See the description of this review.
66	We seem to do both, though malloc() seems to be more dominant. I am happy to change that. I would not assume M_WAITOK is okay in this case.
79	I am not sure either. athp also has a weird scheme in place internally still to map / to _ internally in rewritten code. But by FreeBSD's module conventions is that the names are flat inside a search directory. Then it's a matter of how we provide (generate) the firmware modules. I can have a look; what's the preferred source URL currently?

manu added inline comments.Sep 1 2020, 4:04 PM

sys/compat/linuxkpi/common/include/linux/firmware.h
53	Use a function macro to change the name, but the function symbol should be linux_XXX
sys/compat/linuxkpi/common/src/linux_firmware.c
39	I saw after, why not making the same thing as in drm-kmod ? #define/#undef trick ? Heck, why not taking the code from drm-kmod ? It's not gpl
66	We do WAITOK in drm and never had a problem. Also with bsd func you can have a MALLOC_DEFINE so the memory is traceable.
79	https://github.com/freebsd/drm-kmod/blob/master/linuxkpi/gplv2/src/linux_firmware.c

I see manu had some comments. I'll wait for these to resolve before doing a review.

bz added inline comments.Sep 1 2020, 4:23 PM

sys/compat/linuxkpi/common/include/linux/firmware.h
53	Why? Most of the linuxkpi functions don't do this. See linux/pci.h for just one example. Do you want to macrofize and change all of it to linux_? or actually better linuxkpi_* then as linux_ is the linuxolator I'd assume?
sys/compat/linuxkpi/common/src/linux_firmware.c
39	Sorry, I guess same problem as you; hadn't made it into the tree yet and was only in other public code drops. I wrote half of that code outside linuxkpi last year for the first driver and cleaned it up and extended it for iwlwifi and moved it into linuxkpi so it was usable for rtw88 as well and am now looking at a 4th driver which had this kind of code duplicated elsewhere as well by someone else.
66	I'll check the callers again about NOWAIT. MALLOC_DEFINE Is fine for as long as we are sure (which I think we can assume in this case) that no one in Linux land simply calls "free" on things. As I said I am willing to change that.
79	Thanks, will do.

manu added inline comments.Sep 1 2020, 4:30 PM

sys/compat/linuxkpi/common/include/linux/firmware.h
53	See https://svnweb.freebsd.org/base/head/sys/compat/linuxkpi/common/include/linux/wait.h?revision=364232&view=markup#l128 for example. Or if you want to check linux/pci.h see https://svnweb.freebsd.org/base/head/sys/compat/linuxkpi/common/include/linux/pci.h?revision=361828&view=markup#l548

The kernel linker i FreeBSD has a subtle feature, that if multiple symbols with the same name exist, only the first one is resolved, and no warning is given. Can only be found by a LINT compile.

Good practice is to prefix functions with linux_ or linuxkpi_ like manu@ pointed out.

Note that this commit, once approved, should be synced with ports update to drm-current-kmod and drm-devel-kmod.

In D26277#584141, @manu wrote:

Note that this commit, once approved, should be synced with ports update to drm-current-kmod and drm-devel-kmod.

I'll do the due-diligens based on the URL you gave me earlier to see what else we have as common code; which of the branches there do I need to check?

Then we can put it all up (either yours or mine) in here for review, cross-check that all still works and get it into FreeBSD in one go, which should make checking in ports for available functionality in base a lot easier. In other words, I'll not push this into head alone if there's other stuff.

In D26277#584185, @bz wrote:

In D26277#584141, @manu wrote:

Note that this commit, once approved, should be synced with ports update to drm-current-kmod and drm-devel-kmod.

I'll do the due-diligens based on the URL you gave me earlier to see what else we have as common code; which of the branches there do I need to check?

Then we can put it all up (either yours or mine) in here for review, cross-check that all still works and get it into FreeBSD in one go, which should make checking in ports for available functionality in base a lot easier. In other words, I'll not push this into head alone if there's other stuff.

Question: given the linux_firmware.c file there has no author or license on it, do I have to assume the files there are indeed as the directory tree suggests gplv2? Because if they are I don't want to go near their contents.

In D26277#584190, @bz wrote:

In D26277#584185, @bz wrote:

In D26277#584141, @manu wrote:

Note that this commit, once approved, should be synced with ports update to drm-current-kmod and drm-devel-kmod.

I'll do the due-diligens based on the URL you gave me earlier to see what else we have as common code; which of the branches there do I need to check?

Then we can put it all up (either yours or mine) in here for review, cross-check that all still works and get it into FreeBSD in one go, which should make checking in ports for available functionality in base a lot easier. In other words, I'll not push this into head alone if there's other stuff.

Question: given the linux_firmware.c file there has no author or license on it, do I have to assume the files there are indeed as the directory tree suggests gplv2? Because if they are I don't want to go near their contents.

No, if you look at the history it was imported by markj from drm-next, mmacy is the author I think

In D26277#584193, @manu wrote:

In D26277#584190, @bz wrote:

In D26277#584185, @bz wrote:

In D26277#584141, @manu wrote:

Note that this commit, once approved, should be synced with ports update to drm-current-kmod and drm-devel-kmod.

I'll do the due-diligens based on the URL you gave me earlier to see what else we have as common code; which of the branches there do I need to check?

Then we can put it all up (either yours or mine) in here for review, cross-check that all still works and get it into FreeBSD in one go, which should make checking in ports for available functionality in base a lot easier. In other words, I'll not push this into head alone if there's other stuff.

Question: given the linux_firmware.c file there has no author or license on it, do I have to assume the files there are indeed as the directory tree suggests gplv2? Because if they are I don't want to go near their contents.

No, if you look at the history it was imported by markj from drm-next, mmacy is the author I think

Cool. Which branches should I all check or is the linuxkpi basically identical?

In D26277#584194, @bz wrote:

In D26277#584193, @manu wrote:

In D26277#584190, @bz wrote:

In D26277#584185, @bz wrote:

In D26277#584141, @manu wrote:

Note that this commit, once approved, should be synced with ports update to drm-current-kmod and drm-devel-kmod.

I'll do the due-diligens based on the URL you gave me earlier to see what else we have as common code; which of the branches there do I need to check?

Then we can put it all up (either yours or mine) in here for review, cross-check that all still works and get it into FreeBSD in one go, which should make checking in ports for available functionality in base a lot easier. In other words, I'll not push this into head alone if there's other stuff.

Question: given the linux_firmware.c file there has no author or license on it, do I have to assume the files there are indeed as the directory tree suggests gplv2? Because if they are I don't want to go near their contents.

No, if you look at the history it was imported by markj from drm-next, mmacy is the author I think

Cool. Which branches should I all check or is the linuxkpi basically identical?

The firmware part is identical everywhere (even at https://github.com/FreeBSDDesktop/kms-drm)

In D26277#584196, @manu wrote:

In D26277#584194, @bz wrote:

In D26277#584193, @manu wrote:

In D26277#584190, @bz wrote:

In D26277#584185, @bz wrote:

In D26277#584141, @manu wrote:

Note that this commit, once approved, should be synced with ports update to drm-current-kmod and drm-devel-kmod.

I'll do the due-diligens based on the URL you gave me earlier to see what else we have as common code; which of the branches there do I need to check?

Then we can put it all up (either yours or mine) in here for review, cross-check that all still works and get it into FreeBSD in one go, which should make checking in ports for available functionality in base a lot easier. In other words, I'll not push this into head alone if there's other stuff.

Question: given the linux_firmware.c file there has no author or license on it, do I have to assume the files there are indeed as the directory tree suggests gplv2? Because if they are I don't want to go near their contents.

No, if you look at the history it was imported by markj from drm-next, mmacy is the author I think

Cool. Which branches should I all check or is the linuxkpi basically identical?

The firmware part is identical everywhere (even at https://github.com/FreeBSDDesktop/kms-drm)

And the rest of all the linuxkpi changes. I have a ton sitting here locally. Hence my question, which branches to all check in your trees.

In D26277#584197, @bz wrote:

In D26277#584196, @manu wrote:

In D26277#584194, @bz wrote:

In D26277#584193, @manu wrote:

In D26277#584190, @bz wrote:

In D26277#584185, @bz wrote:

In D26277#584141, @manu wrote:

Note that this commit, once approved, should be synced with ports update to drm-current-kmod and drm-devel-kmod.

I'll do the due-diligens based on the URL you gave me earlier to see what else we have as common code; which of the branches there do I need to check?

Then we can put it all up (either yours or mine) in here for review, cross-check that all still works and get it into FreeBSD in one go, which should make checking in ports for available functionality in base a lot easier. In other words, I'll not push this into head alone if there's other stuff.

Question: given the linux_firmware.c file there has no author or license on it, do I have to assume the files there are indeed as the directory tree suggests gplv2? Because if they are I don't want to go near their contents.

No, if you look at the history it was imported by markj from drm-next, mmacy is the author I think

Cool. Which branches should I all check or is the linuxkpi basically identical?

The firmware part is identical everywhere (even at https://github.com/FreeBSDDesktop/kms-drm)

And the rest of all the linuxkpi changes. I have a ton sitting here locally. Hence my question, which branches to all check in your trees.

I've delete every branch that was merged to master, so every branch is currently active.

In D26277#584127, @hselasky wrote:

The kernel linker i FreeBSD has a subtle feature, that if multiple symbols with the same name exist, only the first one is resolved, and no warning is given. Can only be found by a LINT compile.

I am not sure about that. Apply the patch. Compile and load the zzz.ko and yyy.ko and you'll see that despite both having a int foo(int) function which is non-static the results are still different .. https://people.freebsd.org/~bz/tmp/20200901-dusym-test2.diff

Good practice is to prefix functions with linux_ or linuxkpi_ like manu@ pointed out.

Okay. For good practise I'll mangle it all .. Should we start README under linuxkpi/ somewhere with all these "implict" rules not documented anywhere?