This return is not needed.

Same.

What is the purpose of this comment ?

s/if/with/ ?

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

Extra () around condition.

^^^

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.

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 think this comment is obsolete now that you only define the struct once.

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

Sorry 20 year old habit.

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.

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?

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?

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?

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.

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.

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

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.

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.

Thank you!

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?

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.

Is this comment useful ?

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

Why this comment ? Why is 0 significant for drivers ?

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.

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.

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

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.

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.

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

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.

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

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

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