Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GSoC Warm Up Kernel Task #234

Open
wants to merge 17 commits into
base: 5.4-rt
Choose a base branch
from
Open

GSoC Warm Up Kernel Task #234

wants to merge 17 commits into from

Conversation

NiklasWan
Copy link

This is the pull request for Media Ip Streaming project, which implements a dummy kernel driver for the GSoC 2020 warm up task.

RobertCNelson and others added 17 commits March 10, 2020 13:24
patch-5.4.19-rt11.patch.xz

Signed-off-by: Robert Nelson <[email protected]>
Reference: v5.6-rc5
Signed-off-by: Robert Nelson <[email protected]>
Reference: v5.5.8
Signed-off-by: Robert Nelson <[email protected]>
Copy link
Contributor

@dlech dlech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good luck with your project. Looks like you are off to a good start.

I just wanted to share a few housekeeping things I learned from contributing to the Linux kernel (see inline comments).

config GSOC_DUMMY_CHAR_DEVICE
tristate "GSoC Warmup Driver"
help
This builds the GSoC Dummy Char Driver, which is required for the warmup task.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help lines should be indented with 1 tab + 2 spaces

only mixes the entropy pool.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watch out for unintentional changes like editors that automatically add a newline at the end of the file.

* Dummy character driver by
* John Madieu <[email protected]>
*
* This program is free software; you can redistribute it and/or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using SPDX license identifier is used nowdays instead of license paragraphs.

https://www.kernel.org/doc/html/latest/process/license-rules.html

#include <linux/cdev.h>
#include <linux/uaccess.h>

static int gsoc_char_dev_open(struct inode * inode, struct file * file);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably depends on the area of the kernel but most code I have seen tries to avoid forward declarations if possible by rearranging the code.

// register a range of char dev numbers 0 to 1 in this case
err = alloc_chrdev_region(&gsoc_dev, 0, 1, "gsoc_dummy_char_dev");
if (err < 0) {
pr_err("Can't get major number\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good idea to include the module name in the error message so users know which module the error came from.

static ssize_t gsoc_char_dev_read(struct file *file, char __user * buf, size_t count,
loff_t * offset)
{
pr_info("Read function of GSoC dummy driver called.\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be fun to actually put something in the buffer so that you can actually read something from the character device.

@NiklasWan
Copy link
Author

Hey David,

thank you very much for these helpful tricks.

Best,
Niklas

@RobertCNelson RobertCNelson force-pushed the 5.4-rt branch 2 times, most recently from 681c6fa to 3fef3b8 Compare January 5, 2021 03:15
@RobertCNelson RobertCNelson force-pushed the 5.4-rt branch 2 times, most recently from dd2e1d9 to b687de1 Compare March 23, 2021 20:34
@RobertCNelson RobertCNelson force-pushed the 5.4-rt branch 3 times, most recently from a0eb9c6 to f1d27f8 Compare May 21, 2021 18:00
@RobertCNelson RobertCNelson force-pushed the 5.4-rt branch 2 times, most recently from 1296885 to 1ec7015 Compare June 3, 2021 01:44
@RobertCNelson RobertCNelson force-pushed the 5.4-rt branch 2 times, most recently from 35c83cd to 99cce5a Compare September 16, 2021 15:54
RobertCNelson pushed a commit that referenced this pull request Feb 27, 2025
commit 6a97f41 upstream.

die() can be called in exception handler, and therefore cannot sleep.
However, die() takes spinlock_t which can sleep with PREEMPT_RT enabled.
That causes the following warning:

BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 285, name: mutex
preempt_count: 110001, expected: 0
RCU nest depth: 0, expected: 0
CPU: 0 UID: 0 PID: 285 Comm: mutex Not tainted 6.12.0-rc7-00022-ge19049cf7d56-dirty #234
Hardware name: riscv-virtio,qemu (DT)
Call Trace:
    dump_backtrace+0x1c/0x24
    show_stack+0x2c/0x38
    dump_stack_lvl+0x5a/0x72
    dump_stack+0x14/0x1c
    __might_resched+0x130/0x13a
    rt_spin_lock+0x2a/0x5c
    die+0x24/0x112
    do_trap_insn_illegal+0xa0/0xea
    _new_vmalloc_restore_context_a0+0xcc/0xd8
Oops - illegal instruction [#1]

Switch to use raw_spinlock_t, which does not sleep even with PREEMPT_RT
enabled.

Fixes: 76d2a04 ("RISC-V: Init and Halt Code")
Signed-off-by: Nam Cao <[email protected]>
Cc: [email protected]
Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Palmer Dabbelt <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants