From: Elias Vanderstuyft <hidden> Date: 2014-02-19 11:42:26
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
(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...).
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.
--
Anssi Hannula
From: Elias Vanderstuyft <hidden> Date: 2014-02-19 17:32:55
On Wed, Feb 19, 2014 at 5:49 PM, Anssi Hannula [off-list ref] wrote:
(added Dmitry to CC)
19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
quoted
Hi,
Hi,
quoted
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.
quoted
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.
quoted
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.
From: Andrew Eikum <hidden> Date: 2014-02-19 19:32:15
On Wed, Feb 19, 2014 at 06:32:54PM +0100, Elias Vanderstuyft wrote:
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.
Patches ready to be included in Wine should go to
wine-patches@winehq.org. I've done a bit of work in dinput myself, so
you can feel free to send them to me and/or wine-devel first if you'd
like a review.
Andrew
From: Elias Vanderstuyft <hidden> Date: 2014-02-19 19:49:50
On Wed, Feb 19, 2014 at 8:32 PM, Andrew Eikum [off-list ref] wrote:
On Wed, Feb 19, 2014 at 06:32:54PM +0100, Elias Vanderstuyft wrote:
quoted
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.
Patches ready to be included in Wine should go to
wine-patches@winehq.org. I've done a bit of work in dinput myself, so
you can feel free to send them to me and/or wine-devel first if you'd
like a review.
<offtopic>
Alright, thanks Andrew!
I've also done some work on reverse engineering the FFE file format,
which is needed to complete some missing functionality
(http://wiki.winehq.org/ForceFeedbackSummerOfCode2005Summary) of the
following functions:
IDirectInputDevice8::WriteEffectToFile and
IDirectInputDevice8::EnumEffectsInFile.
Can I send it to you (in a new mail subject, of course) and cc wine-devel?
</offtopic>
On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
(added Dmitry to CC)
19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
quoted
Hi,
Hi,
quoted
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?
Yeah, it looks like we need to change evdev.c to read:
error = input_ff_upload(dev, &effect, file);
if (error)
return error;
if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
return -EFAULT;
return 0;
Unfortunately applications should still expect changed effect ID for
quite some time.
Thanks.
--
Dmitry
From: Elias Vanderstuyft <hidden> Date: 2014-02-19 22:14:19
On Wed, Feb 19, 2014 at 10:05 PM, Dmitry Torokhov
[off-list ref] wrote:
On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
quoted
(added Dmitry to CC)
19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
quoted
Hi,
Hi,
quoted
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?
Yeah, it looks like we need to change evdev.c to read:
error = input_ff_upload(dev, &effect, file);
if (error)
return error;
if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
return -EFAULT;
return 0;
Alright, who will create the patch?
Do I may / have to do it?
Unfortunately applications should still expect changed effect ID for
quite some time.
Thanks.
--
Dmitry
On Wed, Feb 19, 2014 at 11:14:18PM +0100, Elias Vanderstuyft wrote:
On Wed, Feb 19, 2014 at 10:05 PM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
quoted
(added Dmitry to CC)
19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
quoted
Hi,
Hi,
quoted
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?
Yeah, it looks like we need to change evdev.c to read:
error = input_ff_upload(dev, &effect, file);
if (error)
return error;
if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
return -EFAULT;
return 0;
Alright, who will create the patch?
Do I may / have to do it?
If you could create the patch that would be appreciated.
Thanks.
--
Dmitry
On Wed, Feb 19, 2014 at 11:14:18PM +0100, Elias Vanderstuyft wrote:
quoted
On Wed, Feb 19, 2014 at 10:05 PM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
quoted
(added Dmitry to CC)
19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
quoted
Hi,
Hi,
quoted
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?
Yeah, it looks like we need to change evdev.c to read:
error = input_ff_upload(dev, &effect, file);
if (error)
return error;
if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
return -EFAULT;
return 0;
Alright, who will create the patch?
Do I may / have to do it?
If you could create the patch that would be appreciated.
Thanks.
--
Dmitry