Page MenuHomeFreeBSD
Differential D48941

LinuxKPI: Rework LINUXKPI_PARAM_charp()
Needs ReviewPublic
Actions

Authored by zlei on Tue, Feb 11, 1:03 PM.
Tags
None
Referenced Files
F110243168: D48941.id150913.diff
Sat, Feb 15, 3:44 PM
F110218873: D48941.id150847.diff
Sat, Feb 15, 6:55 AM
Unknown Object (File)
Fri, Feb 14, 9:52 PM
Unknown Object (File)
Fri, Feb 14, 8:30 PM
Subscribers
emaste
imp
kib
olce
wulf

Details

Reviewers
bz
Group Reviewers
linuxkpi
Summary

This was introduced by change [1] but never worked. SYSCTL_STRING
requires a compile-time constant pointer to a string, but
LINUXKPI_PARAM_charp pass in a pointer type char ** .

[1] c1661d59e68e LinuxKPI: add LINUXKPI_PARAM_charp()

Fixes: c1661d59e68e LinuxKPI: add LINUXKPI_PARAM_charp()
MFC after: 1 week

Test Plan

Test preloaded module,

# echo 'compat.linuxkpi.iwlwifi_nvm_file="/tmp/foo"' >> /boot/loader.conf
# echo 'if_iwlwifi_load="YES"' >> /boot/loader.conf
# shutdown -r now
...
# sysctl compat.linuxkpi.iwlwifi_nvm_file
compat.linuxkpi.iwlwifi_nvm_file: /tmp/foo

Test dynamically loaded module,

# kldunload if_iwlwifi
# kenv -u compat.linuxkpi.iwlwifi_nvm_file
# kldload if_iwlwifi
# sysctl compat.linuxkpi.iwlwifi_nvm_file
compat.linuxkpi.iwlwifi_nvm_file:
# kldunload if_iwlwifi
# kenv compat.linuxkpi.iwlwifi_nvm_file=/tmp/bar
# kldload if_iwlwifi
# sysctl compat.linuxkpi.iwlwifi_nvm_file
compat.linuxkpi.iwlwifi_nvm_file: /tmp/bar

Test with slight modified iwlwifi driver, to verify that CTLFLAG_WR works as intended.

