Re: [PATCH v2] LDT - Linux Driver Template

Re: [PATCH v2] LDT - Linux Driver Template

[Greg KH]

On Thu, Nov 15, 2012 at 09:22:17PM +0200, Constantine Shulyupin wrote:
> From: Constantine Shulyupin <const@MakeLinux.com>
> 
> 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.

> 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.

> --- /dev/null
> +++ b/samples/ldt/ldt.c
> @@ -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.

> + *	The driver demonstrates usage of following Linux facilities:
> + *
> + *	Linux kernel module
> + *	file_operations
> + *		read and write (UART)
> + *		blocking read and write
> + *		polling
> + *		mmap
> + *		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.

> + *	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.

> + *	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.

> +/*
> + *	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.

> +/*
> + *	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.

> +static void ldt_work_func(struct work_struct *work)
> +{
> +	ldt_work_counter++;
> +}

No locking?  Not good.

> +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.

> +#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 :(

> +static DEFINE_MUTEX(ioctl_lock);

Why?

> +static long ldt_ioctl(struct file *f, unsigned int cmnd, unsigned long arg)

No, no new ioctls, don't even think about it, sorry.

> +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.

> +	if (!IS_ERR_OR_NULL(debugfs))
> +		debugfs_remove(debugfs);

Why check this?

> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -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.

>  		{
>  			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?

> --- /dev/null
> +++ b/tools/testing/ldt/ldt-test
> @@ -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"
> +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?


> +
> +tracing_start()
> +{
> +	tracing "echo :mod:ldt > set_ftrace_filter"
> +	tracing "echo function > current_tracer" # need for draw_functrace.py
> +	#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?

> +
> +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"
> +	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?

> +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

Re: [PATCH v2] LDT - Linux Driver Template

[Bjørn Mork]

Greg KH <gregkh@linuxfoundation.org> writes:

>  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

Re: [PATCH v2] LDT - Linux Driver Template

[Constantine Shulyupin]

On Fri, Nov 16, 2012 at 11:46 AM, Bjørn Mork <bjorn@mork.no> wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
>
>>  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)
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.

> 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?

>
> 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.

Re: [PATCH v2] LDT - Linux Driver Template

[Greg KH]

On Fri, Nov 16, 2012 at 10:46:18AM +0100, Bjørn Mork wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
> 
> >  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 agree, this is a much better idea.  Basing any new driver on a
known-working driver is highly preferable.

thanks,

greg k-h

Re: [PATCH v2] LDT - Linux Driver Template

[Constantine Shulyupin]

On Fri, Nov 16, 2012 at 12:40 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 15, 2012 at 09:22:17PM +0200, Constantine Shulyupin wrote:
>> From: Constantine Shulyupin <const@MakeLinux.com>
>>
>> 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:

On Wed, Nov 14, 2012 at 1:32 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 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.

>> 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.


>> + *   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:

On Sun, Oct 7, 2012 at 3:18 PM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
> 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".

> 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.

>
>> +#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 :(
>
>> +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?

>> +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.

> 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!

-- 
Constantine Shulyupin
http://www.MakeLinux.com/
Embedded Linux Systems,
Device Drivers, TI DaVinci