From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751284AbWFESw7 (ORCPT ); Mon, 5 Jun 2006 14:52:59 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751287AbWFESw7 (ORCPT ); Mon, 5 Jun 2006 14:52:59 -0400 Received: from wr-out-0506.google.com ([64.233.184.231]:17083 "EHLO wr-out-0506.google.com") by vger.kernel.org with ESMTP id S1751284AbWFESw6 (ORCPT ); Mon, 5 Jun 2006 14:52:58 -0400 DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=beta; d=gmail.com; h=received:message-id:date:from:reply-to:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=V5WStlqZcf7DGrTaVV8t17lDi1ZuPP0nIoEg7mEX2YArb01mBwOwU+KxfM39Ewb1+0fcPbFdHRQsDgT4tUaHafq4J5oZs1y9L7UuxgFrK9P2WoFn+r3Mk1ONLuKP3Pus+vvXVs/fM6U5FvOWsVVKljz6n+HVbKx2DRQ7F0vgvRc= Message-ID: Date: Mon, 5 Jun 2006 14:52:57 -0400 From: "Dmitry Torokhov" Reply-To: dtor_core@ameritech.net To: "Anssi Hannula" Subject: Re: [patch 03/12] input: new force feedback interface Cc: linux-joystick@atrey.karlin.mff.cuni.cz, linux-kernel@vger.kernel.org, "Andrew Morton" In-Reply-To: <20060530110131.136225000@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20060530105705.157014000@gmail.com> <20060530110131.136225000@gmail.com> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org On 5/30/06, Anssi Hannula wrote: > Implement a new force feedback interface, in which all non-driver-specific > operations are separated to a common module. This includes handling effect > type validations, effect timers, locking, etc. > Still looking at it, couple of random points for now... > > The code should be built as part of the input module, but unfortunately that > would require renaming input.c, which we don't want to do. So instead we make > INPUT_FF_EFFECTS a bool so that it cannot be built as a module. > I am not opposed to rename input.c, I wonder what pending changes besides David's header cleanup Andrew had in mind. > @@ -865,6 +865,9 @@ struct input_dev { > unsigned long sndbit[NBITS(SND_MAX)]; > unsigned long ffbit[NBITS(FF_MAX)]; > unsigned long swbit[NBITS(SW_MAX)]; > + > + struct ff_device *ff; > + struct mutex ff_lock; I believe that ff_lock should be part of ff_device and be only used to controll access when uploading/erasing effects. The teardown process should make sure that device inactive anyway only then remove ff_device from input_dev; by that time noone should be able to upload/erase effects. Therefore ff_lock is not needed to protect dev->ff. > =================================================================== > --- linux-2.6.17-rc4-git12.orig/drivers/input/input.c 2006-05-27 02:28:57.000000000 +0300 > +++ linux-2.6.17-rc4-git12/drivers/input/input.c 2006-05-27 02:38:35.000000000 +0300 > @@ -733,6 +733,17 @@ static void input_dev_release(struct cla > { > struct input_dev *dev = to_input_dev(class_dev); > > + if (dev->ff) { > + struct ff_device *ff = dev->ff; > + clear_bit(EV_FF, dev->evbit); > + mutex_lock(&dev->ff_lock); > + del_timer_sync(&ff->timer); This is too late. We need to stop timer when device gets unregistered. Clearing FF bits is pointless here as device is about to disappear; locking is also not needed because we are guaranteed to be the last user of the device structure. I wonder if ff should be released right at unregister time... > + dev->flush = NULL; > + dev->ff = NULL; > + mutex_unlock(&dev->ff_lock); > + kfree(ff); > + } > + > kfree(dev); > module_put(THIS_MODULE); > } > +static inline int input_ff_safe_lock(struct input_dev *dev) > +{ > + mutex_lock(&dev->ff_lock); > + if (dev->ff) > + return 0; > + > + mutex_unlock(&dev->ff_lock); > + return 1; > +} This needs to go away. Users should check whether a device supports FF and if it is then it is device's responsibility to keep it there until untregister time. We don't expect FF capabilities to flip/flop on a live device. > +static void input_ff_calc_timer(struct ff_device *ff) > +{ > + int i; > + int events = 0; > + unsigned long next_time = 0; ... > + > + if (time_after(jiffies, event_time)) { > + event_time = jiffies; Should it be next_time = jiffies? We want to schedule thetimer ASAP, right? > + > +/** > + * abs() with -0x8000 => 0x7fff exception > + */ > +static inline u16 input_ff_unsign(s16 value) > +{ > + if (value == -0x8000) > + return 0x7fff; > + > + return (value < 0 ? -value : value); > +} Why is it needed? > + > +/** > + * Safe sum > + * @a: Integer to sum > + * @b: Integer to sum > + * @limit: The sum limit > + * > + * If @a+@b is above @limit, return @limit > + */ > +static int input_ff_safe_sum(int a, int b, int limit) > +{ > + int c; > + if (!a) > + return b; > + c = a + b; > + if (c > limit) > + return limit; > + return c; > +} As it was mentioned the result will not be limited if a == 0. Is it intended? PLease don;t start making any changes yet, I am still looking... -- Dmitry