Using a designated initializer for qman_errors should be a significant win longer term. At this point I think I've fully figured out how the mess with the DEFINE_CLASS*() macros occurred so I'm going to bring those up all the time. Hopefully someone with better knowledge of the previous driver will provide better comments on the algorithms soon.

I'm unsure whether style items are left for later, but since I'm aware of them. Hopefully D48449 gets in soon and fixes the issues I'm aware of which have been suppressing use of macros.

Looks like the stringify originates from DEFINE_CLASS() which doesn't explicitly pass the class name string. When the DEFINE_CLASS_#() macros were added at rG:46ba7a35f2e they simply copied the behavior. Since at the time there was a strict class-variable-name <=> class-string relationship, using the name for the base class was likely a simple goof.

On further consideration I wonder if perhaps the use of the stringify was intended to discourage creating multiple classes with the same driver name. Unfortunately it seems to have instead discouraged the use of DEFINE_CLASS*().

This is still a distortion, but far less of one than having it in machine/xen/arch-intr.h. Naturally this means header adjustment elsewhere.

I had noted this elsewhere (Github #1748) is it really a good idea to pass around struct trapframe * so much? In particular I notice for normal interrupts it ends up in 3-4 stack frames before reaching intr_event_handle() which then dumps it into curthread->td_intr_frame. Most uses retrieve it from curthread->td_intr_frame.

In D56005#1281270, @kib wrote:

In D56005#1281266, @ehem_freebsd_m5p.com wrote:

This is wrong. Specifically machine/xen/arch-intr.h is the MI <=> MD interface whereas xen_arch_intr_handle_upcall() is the x86 assembly-language <=> C interface. Mixing the two together would be bad.

It is C function, it can be called from C. What is the problem?

This is wrong. Specifically machine/xen/arch-intr.h is the MI <=> MD interface whereas xen_arch_intr_handle_upcall() is the x86 assembly-language <=> C interface. Mixing the two together would be bad.

There seem to be 3 factors which have discouraged the use of the existing DEFINE_CLASS_#() macros.

I had commits for nearly all of these handy, but I'm lacking a commit bit. Worse I'm starving for committer bandwidth so I end up prioritizing higher-importance commits over cleanup. I've got lots more of this handy in association with D48449, just need someone I can hand it off too.

I ended up looking at two of the files which originated from D21335. These spots appear unused and it is in my queue for getting these hunks removed (followed by further cleanup of the files related to D48449).

Drat. Phabricator flagged me due to the Xen changes, but I didn't look too closely since it seemed neutral.

In D48449#1248197, @jrtc27 wrote:

Switching from name ## _baseclasses to basevar ## _baseclasses to avoid the gross #defines you removed seems worthwhile, but I don't see the point of the churn for the rest. Especially since deprecating something that is the only thing being used is a foolish thing to do.

I have a question for @jhibbits/@kib or whomever might know.

I guess I should also add I'm thinking of deprecating in terms of hoping the older macros will be removed at a vague future point. Note I've redone the older macros in terms of the CLASSN macros to suggest the alternative. Removal isn't at all urgent, just hoping to encourage use of the newer macros instead (which have the exact same arguments/order). Marking the older macros deprecated isn't absolutely required, just seems the way to go.

That diff doesn't render too well. Isn't deprecating older macros usually done, particularly when major functionality of the older macros has become rotten? I don't know whether the functionality which has become broken due to changing standards is the main reason why the macros are only used 50% of the time, but I suspect it is a factor.

This is part of a fascinating end point I reached in combination with other commits. Specifically, Github #1457. I think this is better to review via GitHub, since the full git support allows viewing as part of the full series. Phabicator isn't all that well suited to the role.

Trivial update. Wondering which functions to include this flag for initially. Destroy is helpful as it provides an alternative elsewhere.

Minor tweak. The ->ops gets added by all observed callers so far, so seems best to move it into the macro. Slight adjustment to parentheses.

An alternative. Considering whether the _Static_assert() will hopefully generate a better error message. Near-certain to be a giant explosion no matter what if the circumstance occurs.

Hrmm, guess anti-doubling measure failed.

Since this has been out for a while now it is time for fall-out observations.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

A more recent version of the series is now on Github as #1301. Note that is marked as a draft since something else is higher priority right now.

Now looking for this as a commit series will be adding a single parent kernel object to several objects; without something like this several of those will become global (though undeclared).

Updating to current repository state. Mark older macros as deprecated. Trivial reduction in diff size. May need to update D48449 to match the commit message as that no longer resembles what it did when when created.

This is wrong. The triplet of pic_post_filter/pic_post_ithread/pic_pre_ithread are always *required* to be implemented by all INTRNG PICs. Omitting pic_post_filter and pic_post_ithread is a violation of the interface.

Thus things i386 continue to disappear. I suspect AT PIC support should have been removed from AMD64 first. While some AMD64 systems may have working AT PICs, they're near-certain to have local-APICs even on single-processor systems. About the only reason to keep AT PIC building in AMD64 LINT builds is to flag source breakage sooner. I'm presently unable to do any sort of runtime testing of AT PIC since I was unable to get the driver to try loading under any formulation of Qemu.

@mhorne this is a follow-on to D33479. If the normal interface is going to include NULL-checks, but they can be omitted in some fairly common circumstances what do you prefer for new names? Underscore prefix tends to point to internal (though can also indicate lower-level), but some letter suffix would also work. Your view since you've been emphasizing the consistency here?

Sorry about being disorganized in responses. Hopefully you can accommodate since there isn't much I can do about that in the near term. Just be glad you don't have to deal with such 24/7 as that is much worse.

I've got someone up for committing this, but they want D35406 formally approved.

Now at the 2 month mark, and neither approved nor rejected.

Does 2 weeks of silence mean this is implicitly accepted?

I guess I wonder what I'm supposed to do now. Does the lack of updated reviews mean the current version is implicitly accepted? (the requested issue was addressed)

I'm unsure of the preferred handling of these, they tend to get forgotten. This may get in as part of #1457 where removing PIC hooks is helpful.

@kevans is another person who might provide valuable insight into this (due to D39824 and D48079). The real issue is the declarations of kernel objects are very inconsistent. It is split between the uses of the existing macros (DEFINE_CLASS() and DEFINE_CLASS_[0-3]()) and inline uses of struct kobj_class. Feels roughly equal, so somewhere between 25:75 and 50:50.

Correct the count of ampersand adds, I was close previously, but now do exact match. Upon examination seems the issue I noticed had been worked around at least twice. Curious it wasn't addressed then.

Yeah, further testing confirms I didn't fool the preprocessor. Still, this has 3 rather valuable improvements over the existing macros:

Give up on fooling the preprocessor, simply terminate near existing limit.

Switch to using _classvar as the prefix for the base class list. _name has been a *string*, while _classvar was already required to be a valid C variable name. Apparently doesn't effect too many places, but a lurking issue.

Now there is D48449 which proposes {PRIVATE|PUBLIC}_DEFINE_CLASS() to explicitly declare visibility. This also takes care of the issue there is no way to make private classes which have one or more parents.

If anyone is wondering, the value is KOBJOPLOOKUP() is quite helpful for places which want to use kobj, but existing things won't match well. One such place is sys/x86/isa/clock.c:attimer_attach(). If x86 was converted to kobj, the setting of i8254_pending = needs adjustment and KOBJOPLOOKUP() with function pointer return works well. This is also handy for calling a parent class's version of a function after device-variant work is done.

Looking at this some more, the real value is creating PUBLIC_ and PRIVATE_ versions of the DEFINE_CLASS macros. Handling arbitrary numbers of parents mostly serves to simplify things to only needing two macros. It is notable with the current macros it is impossible to create a private/static class with at least one parent.

Ugh, identify the simple goof made on the original version. Going back with that corrected.

I'm unsure of who I should ask to review this, seems close enough to the core you two may be good candidates.

Natually that last minute change breaks things.

One more comment and possible adjustment. I don't like the KOBJOPLOOKUP name. I think the original author was thinking (KOBJOP)(LOOKUP), but I believe the more common parsing is (KOBJ)(OP)(LOOKUP). KOBJLOOKUPOP or KOBJLOOKUP seem better since the is it is looking up an OP (function pointer) on a KOBJ, not looking up a KOBJOP in some global table.

I would have liked to see a comment left in the file. I suspect someone down the line will wonder why someone bothered to optimize this case. This might help uniprocessor VMs, but multiprocessor VMs are also very common. While a single branch is cheap, this still means a tiny performance loss if you've got more than one.

Until a personal issue is taken care of I'm not going to be eloquent. Looks like you figured out how to suppress the fault, which D34701 lacked. Having a commit bit also helps get things through... I'm glad this is done since most cases it will be a const pointer to writable memory (the const being used to cause the compiler to error if it is written to).

Updating as per current comments, add extension, minor title adjustment. Remove a now unnecessary cast from makeobjsops.awk's output.

This did seem a rather better approach to the issue, but I still have the other approach.

To put it a different way, intr_event_destroy() is called from recovery and shutdown routines. Callers have likely already received an error status from another function (since they passed a NULL event), yet now they're trying to recover and get yet another error indicator. Recovery is impossible.

This seemed a rather simple, yet high value point. Yet it sat here with no reviews and unable to step forward. Then the garbage of D47002 got approved in 5 seconds. I get the message, no it is not merely people got tired of trying to converge the interrupt systems. Many people simply didn't care and unintentionally made it extremely difficult.