(added Dmitry to CC)

19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
> 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

> 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.


>             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.

Anssi Hannula
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to