Re: [PATCH] LDT - Linux Driver Template

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Nov 13, 2012 at 08:46:37PM +0200, Constantine Shulyupin wrote:
> +++ b/samples/ltd/ldt.c
> @@ -0,0 +1,764 @@
> +/*
> + *	LDT - Linux Driver Template
> + *
> + *	Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/
> + *
> + *	Dual BSD/GPL License

That makes no sense for Linux-specific kernel code, why would you want
it to be dual licensed?  Please fix this.

> + *	Device Model (class, device)

Don't use class code in an example, it is slowly going away from the
whole kernel.

> +#define ctracer_cut_path(fn) (fn[0] != '/' ? fn : (strrchr(fn, '/') + 1))
> +#define __file__	ctracer_cut_path(__FILE__)

Why is this needed?

> +/*
> + *	print_context prints execution context:
> + *	hard interrupt, soft interrupt or scheduled task
> + */
> +
> +#define print_context()	\
> +	pr_debug("%s:%d %s %s 0x%x\n", __file__, __LINE__, __func__, \
> +			(in_irq() ? "harirq" : current->comm), preempt_count());

Ick, no, never do that.

> +#define once(exp) do { \
> +	static int _passed; if (!_passed) { exp; }; _passed = 1; } while (0)

We have macros for this already.

> +#define check(a) \
> +	(ret = a, ((ret < 0) ? pr_warning("%s:%i %s FAIL\n\t%i=%s\n", \
> +	__file__, __LINE__, __func__, ret, #a) : 0), ret)

Why?

> +#define pr_debug_hex(h)	pr_debug("%s:%d %s %s = 0x%lX\n", \
> +	__file__, __LINE__, __func__, #h, (long int)h)

This is not needed at all, just use the proper printk() attribute.

> +#define pr_debug_dec(d)	pr_debug("%s:%d %s %s = %ld\n", \
> +	__file__, __LINE__, __func__, #d, (long int)d)

Why?

> +#define pr_err_msg(m)	pr_err("%s:%d %s %s\n", __file__, __LINE__, __func__, m)

Again, why?

Please don't create your own debugging macros, otherwise people will
copy them.  Use the in-kernel ones, as they are the ones everyone should
use.  And never use the __file__ things, that looks horrible when
building a kernel and makes no sense.

> +static char ldt_name[] = KBUILD_MODNAME;

Why not just use the macro itself?

> +static int bufsize = PFN_ALIGN(16 * 1024);

Why align?

> +static void *in_buf;
> +static void *out_buf;
> +static int uart_detected;
> +void *port_ptr;

Not static?  I'm guessing you didn't run this through sparse?

> +static int port;
> +module_param(port, int, 0);
> +static int port_size;
> +module_param(port_size, int, 0);
> +
> +static int irq;
> +module_param(irq, int, 0);
> +
> +static int loopback;
> +module_param(loopback, int, 0);

No descriptions?  Not good.

I've stopped here, I think this has a bunch more work in order to be a
"correct" example in the kernel source tree.

> +MODULE_LICENSE("Dual BSD/GPL");

Note that modules that touch driver core functions, like this one, can't
really be BSD licensed, sorry.

greg k-h