linux-kernel - Re: [PATCH 01/18] lirc core device driver infrastructure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 9 Sep 2008 11:53:48 +0200
From:	Janne Grunau <j@...nau.net>
To:	Sebastian Siewior <lkml@...breakpoint.cc>
Cc:	Jarod Wilson <jwilson@...hat.com>, linux-kernel@...r.kernel.org,
	Jarod Wilson <jarod@...hat.com>,
	Christoph Bartelmus <lirc@...telmus.de>,
	Mario Limonciello <superm1@...ntu.com>,
	Janne Grunau <j@...nau.net>
Subject: Re: [PATCH 01/18] lirc core device driver infrastructure

On Tuesday 09 September 2008 09:40:18 Sebastian Siewior wrote:
> >diff --git a/drivers/input/lirc/Makefile
> > b/drivers/input/lirc/Makefile new file mode 100644
> >index 0000000..cdb4c45
> >--- /dev/null
> >+++ b/drivers/input/lirc/Makefile
> >@@ -0,0 +1,8 @@
> >+# Makefile for the lirc drivers.
> >+#
> >+
> >+# Each configuration option enables a list of files.
> >+
> >+EXTRA_CFLAGS =-DIRCTL_DEV_MAJOR=61 -DLIRC_SERIAL_TRANSMITTER
> > -I$(src)

> LIRC_SERIAL_TRANSMITTER is used in patch 2 and just to enable a
> module options. Since it is always the case, please remove it.

Added CONFIG_LIRC_SERIAL_TRANSMITTER to Kconfig and removed this from 
the Makefile

> I haven't found the source of $src. It is probably a relic.

removed

> >+obj-$(CONFIG_LIRC_DEV)		+= lirc_dev.o
> >diff --git a/drivers/input/lirc/lirc_dev.c
> > b/drivers/input/lirc/lirc_dev.c new file mode 100644
> >index 0000000..c8f325c
> >--- /dev/null
> >+++ b/drivers/input/lirc/lirc_dev.c
> >@@ -0,0 +1,809 @@
...
> >+
> >+#include <linux/version.h>
>
> You souldn't need this one
>
> >+
> >+#include <linux/autoconf.h>
>
> this as well.

fixed, in all other files as well.

> >+#include <linux/semaphore.h>
>
> haven't found any call of down(). Do you really need this?

A relict. Lirc used many single holder semaphore and I forgot to remove 
the include after converting them to mutexes. I'll check the other 
files too.

> >+#define __KERNEL_SYSCALLS__
>
> this define shouldn't be here, should it?

Probably not, but it doesn't make a difference for the current kernel.
The only other hit for this symbol is arch/xtensa/kernel/syscall.c
38:#undef  __KERNEL_SYSCALLS__

...

> >+static inline void init_irctl(struct irctl *ir)
>
> this inline can go in my opinion. The others here as well. It is not
> *that* performance critical.

There is only one other inline. I doubt it makes a difference there but 
I won't change it without test. It does for the object size since the 
function is only called from one place.

...

> >+
> >+	bytes_in_key = p->code_length/8 + (p->code_length%8 ? 1 : 0);
>
> did you actually pass checkpatch.pl ?

yes, and while this is ugly I see no obvious CodingStyle violation.

is following better?

bytes_in_key = p->code_length/8;
if (p->code_length%8)
	bytes_in_key++;

> >+EXPORT_SYMBOL(lirc_register_plugin);
>
> Is EXPORT_SYMBOL_GPL() possible?

Afaik yes, all drivers using this symbol are in this patchset and under 
GPL. If it is desireable to to use EXPORT_SYMBOL_GPL() has someone else 
to decide. Christoph?


> >+/*
> >+ * Overrides for Emacs so that we follow Linus's tabbing style.
> >+ *
> > -------------------------------------------------------------------
> >-------- + * Local variables:
> >+ * c-basic-offset: 8
> >+ * End:
> >+ */
>
> Please don't enforce formating that way.

Removed, from all other files as well.

Thanks for the review. We will address the misssing comments later

Janne
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/