[RFC 2/11] Linux FF: Linux effect status management

[Elias Vanderstuyft]

a)  This is a bug that is either the responsibility of the Linux
kernel, or of Wine:
        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 behavior?
            - If yes:
                Change the following in Wine's dinput/effect_linuxinput.c:84 :
                    LinuxInputEffectImpl *This =
impl_from_IDirectInputEffect(iface);

                    TRACE("(this=%p)\n", This);

                    if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
                to :
                    LinuxInputEffectImpl *This =
impl_from_IDirectInputEffect(iface);
                    int effectId_old;

                    TRACE("(this=%p)\n", This);

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

    => This has been discussed (see
http://www.mail-archive.com/linux-input@vger.kernel.org/msg08513.html),
and the following is true:
        My opinion appeared to be correct, but for to be sure, I was
recommended to apply the change in Wine as well.


b)  Be more precise in returning errors.
    In dinput/effect_linuxinput.c:89, change :
        if (errno == ENOMEM) {
            return DIERR_DEVICEFULL;
        } else {
            FIXME("Could not upload effect. Assuming a disconnected
device %d \"%s\".\n", *This->fd, strerror(errno));
            return DIERR_INPUTLOST;
        }
    to :
        switch (errno) {
            case EINVAL:
                TRACE("Could not upload effect: Invalid argument.\n");
                return DIERR_INVALIDPARAM;
            case ENOSPC:
                TRACE("Could not upload effect: No space left on device.\n");
                return DIERR_DEVICEFULL;
            case ENOMEM:
                TRACE("Could not upload effect: Out of memory.\n");
                return DIERR_OUTOFMEMORY;
            default:
                FIXME("Could not upload effect. Assuming a
disconnected device %d \"%s\".\n", *This->fd, strerror(errno));
                return DIERR_INPUTLOST;
        }


c)  The following in dinput/effect_linuxinput.c:336 :
        if (res != DI_OK)
    should be probably :
        if (FAILED(res))
    for example if a device reports S_FALSE because it has already
updated an identical effect.

    The same for line 549:
        if (retval != DI_OK)
    should be then :
        if (FAILED(retval))


Elias
-------------- next part --------------
/////////////////////////////    Linux effect status management    /////////////////////////////

a)  This is a bug that is either the responsibility of the Linux kernel, or of Wine:
        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 behavior?
            - If yes:
                Change the following in Wine's dinput/effect_linuxinput.c:84 :
                    LinuxInputEffectImpl *This = impl_from_IDirectInputEffect(iface);

                    TRACE("(this=%p)\n", This);

                    if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
                to :
                    LinuxInputEffectImpl *This = impl_from_IDirectInputEffect(iface);
                    int effectId_old;

                    TRACE("(this=%p)\n", This);
                    
                    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 behavior, 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.
    
    => This has been discussed (see http://www.mail-archive.com/linux-input@vger.kernel.org/msg08513.html), and the following is true:
        My opinion appeared to be correct, but for to be sure, I was recommended to apply the change in Wine as well.


b)  Be more precise in returning errors.
    In dinput/effect_linuxinput.c:89, change :
        if (errno == ENOMEM) {
            return DIERR_DEVICEFULL;
        } else {
            FIXME("Could not upload effect. Assuming a disconnected device %d \"%s\".\n", *This->fd, strerror(errno));
            return DIERR_INPUTLOST;
        }
    to :
        switch (errno) {
            case EINVAL:
                TRACE("Could not upload effect: Invalid argument.\n");
                return DIERR_INVALIDPARAM;
            case ENOSPC:
                TRACE("Could not upload effect: No space left on device.\n");
                return DIERR_DEVICEFULL;
            case ENOMEM:
                TRACE("Could not upload effect: Out of memory.\n");
                return DIERR_OUTOFMEMORY;
            default:
                FIXME("Could not upload effect. Assuming a disconnected device %d \"%s\".\n", *This->fd, strerror(errno));
                return DIERR_INPUTLOST;
        }


c)  The following in dinput/effect_linuxinput.c:336 :
        if (res != DI_OK)
    should be probably :
        if (FAILED(res))
    for example if a device reports S_FALSE because it has already updated an identical effect.
    
    The same for line 549:
        if (retval != DI_OK)
    should be then :
        if (FAILED(retval))