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 13:35:17 +0200
From:	Janne Grunau <j@...nau.net>
To:	Andi Kleen <andi@...stfloor.org>
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 11:46:48 Andi Kleen wrote:
> Jarod Wilson <jwilson@...hat.com> writes:
>
> Just some quick comments, no complete review
>
> > +EXTRA_CFLAGS =-DIRCTL_DEV_MAJOR=61 -DLIRC_SERIAL_TRANSMITTER
> > -I$(src)
>
> Put the Ds into some include instead?

LIRC_SERIAL_TRANSMITTER is already gone, not sure that should happen to 
IRCTL_DEV_MAJOR but it could go into lirc.h. moved

> > +
> > +static int debug;
> > +#define dprintk(fmt, args...)					\
> > +	do {							\
> > +		if (debug)					\
> > +			printk(KERN_DEBUG fmt, ## args);	\
> > +	} while (0)
>
> There's some effort to replace that all with pr_printk

pr_printk is not found in include/ nor by google. please explain

> > +	dprintk(LOGHEAD "poll thread started\n", ir->p.name,
> > ir->p.minor); +
> > +	do {
> > +		if (ir->open) {
> > +			if (ir->jiffies_to_wait) {
> > +				set_current_state(TASK_INTERRUPTIBLE);
> > +				schedule_timeout(ir->jiffies_to_wait);
> > +			} else {
> > +				interruptible_sleep_on(
> > +					ir->p.get_queue(ir->p.data));
>
> sleep_on is really discouraged, consider replacing it with
> wait_event()

done

> > +			}
> > +			if (kthread_should_stop())
> > +				break;
> > +			if (!add_to_buf(ir))
> > +				wake_up_interruptible(&ir->buf->wait_poll);
> > +		} else {
> > +			/* if device not opened so we can sleep half a second */
> > +			set_current_state(TASK_INTERRUPTIBLE);
> > +			schedule_timeout(HZ/2);
>
> This means you always wake up every half a second? That will not make
> the powertop users happy. When there's nothing to do it should just
> sleep.

done

> The checks in plugin_register seem a bit extreme.
> At least make that all WARN_ONs
>
> > +
> > +	/* end up polling thread */
> > +	if (ir->task) {
> > +		wake_up_process(ir->task);
> > +		kthread_stop(ir->task);
> > +	}
> > +
> > +	dprintk("lirc_dev: plugin %s unregistered from minor number =
> > %d\n", +		ir->p.name, ir->p.minor);
> > +
> > +	ir->attached = 0;
> > +	if (ir->open) {
> > +		dprintk(LOGHEAD "releasing opened plugin\n",
> > +			ir->p.name, ir->p.minor);
> > +		wake_up_interruptible(&ir->buf->wait_poll);
>
> This seems racy with some users who are lockless
>
> > +		mutex_lock(&ir->buffer_lock);
> > +		ir->p.set_use_dec(ir->p.data);
> > +		module_put(ir->p.owner);
> > +		mutex_unlock(&ir->buffer_lock);
> > +	} else
> > +		cleanup(ir);
> > +	mutex_unlock(&plugin_lock);
> > +
> > +/*
> > + * Recent kernels should handle this autmatically by
> > increasing/decreasing + * use count when a dependant module is
> > loaded/unloaded.
> > + */
>
> All kernels are recent now.

removed

> > +#ifdef MODULE
> > +
> > +/*
> > + *
> > + */
> > +int init_module(void)
> > +{
> > +	return lirc_dev_init();
> > +}
> > +
> > +/*
> > + *
> > + */
> > +void cleanup_module(void)
> > +{
> > +	/* unregister_chrdev returns void now */
> > +	unregister_chrdev(IRCTL_DEV_MAJOR, IRCTL_DEV_NAME);
> > +	class_destroy(lirc_class);
> > +	dprintk("lirc_dev: module unloaded\n");
> > +}
> > +
> > +MODULE_DESCRIPTION("LIRC base driver module");
> > +MODULE_AUTHOR("Artur Lipowski");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS_CHARDEV_MAJOR(IRCTL_DEV_MAJOR);
> > +
> > +module_param(debug, bool, 0644);
> > +MODULE_PARM_DESC(debug, "Enable debugging messages");
> > +
> > +#else /* not a MODULE */
> > +subsys_initcall(lirc_dev_init);
> > +
> > +#endif /* MODULE */
>
> Always use the #ifdef MODULE path, it does DTRT when compiled in too.
> Remove the init_module wrapper, replace with module_init()

done

thanks for the review. My changes are until Jarod pulls them in 
following tree git://git.jannau.net/linux-2.6-lirc.git

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/