Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Sun, 02 Oct 2022 14:34:45 +0300
From:   Anssi Hannula <anssi.hannula@iki.fi>
To:     Paul Dino Jones <paul@spacefreak18.xyz>
Cc:     jikos@kernel.org, linux-kernel@vger.kernel.org,
        linux-input@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH] usbhid: Interpret 0 length ff effects as infinite
 (0xffff) length effects
In-Reply-To: <20221001221657.gexisc2egjn3mpog@localhost.localdomain>
References: <20221001221657.gexisc2egjn3mpog@localhost.localdomain>
Message-ID: <93f708f3f9ac8b5c94e6d0b86c1efaa3@iki.fi>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Paul Dino Jones kirjoitti 2022-10-02 01:16:
> Greetings,

Hello, and thanks for looking into this!

> I started using my Accuforce V2 sim wheel on Linux. I was getting no
> response from racing simulators through wine, while native linux test
> tools worked properly. It appears that many real-world applications 
> will
> send 0 as the replay length, which was resulting in the behavior I was
> observing (nothing). The PID document does not explicitly state that 0
> length effects should be interpreted as infinite, but it does mention
> null effects being infinite effects.

Actually, it is Wine that is translating 0xFFFF from the application to 
0x0000 for the Linux FF API:
https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/winebus.sys/bus_udev.c#L1124

Unfortunately "infinite" duration is not actually specified at all in 
our API currently.
input.h just says that the all durations are in msecs and values above 
0x7fff cause unspecified results.

We have three places where the duration is handled:
- ff-memless: Considers 0 as infinite (in ml_get_combo_effect() and 
calculate_next_time()).
- iforce-ff: Just passes the duration to HW as-is - it is unknown what 
counts as infinite, if any.
- pidff: Just passes the duration to HW as-is, so using the 
unspecified-by-API 0xffff results in infinite duration (per USB HID PID 
spec).

So we probably want to specify some value to work as infinite, likely 
either 0 or 0xFFFF, and explicitly document that in input.h.
I suspect that ff-memless devices are currently the most popular, and 
e.g. Wine already assumes 0 is infinite, and I can't think of a reason 
to have an "actual" 0-duration effect, so I guess 0 would be the most 
sensible value.

Since iforce is an "ancestor" of HID PID of sorts, it may also support 
0xffff = infinite.
I'll try to get hold of one to test, though it may take a couple of 
weeks...


> This patch will interpret 0 length force feedback effects as 0xffff
> (infinite) length effects, leaving other values for replay length
> unchanged.
> 
> Signed-off-by: Paul Dino Jones <paul@spacefreak18.xyz>
> ---
>  drivers/hid/usbhid/hid-pidff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/usbhid/hid-pidff.c 
> b/drivers/hid/usbhid/hid-pidff.c
> index 3b4ee21cd811..70653451c860 100644
> --- a/drivers/hid/usbhid/hid-pidff.c
> +++ b/drivers/hid/usbhid/hid-pidff.c
> @@ -301,7 +301,7 @@ static void pidff_set_effect_report(struct
> pidff_device *pidff,
>  		pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0];
>  	pidff->set_effect_type->value[0] =
>  		pidff->create_new_effect_type->value[0];
> -	pidff->set_effect[PID_DURATION].value[0] = effect->replay.length;
> +	pidff->set_effect[PID_DURATION].value[0] = effect->replay.length ==
> 0 ? 0xffff : effect->replay.length;
>  	pidff->set_effect[PID_TRIGGER_BUTTON].value[0] = 
> effect->trigger.button;
>  	pidff->set_effect[PID_TRIGGER_REPEAT_INT].value[0] =
>  		effect->trigger.interval;

-- 
Anssi Hannula