LDT - Linux Driver Template
Quoted 4 lines · Constantine Shulyupin
From: Constantine Shulyupin <[email protected]>
LDT is useful for Linux driver development beginners, hackers and as starting point for a new drivers.
Really? I strongly doubt it. The odds that a new driver needs to use a misc device is quite low these days. Normally you just start with a driver for a device like the one you need to write and modify it from there. The days when people write char device drivers are pretty much over now.
Quoted 5 lines
The driver uses following Linux facilities: module, platform driver, file operations (read/write, mmap, ioctl, blocking and nonblocking mode, polling), kfifo, completion, interrupt, tasklet, work, kthread, timer, single misc device, Device Model, configfs, UART
0x3f8,
HW loopback, SW loopback, ftracer.
That's a whole lot of different things all mushed together here. If you _really_ want to make something useful, you would do individual drivers for each of these different things, not something all tied together in a way that no one is ever going to use.
samples/ldt/ldt.c line 6
Quoted 9 lines
samples/ldt/ldt.c new file +6
-0@@ -0,0 +1,716 @@ +/* +* LDT - Linux Driver Template +* +* Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/ +* +* GPL License
GPLv1? Cool, I haven't seen that for years and years. Oh, and that's not compatible with GPLv2, sorry.
In short, be explicit.
Quoted 8 lines
samples/ldt/ldt.c new file +8
-0+* The driver demonstrates usage of following Linux facilities: +* +* Linux kernel module +* file_operations +* read and write (UART) +* blocking read and write +* polling +* mmap
samples/ldt/ldt.c line 24
Quoted 10 lines
samples/ldt/ldt.c new file +10
-0+* ioctl +* kfifo +* completion +* interrupt +* tasklet +* timer +* work +* kthread +* simple single misc device file (miscdevice, misc_register) +* multiple char device files (alloc_chrdev_region)
I thought you took this out.
samples/ldt/ldt.c line 28
Quoted 4 lines
samples/ldt/ldt.c new file +4
-0+* debugfs +* platform_driver and platform_device in another module +* simple UART driver on port 0x3f8 with IRQ 4 +* Device Model
Really? I thought this was gone too. And it's not something that a "normal" driver needs.
samples/ldt/ldt.c line 30
Quoted 2 lines
samples/ldt/ldt.c new file +2
-0+* Power Management (dev_pm_ops) +* Device Tree (of_device_id)
Again, that's a lot of non-related things all together in one piece, making it hard to understand, and review, which does not lend itself to being a good example for anything.
samples/ldt/ldt.c line 42
Quoted 12 lines
samples/ldt/ldt.c new file +12
-0+/* +* ldt_received +* Called from tasklet, which is fired from ISR or timer, +* with data received from HW port +*/ + +static void ldt_received (char data) +{ +kfifo_in_spinlocked (&drvdata->in_fifo, &data, +sizeof (data), &drvdata->fifo_lock); +wake_up_interruptible (&drvdata->readable); +}
As others pointed out, if you are going to use function block comments, use the correct kerneldoc style, don't invent your own, as I never want to see this in any driver submitted for inclusion.
samples/ldt/ldt.c line 49
Quoted 7 lines
samples/ldt/ldt.c new file +7
-0+/* +* work section +* +* empty template function for deferred call in scheduler context +*/ + +static int ldt_work_counter;
Again, as others pointed out, you never want static data in a driver.
samples/ldt/ldt.c line 53
Quoted 4 lines
samples/ldt/ldt.c new file +4
-0+static void ldt_work_func (struct work_struct *work) +{ + ldt_work_counter++; +}
No locking? Not good.
samples/ldt/ldt.c line 62
Quoted 9 lines
samples/ldt/ldt.c new file +9
-0+static int ldt_open (struct inode *inode,struct file *file) +{ +pr_debug ("%s: from %s\n" , __func__,current->comm); +pr_debug ("imajor=%d iminor=%d \n" ,imajor (inode),iminor (inode)); +pr_debug ("f_flags & O_NONBLOCK=0x%X\n" , file->f_flags &O_NONBLOCK ); +/* client related data can be allocated here and +stored in file->private_data */ +return 0 ; +}
What's with all of the pr_debug() calls? Why? Why pick only those specific things out of the file structure?
Also, you didn't run this through checkpatch.pl, like was requested.
samples/ldt/ldt.c line 66
Quoted 4 lines
samples/ldt/ldt.c new file +4
-0+#define trace_ioctl (nr) pr_debug("ioctl=(%c%c %c #%i %i)\n", \ + (_IOC_READ & _IOC_DIR(nr)) ? 'r' : ' ', \ + (_IOC_WRITE & _IOC_DIR(nr)) ? 'w' : ' ', \ + _IOC_TYPE(nr), _IOC_NR(nr), _IOC_SIZE(nr))
That's just horrid :(
samples/ldt/ldt.c line 67
Quoted 1 line
samples/ldt/ldt.c new file +1
-0+static DEFINE_MUTEX (ioctl_lock);
Why?
samples/ldt/ldt.c line 68
Quoted 1 line
samples/ldt/ldt.c new file +1
-0+static long ldt_ioctl (struct file *f,unsigned int cmnd,unsigned long arg)
No, no new ioctls, don't even think about it, sorry.
samples/ldt/ldt.c line 72
Quoted 4 lines
samples/ldt/ldt.c new file +4
-0+static int ldt_cleanup (struct platform_device *pdev) +{ +struct ldt_data *drvdata =platform_get_drvdata (pdev); +dev_dbg (&pdev->dev,"%s\n" , __func__);
That's a tracing function, I thought you got rid of these? Hint, anything with __func__ in it should be removed.
samples/ldt/ldt.c line 74
Quoted 2 lines
samples/ldt/ldt.c new file +2
-0+if (!IS_ERR_OR_NULL (debugfs)) +debugfs_remove (debugfs);
Why check this?
scripts/checkpatch.pl sub process { · line 1782
Quoted 8 lines
scripts/checkpatch.pl +1
-1@@ -1779,10 +1779,10 @@ sub process { $rawline !~ /^.\s*\*\s*\@$Ident\s/ && !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ || $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) && - $length > 80) + $length > 90)
Heh, nice try, that's not going to happen.
scripts/checkpatch.pl sub process { · line 1785
Quoted 4 lines
scripts/checkpatch.pl +1
-1{ WARN("LONG_LINE", - "line over 80 characters\n" . $herecurr); + "line ".$length." over 90 characters\n" . $herecurr);
What are you trying to do here? Why are you touching this script at all?
Quoted 18 lines
tools/testing/ldt/ldt-test new file +15
-0@@ -0,0 +1,142 @@ +#!/bin/bash + +# +# LDT - Linux Driver Template +# +# Test script for driver samples/ldt/ +# +# Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/ +# +# Dual BSD/GPL License +# + +RED="\\033[0;31m" +NOCOLOR="\\033[0;39m" +GREEN="\\033[0;32m"
tools/testing/ldt/ldt-test line 25
Quoted 10 lines
tools/testing/ldt/ldt-test new file +10
-0+GRAY="\\033[0;37m" + +set -o errtrace +debugfs=`grep debugfs /proc/mounts | awk '{ print $2; }'` +tracing=$debugfs/tracing + +tracing() +{ + sudo sh -c "cd $tracing; $1" || true +}
sudo? Why?
Quoted 5 lines
tools/testing/ldt/ldt-test new file +5
-0+ +tracing_start() +{ + tracing "echo :mod:ldt > set_ftrace_filter" + tracing "echo function > current_tracer" # need for draw_functrace.py
tools/testing/ldt/ldt-test line 40
Quoted 10 lines
tools/testing/ldt/ldt-test new file +10
-0+ #tracing "echo function_graph > current_tracer" + tracing "echo 1 > function_profile_enabled" + # useful optional command: + #tracing "echo XXX > set_ftrace_notrace" + #sudo cat $tracing/current_tracer + #sudo cat $tracing/set_ftrace_filter + #sudo cat $tracing/function_profile_enabled + # available_filter_functions + # echo $$ > set_ftrace_pid +}
Is this really needed?
Quoted 6 lines
tools/testing/ldt/ldt-test new file +6
-0+ +tracing_stop() +{ + ( echo Profiling data per CPU + tracing "cat trace_stat/function*" )> trace_stat.log && echo trace_stat.log saved + tracing "echo 0 > function_profile_enabled"
tools/testing/ldt/ldt-test line 56
Quoted 10 lines
tools/testing/ldt/ldt-test new file +10
-0+ sudo cp $tracing/trace ftrace.log && echo ftrace.log saved + sudo dd iflag=nonblock if=$tracing/trace_pipe 2> /dev/null > trace_pipe.log || true && echo trace_pipe.log saved + tracing "echo nop > current_tracer" + #export PYTHONPATH=/usr/src/linux-headers-$(uname -r)/scripts/tracing/ + # draw_functrace.py needs function tracer + python /usr/src/linux-headers-$(uname -r)/scripts/tracing/draw_functrace.py \ + < trace_pipe.log > functrace.log && echo functrace.log saved || true +} + +# sudo rmmod parport_pc parport ppdev lp
Why commented out?
tools/testing/ldt/ldt-test line 60
Quoted 4 lines
tools/testing/ldt/ldt-test new file +4
-0+sudo dmesg -n 7 +sudo rmmod ldt ldt_plat_dev 2> /dev/null +sudo dmesg -c > /dev/null +stty -F /dev/ttyS0 115200
Why did you just change my serial port line settings?
What is the goal of this script in the first place?
confused,
greg k-h
Quoted 3 lines · Greg KH
Normally you just start with a
driver for a device like the one you need to write and modify it from there.
Yes.
Even if the template driver is fixed up to be the most beautiful driver ever made, it will still always be made for non-existing hardware. This causes two major problems:
- the driver will not be tested, so it will have bugs
- the driver will not be used by anyone, so it will not be maintained (remember that it is initially perfect, so there is no reason to change it)
May I suggest another approach? How about selecting a set of existing drivers which are suitable as templates, and put all this effort into making those drivers *the* perfect examples instead? Start submitting cleanup patches for the selected drivers until everyone is satisfied and then document them as starting points for anyone wanting to write a similar driver.
I believe many subsystem maintainers already have such sample drivers which they point new submitters to when asked. That does not mean that these drivers necessarily are perfect, so there is still work to do here for anyone interested. And collecting this information and documenting it would be useful in itself.
It would also be nice if hardware availability was considered when selecting the sample drivers. Buying an already supported device to experiment with its driver can be useful even if you have another device you want to write a driver for. Or just for the learning experience.
Just my € .02
Bjørn
Quoted 8 lines · Bjørn Mork
Quoted 3 lines · Greg KH
Normally you just start with a
driver for a device like the one you need to write and modify it from there.Yes.
Quoted 7 lines
Even if the template driver is fixed up to be the most beautiful driver ever made, it will still always be made for non-existing hardware. This causes two major problems:
- the driver will not be tested, so it will have bugs
- the driver will not be used by anyone, so it will not be maintained (remember that it is initially perfect, so there is no reason to change it)
Thanks. I seems you have missed.
The main advantage of LDT - is working driver with real HW and simple test suite.
It implements trivial UART driver just to write to port, receive real HW interrupt
and read data from port. (You can to suggest to use any another HW). Without available UART, LDT emulates loopback in SW for testing. Memory buffers are used for mmap and ioctl operations.
LDT test script ldt-test and test utility dio.c configures the driver for loopback mode, passes data to the driver, receives back and compares with input and gives result of comparison.
To perform validation tests, regression tests need simply to run test script. Detailed kernel log and ftrace log during the test are saved for analysts.
Quoted 6 lines
May I suggest another approach? How about selecting a set of existing drivers which are suitable as templates, and put all this effort into making those drivers *the* perfect examples instead? Start submitting cleanup patches for the selected drivers until everyone is satisfied and then document them as starting points for anyone wanting to write a similar driver.
Thank you. Possible too. Can you or somebody else recommend such drivers?
Quoted 6 lines
I believe many subsystem maintainers already have such sample drivers which they point new submitters to when asked. That does not mean that these drivers necessarily are perfect, so there is still work to do here for anyone interested. And collecting this information and documenting it would be useful in itself.
Thanks. I already research omap panda platform for improvement opportunities.
Thank you.
Quoted 16 lines · Bjørn Mork
Quoted 3 lines · Greg KH
Normally you just start with a
driver for a device like the one you need to write and modify it from there.Yes.
Even if the template driver is fixed up to be the most beautiful driver ever made, it will still always be made for non-existing hardware. This causes two major problems:
- the driver will not be tested, so it will have bugs
- the driver will not be used by anyone, so it will not be maintained (remember that it is initially perfect, so there is no reason to change it)
Quoted 6 lines
May I suggest another approach? How about selecting a set of existing drivers which are suitable as templates, and put all this effort into making those drivers *the* perfect examples instead? Start submitting cleanup patches for the selected drivers until everyone is satisfied and then document them as starting points for anyone wanting to write a similar driver.
I agree, this is a much better idea. Basing any new driver on a known-working driver is highly preferable.
thanks,
greg k-h
Quoted 11 lines · Greg KH
Quoted 4 lines · Constantine Shulyupin
From: Constantine Shulyupin <[email protected]>
LDT is useful for Linux driver development beginners, hackers and as starting point for a new drivers.
Really? I strongly doubt it. The odds that a new driver needs to use a misc device is quite low these days. Normally you just start with a driver for a device like the one you need to write and modify it from there. The days when people write char device drivers are pretty much over now.
In a recent letter you have discouraged to use of class_create and recommended to use misc interface:
Quoted 5 lines · Greg KH
Now I agree using the char interface isn't the most "obvious" and I have a set of ideas/half-baked patches floating around that aim to clean it up, but for now, I'd recommend just using the misc interface, it's worlds simpler, makes sense, and handles all of the struct device work for you automatically.
That is exact what I think too.
Quoted 10 lines
Quoted 5 lines
The driver uses following Linux facilities: module, platform driver, file operations (read/write, mmap, ioctl, blocking and nonblocking mode, polling), kfifo, completion, interrupt, tasklet, work, kthread, timer, single misc device, Device Model, configfs, UART
0x3f8,
HW loopback, SW loopback, ftracer.That's a whole lot of different things all mushed together here. If you _really_ want to make something useful, you would do individual drivers for each of these different things, not something all tied together in a way that no one is ever going to use.
Yes, LDT uses many of Linux facilities, which are used in different drivers. An advantage ot LDT is integration this facilities.
It is easy to remove unused parts of LDT for tailoring it to specific requirement.
Any way, I'll be glad to make a sample driver accordingly your specification. May be it is just need to remove some facilities and simplify LDT.
Quoted 6 lines
Quoted 2 lines
+ * Power Management (
dev_pm_ops)
+ * Device Tree (of_device_id)Again, that's a lot of non-related things all together in one piece, making it hard to understand, and review, which does not lend itself to being a good example for anything.
Indeed, it is a challenge to make it complete, simple and useful. Many reviewers are suggesting different ideas.
For example:
Quoted 2 lines · Jean-Christophe PLAGNIOL-VILLARD
They are not a "good practice example" as you miss too many point about drivers such as PM, Runtime-PM, Devicetree etc...
I am still weighting what to include and to exclude from LDT to approach it to "good practice example".
Quoted 1 line
Also, you didn't run this through checkpatch.pl, like was requested.
I do it each time. I just allow to to some lines be few longer than 80 symbols, accordingly
your presentation "Write and Submit your first Linux kernel Patch"
https://www.youtube.com/watch?v=LLBrBBImJt4 on 10:00.
Quoted 11 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+#define trace_ioctl(nr) pr_debug("ioctl=(%c%c %c #%i %i)\n", \ + (_IOC_READ & _IOC_DIR(nr)) ? 'r' : ' ', \ + (_IOC_WRITE & _IOC_DIR(nr)) ? 'w' : ' ', \ + _IOC_TYPE(nr), _IOC_NR(nr), _IOC_SIZE(nr))That's just horrid :(
Quoted 1 line
+static long
ldt_ioctl(struct file *f, unsigned int cmnd, unsigned long arg)No, no new ioctls, don't even think about it, sorry.
Do you mean that new drivers should not provide ioclt interface? What should be used instead?
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+tracing() +{ + sudo sh -c "cd $tracing; $1" || true +}sudo? Why?
script ldt-test is intended to run from user, but ftrace and insmod require root access.
Quoted 1 line
What is the goal of this script in the first place?
The goal of ldt-test script is to validate LDT driver.
Thank you very much!
Signature · 4 lines
Constantine Shulyupin http://www.MakeLinux.com/ Embedded Linux Systems, Device Drivers, TI DaVinci