Page MenuHomeFreeBSD

LinuxKPI drm-kmod debugfs support
ClosedPublic

Authored by jfree on Jul 23 2022, 1:55 AM.

Details

Summary

This diff extends LinuxKPI to support simple attribute files in debugfs. These simple attributes are an essential component for compiling drm-kmod with CONFIG_DEBUG_FS enabled. This will allow for easier graphics driver debugging using Intel's igt-gpu-tools.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jfree requested review of this revision.Jul 23 2022, 1:55 AM
hselasky added inline comments.
sys/compat/lindebugfs/lindebugfs.c
145

if sb->s_size == 0
return (-EINVAL);

or

MPASS(sb->s_size != 0);

147

What if the length is equal to the buffer size?

You are repeating strlen(buf), should cache value in a variable.

sys/compat/linuxkpi/common/include/linux/debugfs.h
127

BSD style: All return values should be wrapped by ()'s.

jfree marked 2 inline comments as done.
jfree edited the summary of this revision. (Show Details)

Added parenthesis around return values per FreeBSD kernel style guide

@jfree if you generate another diff to update this review please include full context, e.g. git diff -U999999 or git show -U999999

jfree marked an inline comment as done.
  • Moved static inline functions out of debugfs.h and into lindebugfs.c.
  • Removed unnecessary functions from debugfs.h.
  • Generated diff with -U999999 flag.

Minor modifications to lindebugfs.c

Added manual page for lindebugfs

Return positive values in lindebugfs.c so pseudofs vnops does not get confused.

share/man/man5/lindebugfs.5
2

We don't include "$FreeBSD$" in new files. Typically you'd add a copyright statement instead of a simple "written by."

19

Isn't lindebugfs solely for supporting drivers using the LinuxKPI? If so I'd suggest stating that directly. In other words, this is not a general-purpose filesystem but rather 1) exists mostly for the benefit of developers, and 2) only for LinuxKPI-based drivers.

53

Oh, this filesystem should really be called "lindebugfs".

70

You need to update share/man/man5/Makefile to ensure that this makefile gets installed.

sys/compat/lindebugfs/lindebugfs.c
122

Declaring variable-length arrays on the stack is not a good idea. Kernel threads have limited stack space and only primitive means of detecting stack overflow (one guard page at the end of the stack). You should malloc() a temporary buffer instead.

216

What does "this behaviour" refer to exactly?

sys/compat/linuxkpi/common/include/linux/fs.h
332

Why is this commented out?

sys/compat/linuxkpi/common/include/linux/string.h
108 ↗(On Diff #109621)
113 ↗(On Diff #109621)

I think this could probably be committed separately?

sys/compat/linuxkpi/common/src/linux_seq_file.c
56

Is this a bug fix?

sys/compat/linuxkpi/common/src/linux_simple_attr.c
61

Style, continuation lines should be indented by four spaces.

183

buf has a __user annotation, which indicates that buf is a userspace pointer. If that's actually the case (i.e., the annotation is correct), then we must not dereference the user pointer directly. Data should be copied into the kernel's address space first, then kstrtoull() can be called.

Also I don't think this call needs to happen under the mutex?

jfree marked 11 inline comments as done.

Updated diff per markj@'s review and comments

Fixed function parameter list indentation

sys/compat/lindebugfs/lindebugfs.c
399

If there is no return value, you can skip the return statement. Ditto for the other cases below.

jfree marked an inline comment as not done.

Removed return statements from void functions per hselasky's advice

sys/compat/linuxkpi/common/src/linux_simple_attr.c
125

off-by-one ??

175

I think this is off-by-one. Should be:

*ppos >= sizeof(data)

Do you agree?

jfree marked 2 inline comments as done.

Corrected pointer position errors in linux_simple_attr.c thanks to hselasky@

jfree marked an inline comment as done.
This comment was removed by jfree.
  • Updated seq_file seq_read() to report number of bytes read correctly
  • Adjusted lindebugfs.c module declaration - this is done through the PSEUDOFS() macro
  • Changed internal debugfs filesystem name to lindebugfs
  • Modified man page to reflect changes
  • Corrected off-by-one errors in linux_simple_attr.c
sys/compat/lindebugfs/lindebugfs.c
143

In this error case you should call d->dm_fops->release() before returning ?

In other words,

set "rc" and goto below ?

jfree marked an inline comment as done.

Fixed memory leak where lf was not released due to a potential failed malloc() during read. Thanks to @hselasky for pointing this out.

Fixed off-by-one error in linux_seq_file.c

sys/compat/linuxkpi/common/include/linux/fs.h
301

Can you make this a static inline function, instead of using a macro?

static inline loff_t
default_llseek(....)

This avoids the macro, which may match more than it should.

332

Please remove this comment.

jfree marked 2 inline comments as done.
  • Created default_llseek() function in substitution of macro
  • removed i_size_write() comment
jfree marked an inline comment as done.
  • Removed unnecessary code from linux_simple_attr.c

Looks good to me. Make sure you bump the FreeBSD version along this commit.

This revision is now accepted and ready to land.Fri, Sep 2, 9:05 AM
This revision was automatically updated to reflect the committed changes.