modules: increase MAXMODNAME and provide backward compat

[bz]

Summary

With various firmware files used by graphics and wireless drivers
we are exceeding the current 32 character module name (file path
in kldxref) length.
In order to overcome this issue bump it to the maximum path length
for the next version.
To be able to MFC provide backward compat support for another version
of the struct as the offsets for the second half change due to the
array size increase.

MAXMODNAME being defined to MAXPATHLEN needs param.h to be
included first. With only 7 modules (or LinuxKPI module.h) not
doing that adjust them rather than including param.h in module.h [1].

Reported by: Greg V (greg unrelenting.technology)
Sponsored by: The FreeBSD Foundation
Suggested by: imp [1]
MFC after: 10 days
Differential Revision: https://reviews.freebsd.org/D32383

[bz]

I'd be cautious about this change and I will wonder if you change it to what number as you don't want to do it again in a few weeks.
This was 70 (71) characters already: /boot/kernel/brcm_brcmfmac43455_sdio_raspberrypi_3_model_b_plus_txt.ko just as an example including the /boot/kernel/.

[manu]

I'm not sure that this can be MFC'ed so that will means no newer AMD gpu on 13 ever.
I wonder if we shouldn't just work on having subdirs in /boot/modules/ so we just stopped concat the dir name to the module name.

[bz]

We might be able to MFC this but should do it properly by providinv compat code but I haven't looked at all consumers yet; I am planning to do this probably next week.

[bz]

Update with what I believe will work but does not yet change the facts.
If we can be sure this works in the current and future incarnation then
this should be good to go. Putting it out for review ans testing.
I know it compiles and if I got things right all other consumers should
still be fine.

There are three more changes: flip the #define and remove the two CTASSERTs.

[bz]

Please add other reviewers as you see fit!

[val_packett.cool]

(huh, you updated the revision without commandeering it? is that due to some admin rights? kinda confusing that it still belongs to me, please do click "commandeer revision" :D)

[bz]

Sorry, I figured updating yours is easier to track than opening a 2nd one. Found that button; I have no admin rights.

[bz]

If we'll make the switch we need param.h for MAXPATHLEN and it seems a lot
of modules are not actually including this so putting it here rather than
everywhere else seems more logical.

[imp]

In D32383#747216, @bz wrote:

If we'll make the switch we need param.h for MAXPATHLEN and it seems a lot
of modules are not actually including this so putting it here rather than
everywhere else seems more logical.

I'm skeptical of this statement. Almost everything includes param.h, either directly
or indirectly via systm.h. Which modules aren't doing that? A quick grep
of the .depend files didn't turn up anything obvious... There were 6100
.depend files, all but 150 listed sys/param.h, the bulk were either firmware
(which are binary blobs) or other things not really kernel dependent...

Which ones did you find it missing for?

[bz]

In D32383#747219, @imp wrote:

In D32383#747216, @bz wrote:

If we'll make the switch we need param.h for MAXPATHLEN and it seems a lot
of modules are not actually including this so putting it here rather than
everywhere else seems more logical.

I'm skeptical of this statement. Almost everything includes param.h, either directly
or indirectly via systm.h. Which modules aren't doing that? A quick grep
of the .depend files didn't turn up anything obvious... There were 6100
.depend files, all but 150 listed sys/param.h, the bulk were either firmware
(which are binary blobs) or other things not really kernel dependent...

Which ones did you find it missing for?

I don't have a full list as I stopped at some point on Thu and came back to that window earlier today with another kernel build failed; I remember sys/x86/cpufreq/hwpstate_intel.c and then some file systems bits. If you think it's worthwhile to find the remaining few (by your statement) and add the include there I am happy to spend the time and wade through all them that I can find in first run and then start a universe to find the rest.

Do you think the remained bits of this change are mostly sane to do (this way or another)?

[imp]

We can likely drop V1 modstat entirely at this point. It dates from ~2000 and nothing in the tree uses it. kldload/stat/etc from that time frame has little to no chance of working today.
It appears to be a transition between FreeBSD 3.x and FreeBSD 4.x compatibility.
We may need the v2 -> v3 transition, though, since failure scenarios may require running with old binaries to load new kernel modules while rebuilding,
so that code is likely good.
I can prep a patch to remove v1 if that would make your life easier.
I've downloaded the patch and built amd64 GENERIC successfully (meta mode) so I'm not seeing where the problem here is locally, so can't comment on your errors without more info.

[bz]

In D32383#747221, @imp wrote:

We can likely drop V1 modstat entirely at this point. It dates from ~2000 and nothing in the tree uses it. kldload/stat/etc from that time frame has little to no chance of working today.
It appears to be a transition between FreeBSD 3.x and FreeBSD 4.x compatibility.

The V1 seems no extra hassle as all if() need to be present for V2 anyway.

We may need the v2 -> v3 transition, though, since failure scenarios may require running with old binaries to load new kernel modules while rebuilding,
so that code is likely good.

Good.

I can prep a patch to remove v1 if that would make your life easier.

Should we?

I've downloaded the patch and built amd64 GENERIC successfully (meta mode) so I'm not seeing where the problem here is locally, so can't comment on your errors without more info.

won't show up until you actually flip the `#define` and remove the two CTASSERTs.

Seems the list for amd64 is smaller than I thought in the end and I just stopped slightly too early; at least two more are caught by the linuxkpi module.h change:
modified: sys/compat/linuxkpi/common/include/linux/module.h
modified: sys/contrib/rdma/krping/krping_dev.c
modified: sys/dev/videomode/videomode.c
modified: sys/fs/fuse/fuse_device.c
modified: sys/fs/fuse/fuse_io.c
modified: sys/fs/fuse/fuse_main.c
modified: sys/x86/cpufreq/hwpstate_intel.c

So yes, the param.h can be avoided. Thanks for making me go the full way; I'll do a full universe over night and see what else comes up.

[bz]

modules: add param.h for MAXMODNAME to modules to avoid compile breakage
when MAXMODNAME is redefined to MAXPATHLEN.

[bz]

modules: flip MAXMODNAME over from V1V2 to V3.

Remove the CTASSERTs initially put in place to make sure v2 stayed the
same as we did not break it.  Flip MAXMODnaME over to V3 and with that
there is no more need to change kldxref.

[bz]

I've done the last two updates in two steps to be able to see the individual changes in Pabricator by comparing the individual versions.

This one passed a tinderbox last night.

[• hselasky]

sys/kern/kern_module.c

492

missing empty line here?

sys/sys/module.h

259

missing empty newline before struct ?

[bz]

@imp while it doesn't bother me for this change, do you want (me) to rip out V1 support?

[imp]

In D32383#750314, @bz wrote:

@imp while it doesn't bother me for this change, do you want (me) to rip out V1 support?

I'd prefer that, but I'm also OK with you committing w/o doing that and I can do that later.

[bz]

Okay. Last calls for this one. Will go in before the end of the week.
If you think someone else should have a look still please add to Reviewers.

Someone (possibly I) will rip V1 support in HEAD after that.

[bz]

Update with the 2 extra empty lines and all bits flipped to final.

[emaste]

Looks reasonable (I have not reviewed in full detail)