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.
Details
Details
- Reviewers
jrm manu bz markj • hselasky - Group Reviewers
Linux Emulation - Commits
- rG2d1dfdab0950: linuxkpi: Resolve duplicate global symbol name to fix LINT kernel build.
rGa8e31ab2e93f: linuxkpi: drm-kmod debugfs support
rGcbda8bed15a0: linuxkpi: Resolve duplicate global symbol name to fix LINT kernel build.
rGf697b9432d9c: linuxkpi: drm-kmod debugfs support
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| sys/compat/lindebugfs/lindebugfs.c | ||
|---|---|---|
| 145 | if sb->s_size == 0 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. | |
Comment Actions
@jfree if you generate another diff to update this review please include full context, e.g. git diff -U999999 or git show -U999999
Comment Actions
- Moved static inline functions out of debugfs.h and into lindebugfs.c.
- Removed unnecessary functions from debugfs.h.
- Generated diff with -U999999 flag.
| 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. | |
| 215 | What does "this behaviour" refer to exactly? | |
| sys/compat/linuxkpi/common/include/linux/fs.h | ||
| 328 | Why is this commented out? | |
| sys/compat/linuxkpi/common/include/linux/string.h | ||
| 108 | ||
| 113 | I think this could probably be committed separately? | |
| sys/compat/linuxkpi/common/src/linux_seq_file.c | ||
| 52 | 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? | |
| sys/compat/lindebugfs/lindebugfs.c | ||
|---|---|---|
| 399 | If there is no return value, you can skip the return statement. Ditto for the other cases below. | |
This comment was removed by jfree.
Comment Actions
- 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 | ||
|---|---|---|
| 147 | In this error case you should call d->dm_fops->release() before returning ? In other words, set "rc" and goto below ? | |
Comment Actions
Fixed memory leak where lf was not released due to a potential failed malloc() during read. Thanks to @hselasky for pointing this out.
Comment Actions
- Created default_llseek() function in substitution of macro
- removed i_size_write() comment
Comment Actions
There's more to fix in this (debugfs) world now it seems beyond the comment. Is anyone still interested please let me know.
| sys/compat/lindebugfs/lindebugfs.c | ||
|---|---|---|
| 146 | This doesn't work for anything but your specific case most likely. The read implementations are expecting a user address and not a kernel one returned from malloc; this makes a lot of debugfs implementations fail with -EFAULT. | |
Comment Actions
I tested with FreeBSD seq_file and simple attribute file implementations. Both of these were modified in this patch to accommodate the kernel buffer. I did not realize that there were more implementations that are commonly used. This is my mistake, so I would be happy to finish the work. Sorry for any inconvenience that I may have caused. I'll start right away.
| sys/compat/lindebugfs/lindebugfs.c | ||
|---|---|---|
| 145 | The buf[0] = '\0' exists for situations where nothing is written into buf. A good example is the seq_file ->read() implementation which uses seq_printfs to print into its local sbuf instead of writing into buf. Other ->read() implementations write file contents into buf and return their own error codes when read failures occur. I could do if (sbuf_len(sb) == 0) { sbuf_bcpy(sb, buf, strlen(buf)) } to see if a seq_printf went through and sbuf_bcpy based on that, but I feel like the existing code is more readable. Either way, I need the buf[0] = '\0' for strlen(). Please tell me if I am misunderstanding. | |
| 147 | If the length is equal to the buffer size, the if statement will go through. Perhaps I am not understanding what you're suggesting? The compiler (assuming optimizations are turned on) should optimize this out and cache the strlen() value. | |
| 147 | Absolutely. Silly mistake on my part. Thanks for pointing that out. | |
| sys/compat/linuxkpi/common/include/linux/debugfs.h | ||
| 127 | Got it. I will change that! :) | |
| sys/compat/linuxkpi/common/src/linux_seq_file.c | ||
| 52 | Yes. | |
| sys/compat/linuxkpi/common/src/linux_simple_attr.c | ||
| 126 | After some investigation, it appears that strscpy will error out during a zero byte transfer. I am going to apply your suggestion to make the error message more transparent. Thanks. | |
| 176 | I do agree. I'm also changing sizeof(data) to strlen(buf) since this function applies the offset to buf, not data. Thanks for pointing that out. | |
| 183 | The buffer was meant to be a userspace pointer at one time, but lindebugfs uses sbufs to transfer data. I am under the impression that the user to kernel space conversion has already been done by pseudofs in pseudofs_vnops.c:1082. If I use LinuxKPI's copy_from_user() on the data in buf, I get errors. I will remove the misleading __user. The mutex is necessary if multiple simple_attr_write()s are executed at the same time asynchronously. We don't want multiple sources overwriting each other. | |
Comment Actions
There is a lot of code using simple_open with a private read function which at the end simply calls simple_read_from_buffer() or even calls the copy_to_user() or for write copy_from_user() itself (e.g. when doing memory dumps). simple_read_from_buffer() as implemented here will also call the linux copy_to_user().
I am under the impression that it should be the implementation's problem to handle that bit and how to hide it (like seq_file does working on the sbuf)?
PS: I am also looking at debugfs_create_blob() and some other bits. If you are happy I'll try to get bits into here Saturday.
Comment Actions
Bjoern,
At second glance, you are certainly right. It is the implementation's responsibility to handle the user -> kernel buffer conversion.
I'm not at my desktop right now, but some quick intuition leads me to believe that buf = uio->uio_iov[0].iov_base; might grab
the appropriate user buffer. I will test this when I get home.