Re: [patch 03/11] input: new force feedback interface

[Anssi Hannula <anssi@mandriva.org>]

Andrew Morton wrote:
> Anssi Hannula <anssi@mandriva.org> wrote:
> 
>> >>+int input_ff_erase(struct input_dev *dev, int id)
>> >>+{
>> >>+	struct ff_device *ff;
>> >>+	unsigned long flags = 0;
>> >>+	int ret;
>> >>+	if (!test_bit(EV_FF, dev->evbit))
>> >>+		return -EINVAL;
>> >>+	mutex_lock(&dev->ff_lock);
>> >>+	ff = dev->ff;
>> >>+	if (!ff) {
>> >>+		mutex_unlock(&dev->ff_lock);
>> >>+		return -ENODEV;
>> >>+	}
>> >>+	spin_ff_cond_lock(ff, flags);
>> >>+	ret = _input_ff_erase(dev, id, current->pid == 0);
>> >>+	spin_ff_cond_unlock(ff, flags);
>> >>+
>> >>+	mutex_unlock(&dev->ff_lock);
>> >>+	return ret;
>> >>+}
>> > 
>> > 
>> > Perhaps you meant `current->uid == 0' here.  There's no way in which pid
>> > 0 will call this code.
>>
>> Right, a silly mistake.
>>
>> > What's happening here anyway?  Why does this code need to know about pids?
>> > 
>> > Checking for uid==0 woud be a fishy thing to do as well.
>>
>> User ID 0 is allowed to delete effects of other users. Pids are used to
>> keep a track of what process owns what effects. This is the same
>> behaviour as before.
> 
> 
> Oh dear.
> 
> Whatever we do here should remain 100%-compatible with "before".  Which
> rather limits our options.

There are only _very_ few programs using FF on Linux ATM, and I don't
think any of them opens multiple fd:s to the same device and expects to
be able to delete effects created on the other fd. And that is the only
con of changing the behaviour.

Should the behaviour be changed so that the effect owners are file
descriptors, the effects of one fd would not be lost if the process
opens and closes another fd on the same device (currently all effects of
a process are deleted if the process closes any fd to the device).

>> There is a problem with this, though:
>> When a process closes any fd to this device, all pid-matching effects
>> are deleted whether the process has another fd using the device or not.
>>
>> One solution would probably be to add some handle parameter to
>> input_ff_upload() and input_ff_erase(), and then in
>> evdev_ioctl_handler() pass an id unique to this fd. Then effects would
>> be fd-specific, not pid-specific. I think the uid == 0 thing can also be
>> dropped... I don't think the root user needs ability to override user
>> effects (it can delete them anyway, just kill the user process owning
>> the effects).
>>
> 
> 
> Generally we use file descriptors (and driver-specific state at
> file.f_private) to manage things like that.  But I'd imagine that we
> couldn't retain the existing semantics with any such scheme.
> 
> A pragmatic approach would be to put a big fat comment in there explaining
> how it all works and leave it at that.

As I don't see this could break any existing applications, I would very
much like to change the behaviour so that the effects are file
descriptor specific. What should I use to differentiate the descriptors?
Can I just compare the "struct file*"? (it seems to work well, I just
modified the code so)

-- 
Anssi Hannula