ff-core effect id handling in case of a failed effect upload

[Elias Vanderstuyft]

On Wed, Feb 19, 2014 at 5:49 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
> (added Dmitry to CC)
>
> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
>> Hi,
>>
>
> Hi,
>
>> In the process of reviewing the Wine DInput translation layer, I
>> noticed an inconvenience (in the ff-core implementation?) that can
>> possibly lead to confusing problems to application developers (not
>> only for Wine), in short:
>> If a new (id==-1) effect was uploaded (look at
>> ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
>> ff-core will have assigned a positive number to the effect id. This
>> can be confusing because the dev->ff->effects[] array will not contain
>> an element at the index of that new effect id.
>
> I agree that this is a bit confusing, and the kernel code should
> probably be modified to not clobber the ioctl-provided effect on failure
> (effect->id is set to an "undefined" value, i.e. next free effect slot).
>
> Dmitry, WDYT?
>
> The downside is that if changed, any new userspace code may unknowingly
> depend on the fixed non-clobbering behavior (though apparently they
> already do, as evidenced by Wine DInput, so might not be much of an
> argument...).

I don't think that's a problem.
Something that can be more problematic, is that a newer application
assumes the fixed non-clobbering behaviour, but runs on a kernel with
the old clobbering behaviour, could experience problems.

>
>> Here is a more elaborated description/discussion:
>> - This is a bug that is either the responsibility of the Linux kernel,
>> or of Wine (and possibly other applications that do the same thing as
>> described below):
>>     It is caused by the following situation:
>>         When uploading an effect, the specific kernel device driver
>> may return an error,
>>         e.g. EINVAL for example when a periodic's effect period is set to zero.
>>         This error will then be returned by "ioctl(*(This->fd),
>> EVIOCSFF, &This->effect)".
>>         With or without error, one can find out that
>> /drivers/input/ff-core.c:input_ff_upload(...) is called,
>>         which will set effect->id >= 0, if it was set to -1 (=> new
>> effect created) by the user.
>>
>>         But in case of an error:
>>         - Assume effect->id was set to -1 by the user:
>>             The error is reported by ff->upload(...) at
>> /drivers/input/ff-core.c:167,
>>             the effect->id will also be set >= 0 (*).
>>             The offending effect will not be saved in the
>> ff->effects[] array (***).
>>         - Assume effect->id was set >= 0 by the user (and
>> ff->effects[effect->id] is a valid existing effect):
>>             The error is reported by ff->upload(...) at
>> /drivers/input/ff-core.c:167,
>>             the effect->id will remain unchanged (**).
>>             The offending effect will not overwrite the
>> ff->effects[effect->id] element (****).
>>
>>         Is this (see *, **, *** and ****) desired behaviour?
>>         - If yes:
>>             Change the following in Wine's dinput/effect_linuxinput.c:90 :
>>                 if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>>             to :
>>                 int effectId_old = This->effect.id;
>>                 if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>>                     This->effect.id = effectId_old;
>>         - If no for *:
>>             Kernel code /drivers/input/ff-core.c:input_ff_upload(...)
>> has to be patched to revert "effect->id" back to its original value
>> set by the user,
>>             which is only needed when the initial (by user) value of
>> "effect->id" was equal to -1.
>>         - If no for **** (or maybe also ***):
>>             ff->effects[effect->id] could be replaced by an 'empty'
>> effect (however this can get complex because the effect's type has to
>> remain unchanged)
>>             This would be a change in the kernel code
>> /drivers/input/ff-core.c:input_ff_upload(...).
>>         - If no for **:
>>             I don't really know. Discussion is needed.
>>
>>         - In my opinion, **, *** and **** are desired behaviour, while
>> * should leave the effect->id at -1.
>
> Right.
>
>>             In that case, Wine's dinput implementation does not have
>> to be patched, and the kernel code only should apply a minor patch.
>
> Well, maybe better to change dinput anyway so that it can work properly
> with current kernel versions (assuming this gets changed in the future)?
> Not my call, though.

Yes, agreed.
I'll include the code (with effectId_old variable) and this discussion
when I submit my whole patchset for Wine's DInput libs to Wine-devel
mailing list. Right now, I don't know who I should personally mail to.

>
> --
> Anssi Hannula


Elias