Re: [PATCH RESEND 2] HID: Add force feedback support for Speedlink Cougar Vibration Flightstick

[Harald Judt <h.judt@gmx.at>]

Hi,

Am 09.05.26 um 01:14 schrieb Dmitry Torokhov:

[ ... ]

>>> +	strong = (strong / 0xff) * 0x1f / 0xff;
>>> +	weak = (weak / 0xff) * 0x1f / 0xff;
>>> +
>>> +	/* ... and to support the notions of strong vs weak rumble effects,
>>> +	 * increase the magnitude for the strong rumble effect if it is below the
>>> +	 * half of the maximum value, as the strong motor has the same strength as
>>> +	 * the weak one. Likewise, decrease the magnitude for the weak effect.
>>> +	 */
>>> +	if (strong < 0x10 && !weak)         /* fftest effect 4 strong rumble */
>>> +		strong *= 2;
>>> +	else if (!strong && weak >= 0x10)   /* fftest effect 5 weak rumble */
>>> +		weak /= 2;
>>
>> Will this cause issues with non-monotonic force feedback behavior and
>> unintended truncation?

I thought about this too. I have not experienced any negative behavior with
this solution, the stick seems to work fine with it. But honestly, there
are not many tests available. fftest and SuperTuxKart have been the only
applications I found supporting force feedback, it worked fine there,
but the stick is also a bit limited in capabilities, it does not really
support "force feedback", only some degrees of rumble.

Since I wrote this patch some time passed and I have gained a bit more
experience with force feedback in general and implementing support for it.

>> Since the math uses integer division, any input value below 255 for strong
>> or weak will evaluate to zero during the first division by 0xff.

I am aware of this but IIRC this does not have any practical impact for
this stick. It seems to correspond to the values I got from the SET_REPORTs
in Windows and the vibrating behaviour.

>> Additionally, could the conditional scaling cause abrupt changes in rumble
>> strength? For instance, if a game requests a strong magnitude that scales
>> down to 15, it gets doubled to 30 because of the strong < 0x10 check.
>> However, a slightly stronger requested magnitude that scales to 16 will
>> remain 16, meaning an increase in requested strength results in a drop in
>> physical rumble.

True. I have tested this only with fftest and observed the behaviour in
SuperTuxKart, where it worked fine, as it uses both strong and weak
attributes. Existing implementations for other devices in the kernel
source I found sometimes use a "left" motor and a "right" motor,
other times a "strong" motor and a "weak" motor. But the games do a
variation of strong vs weak. So I finally decided to somehow get it
to work with the game I could test it with. I will do more checks
with other parameters/gain and try to improve this or think about
another, better solution. I might probably need to write some
ff tests myself for this, but why not...
>> [ ... ]
>>
>>> +static int gembird_joy_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = hid_parse(hdev);
>>> +	if (ret) {
>>> +		hid_err(hdev, "parse failed\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
>>> +	if (ret) {
>>> +		hid_err(hdev, "hw start failed\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	gembird_joy_init(hdev);
>>> +
>>> +	return 0;
>>> +}
>>
>> What happens if gembird_joy_init fails?
>>
>> If input_ff_create_memless returns an error, gembird_joy_init
>> propagates it, but the return value is ignored here and probe succeeds.
>>
>> Because set_bit(FF_RUMBLE, dev->ffbit) was already called, the device
>> remains registered and advertises rumble support. If userspace then issues
>> an EVIOCSFF ioctl, the kernel will allow it, which could lead to a panic
>> when it tries to dereference the missing force feedback structures.
> 
> Need to have error handling.

I have looked at some other implementations, and if I have not missed
anything, then they do it similarly (hid-gaff.c, hid-sjoy.c, hid-emsff.c,
hid-megaworld.c). I will take a closer look again and try to fix it.

> Thanks.
Thank you for your suggestions and spending your time on this review.
I will try to improve it and send a new patch when ready.

Regards,
Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD