Message-Id: <200707282350.09207.dtor@insightbb.com>
Date:	Sat, 28 Jul 2007 23:50:08 -0400
From:	Dmitry Torokhov <dtor@...ightbb.com>
To:	Indan Zupancic <indan@....nu>
Cc:	linux-input@...ey.karlin.mff.cuni.cz, linux-kernel@...r.kernel.org
Subject: Re: [RFC/RFT 1/5] Input: implement proper locking in input core

Hi Indan,

On Friday 27 July 2007 19:28, Indan Zupancic wrote:
> Hi,
> 
> Not real feedback, just some nitpicks.
> 
> On Tue, July 24, 2007 06:45, Dmitry Torokhov wrote:
> > +static int input_defuzz_abs_event(int value, int old_val, int fuzz)
> > +{
> > +	if (fuzz) {
> > +		if (value > old_val - fuzz / 2 && value < old_val + fuzz / 2)
> > +			return value;
> >
> > -	add_input_randomness(type, code, value);
> > +		if (value > old_val - fuzz && value < old_val + fuzz)
> > +			return (old_val * 3 + value) / 4;
> >
> > -	switch (type) {
> > +		if (value > old_val - fuzz * 2 && value < old_val + fuzz * 2)
> > +			return (old_val + value) / 2;
> > +	}
> 
> Shouldn't the return values of the second and third case be reversed?
> In the 2nd check the new values is weighted for 1/4, while in the 3rd
> case it counts for 1/2, which breaks the "account new value more when
> it is closer to the old one" logic that I thought I saw here. So to sum up,
> should the second return be "return (old_val + value * 3) / 4"?

Thank you for bringing this up. Actually the 1st return valus should be
"old_val", not value. The logic is to "gravitate towards old" when
difference is small.

> 
> 
> > +/*
> > + * Generate software autorepeat event. Note that we take
> > + * dev->event_lock here to avoid racing with input_event
> > + * which may cause keys get "stuck".
> > + */
> 
> Hurray. :-)
> 
> > -			if (code > SW_MAX || !test_bit(code, dev->swbit) || !!test_bit(code, dev->sw) == value)
> > -				return;
> > +		if (dev->rep[REP_PERIOD])
> > +			mod_timer(&dev->timer, jiffies +
> > +					msecs_to_jiffies(dev->rep[REP_PERIOD]));
> > +	}
> 
> Perhaps use a local var for the "msecs_to_jiffies(dev->rep[REP_PERIOD])" part.
>

What would be the benefit of doing so?

