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.
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.
Best regards,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
