In D54519#1267307, @ziaee wrote:

That was my interpretation of his suggestion. That would leaves the version strings in place to be used in the sysctl and verbuf, but won''t post it in the dmesg, iiuc. Let's wait for the elders and see what they think.

In D54519#1267236, @ziaee wrote:

@imp said:
I'd just drop it entirely. This used to be a vendor driver where a version made sense. But i don't think it dies anymore.

Since problems were revealed with the approach, why don't we drop it entirely?

@avg Let me know how to proceed I'd like to cherry-pick.

In D54519#1265780, @avg wrote:

In D54519#1265767, @michaelo wrote:

Making it once, IMHO, should be consistent for all, at least Broadcom/AVAGO/LSI drivers, not just mrsas.

I agree with this. But we made a small step back instead of any step forward in this respect.

In D54519#1265730, @avg wrote:

I see one change in behavior for mrsas driver which can be considered to be for worse.
If there are multiple adapters, the older code would print only one driver version message.
Now the message would be printed for each adapter.
But doesn't make much sense as the driver is the same.

@ziaee, your turn.

In D46313#1264734, @ziaee wrote:

In D46313#1264717, @michaelo wrote:

Can you move the unrelated doc changes to a separate review,

Negative, negative! One logical change includes doc, we want to do everything we can to try and include people to 1. write doc 2. include it with code changes.

@ziaee and me can easily review and merge them?

Thanks for tagging me! Happy to help with manuals anytime :)

This somewhat reminds of of systemd's oneshot type. I agree with other committers against introducing another PID file, we should have an option doing the oneshot, execute-onl, etc. and the manpage shall say which options are ignored in such a case.

Can you move the unrelated doc changes to a separate review, @ziaee and me can easily review and merge them.

@ziaee, should I go on with the merge or wait for another technical reviewer to approve?

Is this going to be MFC'ed?

@siva, @sunpoet does not respond to reviews as he told me last time. You should create a bugzilla issue and attach a patch.

Address comments

Guys, please have a look. It'd be fantastic to have it in 14.4-RELEASE.

In D55122#1260106, @ziaee wrote:

Should we (then) mention execution order as well?

I don't know, is there anything special about the execution order? I've never had a complicated enough newsyslog setup to run into this problem.

Stupid question: Should we (then) mention execution order as well?

In D54519#1257928, @des wrote:

In D54519#1257714, @michaelo wrote:

Let me rephrase: The version banner should be complete in terms of information regards of the args passed to device_printf. At runtime the passed dev does not know (ahead of time) that the string mrsas/mfi is contained in the version banner. How can this assumption be made at compile time?

Right. Perhaps the kernel should provide a function whose purpose is to print a message to the console on behalf of the driver and make sure that the message is accompanied by the driver name so the user can identify where the message comes from. We could call it something like driver_printf(). Or even better, we can include not only the driver name but the device number as well, and call it device_printf(). Would that satisfy your concern?

Drop redundant terms

In D54519#1257776, @ziaee wrote:

I don't understand. What assumption are we making at compile time?

In D54519#1257713, @des wrote:

In D54519#1257568, @michaelo wrote:

The point I was trying to make is that the dev is not aware at that time how driver banner looks like. From that PoV the banner would look incomplete. WDYT?

I have no idea what any of that means.

In D54519#1257565, @des wrote:

The conceptual change is correct, probes should be silent.

In D54519#1257560, @ziaee wrote:

I do very much approve of the conceptual change, and this code seems clear and good to me, but I am too junior at C programming to be the final word on mechanical changes to drivers. I asked some senior colleagues to take a look.

@des, is this acceptable for you?

@ziaee Can you formally approve then? @emaste, @imp any objections?

In D54619#1256076, @markmi_dsl-only.net wrote:

Well, folks that do not use poudriere(-devel) have been locally putting back ar=${AR} in some places so that their make/Makefile or portmaster style of working can find the right ar. An example is:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=292544

for which the original editing for the original problem also had an ar=${AR} removal. Some folks then reported build errors tied to ar commands that failed to recognize command line syntax used during the port build --and later reported that putting back the ar=${AR} got things to build again for them.

Has make/Makefile or portmaster based build testing been done for this context?

Note: I use poudriere-devel .

I used the code logic from install(1).

This has been implicitly fixed by 78c933ad0ae2d48b627179958af56aedf623d97a.

@imp A followup review to D54843 to use comma consistently.

In D54843#1255708, @imp wrote:

Comma is preferred.

Please pick for 14 and 13 as well. It should apply cleanly. I am still on 13 and will put in into production immediately.

In D54619#1255317, @siva wrote:

Anything further for this review?

@jrm Please don't forget to cherry-pick.

Fix typo

In D54843#1254166, @ziaee wrote:

This is fine with me, but I would wait for @imp or even anyone someone else from Loader like @kevans or @tsoome. It sounds like he might not want to declare that space works and then they're committed to supporting that, or making things consistent towards that.

@imp as requested...

In D54842#1253816, @ziaee wrote:

Many tools expect git commit titles to be 50 or less characters

Incorporate comments

In D54842#1253714, @emaste wrote:

To expand on my comment - the reason I think it's worth mentioning that is to make it clear we haven't made a choice to ignore this tunable with UEFI, it's that it just doesn't apply with UEFI.