> 
> > +static void input_start_autorepeat(struct input_dev *dev, int code)
> > +{
> > +	if (test_bit(EV_REP, dev->evbit) &&
> > +	    dev->rep[REP_PERIOD] && dev->rep[REP_DELAY] &&
> > +	    dev->timer.data) {
> > +		dev->repeat_key = code;
> > +		mod_timer(&dev->timer,
> > +			  jiffies + msecs_to_jiffies(dev->rep[REP_DELAY]));
> > +	}
> > +}
> 
> Same here.
> 
> 
> > +	case EV_KEY:
> > +		if (is_event_supported(code, dev->keybit, KEY_MAX) &&
> > +		    !!test_bit(code, dev->key) != value) {
> 
> A bit confusing, test_bit(0 only returns 0 or 1 anyway, doesn't it?
> So "test_bit(code, dev->key) != value" should be all right.
> I noticed that the old code did it too, but still.

Is it guaranteed? I only expect it to return 0/non-0 values, not necessarily
0 and 1.

> 
> > -		case EV_MSC:
> > +	case EV_SW:
> > +		if (is_event_supported(code, dev->swbit, SW_MAX) &&
> > +		    !!test_bit(code, dev->sw) != value) {
> 
> Same.
> 
> > -			break;
> > +	case EV_LED:
> > +		if (is_event_supported(code, dev->ledbit, LED_MAX) &&
> > +		    !!test_bit(code, dev->led) != value) {
> 
> And here.
> 
> 
> > +void input_inject_event(struct input_handle *handle,
> > +			unsigned int type, unsigned int code, int value)
> >  {
> > -	struct input_dev *dev = (void *) data;
> > +	struct input_dev *dev = handle->dev;
> > +	struct input_handle *grab;
> >
> > -	if (!test_bit(dev->repeat_key, dev->key))
> > -		return;
> > +	if (is_event_supported(type, dev->evbit, EV_MAX)) {
> > +		spin_lock_irq(&dev->event_lock);
> >
> > -	input_event(dev, EV_KEY, dev->repeat_key, 2);
> > -	input_sync(dev);
> > +		grab = rcu_dereference(dev->grab);
> > +		if (!grab || grab == handle)
> > +			input_handle_event(dev, type, code, value);
> 
> 'handle' can't be NULL, so can drop the "!grab" check, as checking
> "grab == handle" should be sufficient.
>

It is "or", not "and". The idea is to pass the event if device is not
grabbed by anyone _or_ if source of event is handle that grabbed the
device.
 
> 
> > +/**
> > + * input_open_device - open input device
> > + * @handle: handle through which device is being accessed
> > + *
> > + * This function should be called by input handlers when they
> > + * want to start receive events from given input device.
> > + */
> >  int input_open_device(struct input_handle *handle)
> >  {
> >  	struct input_dev *dev = handle->dev;
> > -	int err;
> > +	int retval;
> >
> > -	err = mutex_lock_interruptible(&dev->mutex);
> > -	if (err)
> > -		return err;
> > +	retval = mutex_lock_interruptible(&dev->mutex);
> > +	if (retval)
> > +		return retval;
> > +
> > +	if (dev->going_away) {
> > +		retval = -ENODEV;
> > +		goto out;
> > +	}
> >
> >  	handle->open++;
> >
> >  	if (!dev->users++ && dev->open)
> 
> Ugh, not your code, and perhaps it's me, but that looks weird.
> The ++ hidden inthe if check is ugly, and would mean that "users"
> can be negative, which is strange.
>

Why would it mean that?
 
> > -		err = dev->open(dev);
> > +		retval = dev->open(dev);
> >
> > -	if (err)
> > -		handle->open--;
> > +	if (retval && !--handle->open) {
> 
> Eek! That -- is hidden well there. Would it hurt to call synchronize_sched()
> unconditionally? Something like:
> 
> 	if (retval) {
> 		handle->open--;
> 
> It's a rare case anyway.
> 

Because it would not be needed and the follwing comment would be false.

> > +		/*
> > +		 * Make sure we are not delivering any more events
> > +		 * through this handle
> > +		 */
> > +		synchronize_sched();
> > +	}
> >
> 
> > +/**
> > + * input_close_device - close input device
> > + * @handle: handle through which device is being accessed
> > + *
> > + * This function should be called by input handlers when they
> > + * want to stop receive events from given input device.
> > + */
> >  void input_close_device(struct input_handle *handle)
> >  {
> >  	struct input_dev *dev = handle->dev;
> >
> > -	input_release_device(handle);
> > -
> >  	mutex_lock(&dev->mutex);
> >
> > +	__input_release_device(handle);
> > +
> >  	if (!--dev->users && dev->close)
> >  		dev->close(dev);
> > -	handle->open--;
> > +
> > +	if (!--handle->open) {
> > +		/*
> > +		 * synchronize_sched() makes sure that input_pass_event()
> > +		 * completed and that no more input events are delivered
> > +		 * through this handle
> > +		 */
> > +		synchronize_sched();
> > +	}
> 
> Same here, though just leaving the original "handle->open--;" there and
> merely adding the if check would be better too I think. Or just get rid of
> the whole if thing.
>

No, we do not want to do synchronize_sched when there are more users.
 
> 
> >  static void input_seq_print_bitmap(struct seq_file *seq, const char *name,
> > @@ -569,7 +765,9 @@ static const struct file_operations inpu
> >
> >  static void *input_handlers_seq_start(struct seq_file *seq, loff_t *pos)
> >  {
> > -	/* acquire lock here ... Yes, we do need locking, I knowi, I know... */
> 
>  ;-)
> 
> > +	if (mutex_lock_interruptible(&input_mutex))
> > +		return NULL;
> > +
> >  	seq->private = (void *)(unsigned long)*pos;
> >  	return seq_list_start(&input_handler_list, *pos);
> >  }
> 
> Greetings,
> 
> Indan
> 
> 

-- 
Dmitry
-
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/

Powered by blists - more mailing lists