Messages in this thread |  | | Date | Mon, 22 May 2006 19:12:38 +0300 | | From | Anssi Hannula <> | | Subject | Re: + input-new-force-feedback-interface.patch added to -mm tree |
| |
Arjan van de Ven wrote: > On Wed, 2006-05-17 at 21:46 -0700, akpm@osdl.org wrote: > >>+ >>+#ifdef DEBUG >>+#define debug(format, arg...) printk(KERN_DEBUG "ff-effects: " format "\n" , ## arg) >>+#else >>+#define debug(format, arg...) do {} while (0) >>+#endif > > > please just use the existing prdebug() thing for this, no need to invent > your own ;)
Couldn't find any info on that one, are you sure you spelled it correctly?
> >>+ >>+EXPORT_SYMBOL(input_ff_allocate); >>+EXPORT_SYMBOL(input_ff_register); >>+EXPORT_SYMBOL(input_ff_upload); >>+EXPORT_SYMBOL(input_ff_erase); > > > should these be _GPL exports? >
Well, I don't know. When should EXPORT_SYMBOLs be EXPORT_SYMBOL_GPLs?
> >>+ >>+#define spin_ff_cond_lock(_ff, _flags) \ >>+ do { \ >>+ if (!_ff->driver->playback) \ >>+ spin_lock_irqsave(&_ff->atomiclock, _flags); \ >>+ } while (0); >>+ >>+#define spin_ff_cond_unlock(_ff, _flags) \ >>+ do { \ >>+ if (!_ff->driver->playback) \ >>+ spin_unlock_irqrestore(&_ff->atomiclock, _flags); \ >>+ } while (0); > > > > hmmm conditional locking like this always makes me very nervous... what > is preventing ->playback from changing halfway a locked sequence?
Well, it's never changed, locked or not. But maybe we can get rid of this condlocking alltogether, see my reply for Andrew Morton.
> >>+ if (!events) { >>+ debug("no actions"); >>+ del_timer(&ff->timer); > > > are you really sure you don't need del_timer_sync() here? >
Yes, this function is also called from inside the timer in question and del_timer_sync() would block.
> > >>+static void input_ff_timer(unsigned long timer_data) >>+{ >>+ struct input_dev *dev = (struct input_dev *) timer_data; >>+ struct ff_device *ff = dev->ff; >>+ struct ff_effect effect; >>+ int i; >>+ unsigned long flags; >>+ int effects_pending; >>+ unsigned long effect_handled[NBITS(FF_EFFECTS_MAX)]; > > > > hmmm stack space? >
I count 76 bytes (x86), is that too much?
>>+ } else { >>+ ret = -ENOSYS; > > > that is almost always a wrong return value >
It's returned when the device is mem-capable but driver doesn't implement set_gain() but sets FF_GAIN or when driver doesn't implement set_autocenter() but sets FF_AUTOCENTER. But yes, if that happens, it's a driver bug, so maybe this is not correct use for -ENOSYS. Probably there should be BUG() too here.
>>+ if (test_bit(FF_CONSTANT, dev->ff->flags)) >>+ set_bit(FF_CONSTANT, dev->ffbit); >>+ if (test_bit(FF_SPRING, dev->ff->flags)) >>+ set_bit(FF_SPRING, dev->ffbit); >>+ if (test_bit(FF_FRICTION, dev->ff->flags)) >>+ set_bit(FF_FRICTION, dev->ffbit); >>+ if (test_bit(FF_DAMPER, dev->ff->flags)) >>+ set_bit(FF_DAMPER, dev->ffbit); >>+ if (test_bit(FF_INERTIA, dev->ff->flags)) > > > are you really sure you need atomic set_bit()'s here?? > if so.. I think you have a race ;)
Well, I'm not. Is there an alternative?
-- Anssi Hannula
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |