linuxkpi: upstream drm-kmod conflicting changes

[bz]

Summary

The upcoming in-kernel implementations for linuxkpi based on work on
iwlwifi (and other wireless drivers) conflicts in a few places with
the drm-kmod grpahics work outside the base system.

In order to transition smoothly and bring implementation parts, such
as a central implementation for linux firmware loading into the tree,
extract the conflicting bits for separate upfront review so that
ports and packages have a chance to deal with this before other things
come.
Some of the files are deliberately "empty" or some implementations
are still labeled with XXX but should provide equal support as the
out-of-tree work.

Changes to the LinuxKPI infrastructure and to drm-kmod will follow
indepedently.

MFC after:		3 months
X-MFS after:		still unclear as it would break kms-drm.
Obtained from:	bz_iwlwifi
Sponsored by:		The FreeBSD Foundation

[kib]

sys/compat/linuxkpi/common/include/asm/unaligned.h

54

This return is not needed.

64

Same.

83

What is the purpose of this comment ?

sys/compat/linuxkpi/common/include/linux/cpu.h

38 ↗

(On Diff #77660)

s/if/with/ ?

sys/compat/linuxkpi/common/include/linux/kobject.h

160

Again return. Would be nice to give a hint about intended functionality.

sys/compat/linuxkpi/common/src/linux_firmware.c

57 ↗

(On Diff #77660)

Extra () around condition.

149 ↗

(On Diff #77660)

^^^

[• hselasky]

This change looks good. Just address @kib's comments.

[manu]

sys/compat/linuxkpi/common/include/linux/cpu.h

41 ↗

(On Diff #77660)

No point of adding this empty file, drm-kmod only have an dummy include so it will be used if base or gplv2/ doesn't have it.

sys/compat/linuxkpi/common/include/linux/device.h

619

Not sure we want to add those functions right now.
We need to propertly track all those malloc in the device struct in some way by either adding the devres stuff if it's also used directly by some drivers or doing our own stuff.

sys/compat/linuxkpi/common/include/linux/firmware.h

42 ↗

(On Diff #77660)

I think this comment is obsolete now that you only define the struct once.

sys/compat/linuxkpi/common/include/linux/pm.h

38

I don't think that we build anything with CONFIG_PM_SLEEP added now so no need to add this for now.

[bz]

I'll wait to see what @manu or others in case of kobject_uevent_env() say and then upload the final diff. Interim version coming in a minute.

sys/compat/linuxkpi/common/include/asm/unaligned.h

83

Sorry 20 year old habit.

sys/compat/linuxkpi/common/include/linux/cpu.h

41 ↗

(On Diff #77660)

I believe I had issues at one point; otherwise I would not have added it but I am fine to leave it out if you say it's not needed and then bring it back later with the content.

sys/compat/linuxkpi/common/include/linux/device.h

619

Yes, but currently this is no better or worse than the DRM implementation from what I gathered.
The reason of this set of changes is not to change the status-quo but to resolve the linuxkpi conflicts so that further additions to the linuxkpi in base can be done without worrying (too much) about DRM conflicts again and again.
In that light, would it be okay to leave them like this?

sys/compat/linuxkpi/common/include/linux/kobject.h

160

Again return. Would be nice to give a hint about intended functionality.

Removed the "return".
Not sure about what we'd do with it; hence left the XXX; I know iwlwifi sends an INACCESSIBLE event when it detects that the card (pice endpoint) gone and it attempts a removal cleanup. Not sure if we do anything related to udev/sysfs at the moment or need a shortcut or simply ignore it; hence left the /* XXX */ to clearly distinguish from a purposefully left empty function. I'll be happy to put a suggested text there if any of you know more?

sys/compat/linuxkpi/common/include/linux/pm.h

38

I believe one of the drivers in my tree did need them as they had not properly #ifdefed everything.
Otherwise I'd not have done them.
I can leave them out for the pure DRM conflict resolving but then they'd come back soon anyway.
Which one would you prefer?

[bz]

Address comments from kib mostly.

[• hselasky]

sys/compat/linuxkpi/common/include/linux/device.h

608

Why can't you just make a list in the _dev, and extend the allocation with a STAILQ entry?
Then there is at least some way to clean up.

[emaste]

I added a linuxkpi group in Phabricator that folks with an interest may join

[manu]

sys/compat/linuxkpi/common/include/linux/device.h

619

There is a lot in the drm-kmod code that needs to be in tree, the main reason that they aren't there for now is that nobody took the time to clean them and properly done them.
I don't think that this is a good things to add stuff in linuxkpi in base if they aren't correct.

sys/compat/linuxkpi/common/include/linux/pm.h

38

The only user in drm-kmod is backlight which will go away soon so you can leave it.

[bz]

sys/compat/linuxkpi/common/include/linux/device.h

608

Because it is more than just tracking mallocs imho; I have drivers not cleaning up PCI things either and neither this nor DRM currently track it. It's an entire separate thing to implement all this and test it. If I keep doing this I'll still be talking about conflicting things with DRM in 6 months. No one says we shouldn't do this. I am trying to keep this one simple; it's been hard enough to compile it.

[bz]

sys/compat/linuxkpi/common/include/linux/device.h

619

The code from the gplv2 directory is mostly not applicable to be moved into head I was told. Neither did I know about it when I initially worked on my code (which was good). I had shared my code previously so people could pick and choose and knew what was coming. The only reason I went near the DRM linuxkpi implementation is to avoid any breakage when starting to move things into HEAD putting #ifdefs around duplicate code.

The only way to move forward for me is (a) to resolve conflicts with DRM, ideally in one go and bump (rather than 10) or (b) break DRM. I've been really trying hard the last weeks not to have to do (b). I can alternatively give you another big dump of my linuxkpi directory as I did in the past and let you guys sort it out.. it's just going to get bigger and won't be able to merged until the parts of this is solved.

Can't we simply do the resource tracking for all the "m" functions in head a week or two after these macros went in? I'll be more than happy if I can finally unload and re-load my driver without a reboot in between as well. But I also need to track PCI resources and other things as mentioned above, not just mallocs. And I don't want to test this then against DRM while I am still resolving conflicts. But you'll get these things for free to test independently of conflicting code and can focus on the DRM parts.

sys/compat/linuxkpi/common/include/linux/pm.h

38

Thank you!

[• hselasky]

sys/compat/linuxkpi/common/include/linux/device.h

608

I understand, but I still think we should go the extra mile and implement these functions properly. At least add a global STAILQ_HEAD() where all these allocations are listed. When we unload these are flushed.

It is not much effort. (size) + sizeof(void *).

Do you want me to fix it?

[bz]

sys/compat/linuxkpi/common/include/linux/device.h

608

No, as I said, the extra nile is not the extra yard.

If this is done it needs to be done properly for other resources as well. I started to look into that and put a separate review up for it to make you feel more comfortable that it'll be done.

Otherwise I'll just had to deal with changing everything again in a week or two.

But thanks for the offer.

[kib]

sys/compat/linuxkpi/common/include/linux/kobject.h

44

Is this comment useful ?

160

The first half of the last paragraph makes quite good note to put there, IMO.

sys/compat/linuxkpi/common/include/linux/pci.h

1177

Why this comment ? Why is 0 significant for drivers ?

[bz]

sys/compat/linuxkpi/common/include/linux/device.h

619

Not sure we want to add those functions right now.
We need to propertly track all those malloc in the device struct in some way by either adding the devres stuff if it's also used directly by some drivers or doing our own stuff.

I had no idea what devres was. Sounds like "we are too lazy to properly write drivers" or "we are smart and let some common code do all the hard work for us even if it means we might hold unneeded resources for ages". Would be interesting to "profile" hit rates on these resources one day (sorry there's a researcher here in me ;-).

It seems drm does use devres; at least I saw mentions of it.
My current set of wifi drivers don't seem to.
I'll go and see what I can extract from the non-GPL drm/driver code in terms of API and start using that.
My initial idea was to hang it off the kobj/dev somewhere and then deal with it there.
Might be a better way than what I thought to do and certainly better than just adding a tailq for mallocs.

[bdragon]

sys/compat/linuxkpi/common/include/linux/pci.h

1177

IIRC, On POWER8 / POWER9, each pci slot on the board has its own root complex, which makes it a separate domain, so, no, you should not assert that it is 0.

Intel machines tend to stick everything on one root complex.

[bz]

sys/compat/linuxkpi/common/include/linux/device.h

608

Just as note:

I have basic "devres" support locally for some devm_ functions; currently doing the 3 pcim_ I need as well.

I'll also do devres_destroy() tomorrow and that should also give @manu all what is needed for drm::drivers/gpu/drm/ttm/ttm_page_alloc_dma.c .

Depending on how D27550 continues we can put things up and get them all into .. git then ;-)

[bz]

Changes since the last one:

• I hope I addressed all comments some way
• firmware changes are in D27414 now (update in a few minutes)
• Cleaned up some PCI bits migrated more parts into lkpifill_pci_dev()
• That plus a dev release function allows DRM to also work with "dev/kobj" magic and get the proper cleanup; the current code is still a short-cut but one I think we can live with. pci_dev_put() seems to do the right thing and I can still unload i915kms.ko.
• Implemented the devres bits we need for devm_k*alloc() and devm_kasprintf() and the DRM parts (I do have more devres cleanup in my local tree but that's unrelated to DRM so will come later to keep this one smaller).

[manu]

TBH this is to big for me to review, if you could split the review based on the commits (as I hope you didn't commit this whole thing in one commit) that would be easier to review.

[bz]

In D26598#629837, @manu wrote:

TBH this is to big for me to review, if you could split the review based on the commits (as I hope you didn't commit this whole thing in one commit) that would be easier to review.

Sure will do; I'll split out the devres and PCI bits into separate reviews; I'll leave the minor changes and new files here.

[bz]

In addition to firmware changes, also the PCI and devres changes
got factored out into the respective child reviews now.

These are the more or less all per-file self-standing changes
which are left and been part of this review in one way or another
from the beginning.

[bz]

Anyone any last comments; I'd love to commit this before Monday.

[• hselasky]

Just one minor comment and I will accept.

Thank you!

sys/compat/linuxkpi/common/include/linux/kernel.h

611

Hi Bjoern,

Can you generalize these macros like this instead?

#define YES(...) __VA_ARGS__
#define NO(...)

/* (NOT(YES) == NO) and (NOT(NO) == YES) */
#define NOT(arg) _NOT(YES arg(() NO))
#define          _NOT(...) __VA_ARGS__

#define PASS(...) __VA_ARGS__


/* test macro for YES == 1 or NO == 0 */
#define EVAL(macro) (0 macro(+1))

#define ___XAB_0 NO
#define ___XAB_1 YES

Other values will cause an error.

#define IS_BULTIN(_x) PASS(EVAL(__XAB_##_x))

Same for the others.

[bz]

Thanks @hselasky . Can you clarify on this one a bit more?

sys/compat/linuxkpi/common/include/linux/kernel.h

611

This is not going to work; you need an intermediate as otherwise you end up with PASS(EVAL(___XAB_CONFIG_FOO)) (and an extra _ in your example above):

#define IS_BULTIN(_x) PASS(EVAL(__XAB_##_x))

becomes:

#define _IS_XAB(_x)     PASS(EVAL(___XAB_##_x))
#define IS_BUILTIN(_x)  _IS_XAB(_x)
#define IS_MODULE(_x)   _IS_XAB(_x ## _MODULE)

I'll be happy to use whatever name for _IS_XAB(_x_ ... __IS_ZERO_ONE(_x)?

I think otherwise we might be good for as long as the input values will indeed always be 0 and 1. Let's hope the format doesn't change.

That said do we also need to use an `__enabled_` prefix as well then? Will make our Makefiles ugly ... I haven't seen the Linux files only just read an online article about how these options are generated and expanded.

[• hselasky]

sys/compat/linuxkpi/common/include/linux/kernel.h

611

and an extra _ in your example above

Yes, I miscounted the number if "_" characters in those macros. Glad you figured it out.

I'll be happy to use whatever name for _IS_XAB(_x_ ... __IS_ZERO_ONE(_x)?

Do you need the _IS_XAB() macro. Can't you just expand it where you use it?

I think otherwise we might be good for as long as the input values will indeed always be 0 and 1.

It will be easy to fix if the values are y/n/m instead.

That said do we also need to use an __enabled_ prefix as well then? Will make our Makefiles ugly ... I haven't seen the Linux files only just read an online article about how these options are generated and expanded.

Haven't seen this in use during my experiences with the Linux kernel.

--HPS

[bz]

sys/compat/linuxkpi/common/include/linux/kernel.h

611

It doesn't actually work.

In the undefined case EVAL() will be presented a "_XAB_CONFIG_FOO" and become (0 _XAB_CONFIG_FOO(+1)) and fail compiling.
That may work for Linux in case they always ensure that all the possible options are properly defined out of the build system; we do not. It would mean we'll be forced to -DCONFIG_FOO=0 all possible options we don't want. Kind-of unhelpful.

I'll keep pondering this a bit more over the next hour .. do you have some more input?

I believe for the same reason the IS_MODULE() case will also not work in my version though I haven't tested that yet.

[bz]

sys/compat/linuxkpi/common/include/linux/kernel.h

611

Ignore the IS_MODULE() case; that will work as it'll end with false as wall in the undefined case.

[• hselasky]

No more comments from me.

[bz]

@hselasky I think the only way to avoid this problem in the future is to audit Makefiles for everything and then insist on all of the option being defined and then I'd be happy to come back to your suggestion and enforce it.

Anyone else anything? Otherwise I'd really love to coordinate with @manu tomorrow to get this in so that DRM and WiFi can get along way better in 13 as well.

[bz]

Use 0/1 instead of false/true for the IS_*() macros so that prepocessor
directives (#if) are more happy.

Discovered while trying to implement @hselasky's more restrictive
checks which currently do not work with undefined CONFIG_ options.
Need to come back to them at a later point as cross-testing DRM and
WiFi between machiens and trees is becoming expensive until this
chain of committs if in.

[val_packett.cool]

sys/compat/linuxkpi/common/include/linux/kobject.h

164

@bz in drm we do devctl_notify instead of kobject_uevent_env: https://github.com/FreeBSDDesktop/kms-drm/pull/215/files

If kobject_uevent_env just worked to produce the same kind of devctl notification it would be very nice.

[bz]

Thanks for pointing this out!

sys/compat/linuxkpi/common/include/linux/kobject.h

164

Oh cool.
I'll go and have a look and then we could possibly even unifdef that part in drm_sysfs_hotplug_event() and put it where it belongs :-)