mfi(4)/mrsas(4): Print driver versions consistently

[michaelo]

Summary

MFC after: 3 days

[michaelo]

Screenshot from the machine with both drivers now side by side:

[ziaee]

Thanks for tagging me!

I try to be careful with "FreeBSD" as a search term, I don't think it's appropriate in these driver strings. I also don't know if it matters that it's broadcom megaraid now.

Maybe imp should review this. I'm going to add emaste because he's one marked up the broadcom.

[michaelo]

In D54519#1246275, @ziaee wrote:

Thanks for tagging me!

I try to be careful with "FreeBSD" as a search term, I don't think it's appropriate in these driver strings. I also don't know if it matters that it's broadcom megaraid now.

Maybe imp should review this. I'm going to add emaste because he's one marked up the broadcom.

I *intentionally* did not write Broadcom because the driver was developed when the company still was LSI, then bought by AVAGO which is now named Broadcom:

MFI(4)                 FreeBSD Kernel Interfaces Manual                 MFI(4)

NAME
     mfi – LSI MegaRAID SAS driver

This is fully in line with the driver intro.

[michaelo]

@imp Any objections or opinions here?

[emaste]

device_printf will include the device prefix mfi so I'm not sure why it needs to be repeated, and I'm also not sure what value including FreeBSD in the message brings.
Including LSI, correctly capitalizing MegaRAID, and expanding version all sound good to me.

[imp]

LSI hasn't owned or marked this for a decade.

[ziaee]

correctly capitalizing MegaRAID, and expanding version all sound good to me.

I also agree with this, unsure/no-opinion on LSI.

[michaelo]

I had the following objective:

• Make the string compatible with mrsas(4) since both drivers overlap
• Reflect the author of the driver back then, not now
• Keep mfi in the script because it makes the string consistent with the first point

What is your proposition?

LSI MegaRAID FreeBSD SAS driver version: %s

That would be OK because of the prefix. Unfortunately, mrsas(4) does not print the device prefix and stops at the first device which is inconsistent with mfi(4).

[ziaee]

I think if you want to make them the same you should remove FreeBSD from both. If people search logs for FreeBSD they shouldn't get these drivers, I think.

[michaelo]

In D54519#1248132, @ziaee wrote:

I think if you want to make them the same you should remove FreeBSD from both. If people search logs for FreeBSD they shouldn't get these drivers, I think.

That would be fine for me as long as we (all) agree upon.

[michaelo]

In D54519#1248132, @ziaee wrote:

I think if you want to make them the same you should remove FreeBSD from both. If people search logs for FreeBSD they shouldn't get these drivers, I think.

Done in https://reviews.freebsd.org/D54630

[michaelo]

Drop 'FreeBSD' in version banner

[michaelo]

Unify both changes, use device_printf() in mrsas(4) as well

Will test this also on the server next week

[michaelo]

Looking at booth drivers, mfi(4) uses mfi_attach() to print the message while mrsas(4) uses probe. I could move both to *_attach(), but cannot tell which of both is correct. Logically attach() seems right.

[michaelo]

print driver version in *_attach() function

[michaelo]

I ran this and it looks good now:

root@deblndw014x:~ # dmesg | grep mrsas0
mrsas0: <AVAGO Thunderbolt SAS Controller> port 0x8000-0x80ff mem 0xdfa60000-0xdfa63fff,0xdfa00000-0xdfa3ffff irq 26 at device 0.0 on pci1
mrsas0: FW now in Ready state
mrsas0: Using MSI-X with 16 number of vectors
mrsas0: FW supports <16> MSIX vector,Online CPU 32 Current MSIX <16>
mrsas0: mrsas_init_adapter: sc->reply_q_depth 0x7e0,sc->request_alloc_sz 0x1f78, sc->reply_alloc_sz 0x3f00,sc->io_frames_alloc_sz 0x3f100
mrsas0: max sge: 0x46, max chain frame size: 0x400, max fw cmd: 0x3ef sc->chain_frames_alloc_sz: 0xfbc00
mrsas0: Issuing IOC INIT command to FW.
mrsas0: IOC INIT response received from FW.

[ziaee]

I agree with this from a doc principle but I don't know enough from a C standpoint, I'd like @emaste or @imp to check it.

[michaelo]

In D54519#1252062, @ziaee wrote:

I agree with this from a doc principle but I don't know enough from a C standpoint, I'd like @emaste or @imp to check it.

The conceptional change (attach instead of probe) is that it is only displayed when the driver is used and the device is attached. Before, mrsas was just probed and how a lower pref than mfi. At the end, there is no behavior change in the driver itself, but only a change of the banner.

[michaelo]

Folks, any opinion?

[michaelo]

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

[ziaee]

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.

[michaelo]

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.

Thank you, looking forward for their approval. Did you check the screenshot output actually?

[des]

The conceptual change is correct, probes should be silent.

sys/dev/mfi/mfi.c

375

the “mfi” part here is redundant since device_printf() will prefix this with the device name and unit number, e.g. mfi0:

sys/dev/mrsas/mrsas.c

827

the “mrsas” part here is redundant since device_printf() will prefix this with the device name and unit number, e.g. mrsas0:

[michaelo]

In D54519#1257565, @des wrote:

The conceptual change is correct, probes should be silent.

I agree with you, but I left (added) them on purpose for the sake of completeness of the driver banner. 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?

[des]

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.

[michaelo]

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.

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?

Is that better understandable?

[ziaee]

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

[michaelo]

In D54519#1257776, @ziaee wrote:

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

The assumption whether the dev name is always identical to the driver C file. I don't know this.

[imp]

sys/dev/mfi/mfi.c

375

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.

sys/dev/mrsas/mrsas.c

827

Same comment

[michaelo]

Drop redundant terms

[des]

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?

[michaelo]

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?

Yes, it does. You have correctly replicated my concern.