lkml.org 
[lkml]   [2008]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 01/18] lirc core device driver infrastructure
Date
On Tuesday 09 September 2008 11:46:48 Andi Kleen wrote:
> Jarod Wilson <jwilson@redhat.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


\
 
 \ /
  Last update: 2008-09-09 13:37    [W:0.488 / U:2.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site