diff --git a/sys/contrib/dev/iwlwifi/iwl-drv.c b/sys/contrib/dev/iwlwifi/iwl-drv.c
index b99204d87283..2af0bcbb9bdb 100644
--- a/sys/contrib/dev/iwlwifi/iwl-drv.c
+++ b/sys/contrib/dev/iwlwifi/iwl-drv.c
@@ -2025,7 +2025,7 @@ MODULE_PARM_DESC(amsdu_size,
 module_param_named(fw_restart, iwlwifi_mod_params.fw_restart, bool, 0444);
 MODULE_PARM_DESC(fw_restart, "restart firmware in case of error (default true)");
 
-module_param_named(nvm_file, iwlwifi_mod_params.nvm_file, charp, 0444);
+module_param_named(nvm_file, iwlwifi_mod_params.nvm_file, charp, 0644);
 MODULE_PARM_DESC(nvm_file, "NVM file name");
 
 module_param_named(uapsd_disable, iwlwifi_mod_params.uapsd_disable, uint, 0644);
# sysctl compat.linuxkpi.iwlwifi_nvm_file="/tmp/nvm_file"
compat.linuxkpi.iwlwifi_nvm_file: /tmp/bar -> /tmp/nvm_file

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

zlei created this revision.Tue, Feb 11, 1:03 PM
Herald added subscribers: emaste, imp. · View Herald TranscriptTue, Feb 11, 1:03 PM
zlei requested review of this revision.Tue, Feb 11, 1:03 PM
zlei added a comment.Tue, Feb 11, 1:06 PM

Currently the only one consumer of LINUXKPI_PARAM_charp() is iwlwifi. I do not have the hardware so no am unable to test whether the function to load iwlwifi_nvm_file works or not. This is focusing only loader tunable feature.

kib added inline comments.Tue, Feb 11, 2:10 PM
sys/compat/linuxkpi/common/include/linux/moduleparam.h
129

Is KMEM for having malloc working?

sys/compat/linuxkpi/common/src/linux_compat.c
2777

This probably should be sx_sunlock(). What is the point of sysctlcharplock? What does it protect?

2850

Why reallocating and not reusing the initial alloc? Is it to save memory? I suspect it is not too much economy.

zlei marked an inline comment as done.Tue, Feb 11, 3:25 PM
zlei added inline comments.
sys/compat/linuxkpi/common/include/linux/moduleparam.h
129

Is KMEM for having malloc working?

function linux_tunable_charp_init requires malloc(..., M_KMALLOC), and M_KMALLOC is defined in sys/compat/linuxkpi/common/src/linux_compat.c:

MALLOC_DEFINE(M_KMALLOC, "lkpikmalloc", "Linux kmalloc compat");

while MALLOC_DEFINE in sys/sys/malloc.h:

#ifdef _KERNEL
#define MALLOC_DEFINE(type, shortdesc, longdesc)                        \
        struct malloc_type type[1] = {                                  \
                {                                                       \
                        .ks_next = NULL,                                \
                        .ks_version = M_VERSION,                        \
                        .ks_shortdesc = shortdesc,                      \
                }                                                       \
        };                                                              \
        SYSINIT(type##_init, SI_SUB_KMEM, SI_ORDER_THIRD, malloc_init,  \
            type);                                                      \
        SYSUNINIT(type##_uninit, SI_SUB_KMEM, SI_ORDER_ANY,             \
            malloc_uninit, type)

So YES, SI_SUB_KMEM is required.

142

Is this arbitrary size large enough ?

sys/compat/linuxkpi/common/src/linux_compat.c
2777

This probably should be sx_sunlock(). What is the point of sysctlcharplock? What does it protect?

Ahh, yes, you're right. A copy-paste bug.

2850

The initial alloc may be arbitrary large, I guess it is better to have a max length parameter in LINUXKPI_PARAM_charp()

---#define	LINUXKPI_PARAM_charp(name, var, perm)
+++#define	LINUXKPI_PARAM_charp(name, var, size, perm)

LINUXKPI_PARAM_charp() actually emulate the linux version, I'm not sure if a new parameter size is proper.

zlei marked an inline comment as done.Tue, Feb 11, 3:34 PM
zlei added inline comments.
sys/compat/linuxkpi/common/src/linux_compat.c
2775

Emm, missing the null terminator, but sysctl(8) tolerates this.

zlei updated this revision to Diff 150847.Tue, Feb 11, 4:02 PM

Fixed off-by-one error. Fixed sx_sunlock().

zlei marked an inline comment as done.Tue, Feb 11, 4:05 PM
wulf added a subscriber: wulf.Tue, Feb 11, 4:34 PM

There is prior work to fix that D34252

zlei added a comment.Wed, Feb 12, 2:30 PM
In D48941#1116247, @wulf wrote:

There is prior work to fix that D34252

Emm, I did not realize that a prior work exists. Should I continue working on this one, or wait for D34252 ?

wulf added a comment.Wed, Feb 12, 2:48 PM
In D48941#1116640, @zlei wrote:

Emm, I did not realize that a prior work exists. Should I continue working on this one, or wait for D34252 ?

I do not know. It looks abandoned

zlei updated this revision to Diff 150913.Wed, Feb 12, 5:13 PM
  1. Avoided reallocating in function linux_tunable_charp_init().
  2. Fixed retrying a snapshot
  3. Fixed infinite loop ( Not sure why previous runtime test not cover that, maybe compiler is smart ? )
  4. Removed max length limit.
zlei marked an inline comment as done.Wed, Feb 12, 5:15 PM
zlei edited the test plan for this revision. (Show Details)Wed, Feb 12, 5:32 PM
zlei added a comment.Thu, Feb 13, 5:06 PM

@kib Does the latest revision look good to you ?

kib added inline comments.Sat, Feb 15, 1:57 PM
sys/compat/linuxkpi/common/src/linux_compat.c
2786

Why not simply srtdup() under the slock?

2909

This is not a usable name. The first six characters of the sx name are displayed for wmesg of the sleeping thread by the userspace tools like ps and top. The name should be short and pref. not contain spaces.

zlei added inline comments.Sat, Feb 15, 5:08 PM
sys/compat/linuxkpi/common/src/linux_compat.c
2786

I think you're right, sx(9) may be held during unbounded sleep, so it is safe to malloc(M_WAITOK) while holding sx(9).

2909

This is not a usable name. The first six characters of the sx name are displayed for wmesg of the sleeping thread by the userspace tools like ps and top.

Is that actually eight characters ?

% grep -r 'WMESGLEN' sys/sys
sys/sys/user.h:#define	WMESGLEN	8		/* size of returned wchan message */
sys/sys/user.h:	char	ki_wmesg[WMESGLEN+1];	/* wchan message */
# ps -alx
 UID   PID  PPID C PRI NI   VSZ   RSS MWCHAN   STAT TT        TIME COMMAND
...
   0     3     0 2 -16  0     0    80 crypto_w DL    -     0:00.00 [crypto]
...

The name should be short and pref. not contain spaces.

What about this one,

---	sx_init(&sysctlcharplock, "linux sysctl charp handler");
+++	sx_init(&sysctlcharplock, "lkpi-sysctl-charp-handler");

The short version "lkpi-sys" has not been used in sys/compat/linuxkpi yet.

Revision Contents
Changeset List

PathSize
sys/
compat/
linuxkpi/
common/
include/
linux/
36 lines
src/
129 lines

Diff 150913

View Options

sys/compat/linuxkpi/common/include/linux/moduleparam.h

Loading...
View Options

sys/compat/linuxkpi/common/src/linux_compat.c

Loading...