hid-pidff bug: fails to find all required reports of saitek gamepad

hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

Hello!

After reading HID/PID specs I understood, that Saitek gamepads doesn't
require any driver -- they are PID compliant. Then, after loading
usbhid with debug messages enabled, I saw this:
[  773.716917] usbhid:hid-pidff: starting pid init
[  773.716947] usbhid:hid-pidff: found usage 0x21 from field->logical
[  773.716965] usbhid:hid-pidff: found usage 0x5a from field->logical
[  773.716983] usbhid:hid-pidff: found usage 0x73 from field->logical
[  773.717000] usbhid:hid-pidff: found usage 0x6e from field->logical
[  773.717018] usbhid:hid-pidff: found usage 0x77 from field->logical
[  773.717036] usbhid:hid-pidff: found usage 0x96 from field->logical
[  773.717075] usbhid:hid-pidff: found usage 0x7d from field->logical
[  773.717127] usbhid:hid-pidff: found usage 0x7f from field->logical
[  773.717155] usbhid:hid-pidff: 4 missing
[  773.717176] usbhid:hid-pidff: reports not ok, aborting

Definitely, HID descriptor parser has problems, which stops driver
from reading all reports. Here is hid descriptor of gamepad:
http://dmitriy.geels.googlepages.com/HIDDescriptor.html

I added some more debug messages:
http://dmitriy.geels.googlepages.com/debug.patch
Then got this:

[  773.716917] usbhid:hid-pidff: starting pid init
[  773.716937] usbhid:hid-pidff: checking field->logical == 0x000f0021
[  773.716947] usbhid:hid-pidff: found usage 0x21 from field->logical
[  773.716956] usbhid:hid-pidff: checking field->logical == 0x000f005a
[  773.716965] usbhid:hid-pidff: found usage 0x5a from field->logical
[  773.716974] usbhid:hid-pidff: checking field->logical == 0x000f0073
[  773.716983] usbhid:hid-pidff: found usage 0x73 from field->logical
[  773.716992] usbhid:hid-pidff: checking field->logical == 0x000f006e
[  773.717000] usbhid:hid-pidff: found usage 0x6e from field->logical
[  773.717009] usbhid:hid-pidff: checking field->logical == 0x000f0077
[  773.717018] usbhid:hid-pidff: found usage 0x77 from field->logical
[  773.717027] usbhid:hid-pidff: checking field->logical == 0x000f0096
[  773.717036] usbhid:hid-pidff: found usage 0x96 from field->logical
[  773.717045] usbhid:hid-pidff: checking field->logical == 0x00ff0302
[  773.717055] usbhid:hid-pidff: checking hid->collection[11].usage ==
0x00ff0302
[  773.717066] usbhid:hid-pidff: checking field->logical == 0x000f007d
[  773.717075] usbhid:hid-pidff: found usage 0x7d from field->logical
[  773.717084] usbhid:hid-pidff: checking field->logical == 0x00000000
[  773.717093] usbhid:hid-pidff: checking hid->collection[18].usage ==
0x00000000
[  773.717118] usbhid:hid-pidff: checking field->logical == 0x000f007f
[  773.717127] usbhid:hid-pidff: found usage 0x7f from field->logical
[  773.717136] usbhid:hid-pidff: checking field->logical == 0x000f0025
[  773.717145] usbhid:hid-pidff: checking field->logical == 0x000f008b
[  773.717155] usbhid:hid-pidff: 4 missing
[  773.717162] usbhid:hid-pidff: 5 missing
[  773.717169] usbhid:hid-pidff: 7 missing
[  773.717176] usbhid:hid-pidff: reports not ok, aborting

25, 8b? these are inside of required 0xab and 0x89 reports, and 0x90
is just ignored...

I don't understand yet, how to fix this bug, so I'm posting this to
maillist with hope on someones help.

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
> Hello!

Hi!

> After reading HID/PID specs I understood, that Saitek gamepads doesn't
> require any driver -- they are PID compliant. Then, after loading
> usbhid with debug messages enabled, I saw this:
> [  773.716917] usbhid:hid-pidff: starting pid init
> [  773.716947] usbhid:hid-pidff: found usage 0x21 from field->logical
> [  773.716965] usbhid:hid-pidff: found usage 0x5a from field->logical
> [  773.716983] usbhid:hid-pidff: found usage 0x73 from field->logical
> [  773.717000] usbhid:hid-pidff: found usage 0x6e from field->logical
> [  773.717018] usbhid:hid-pidff: found usage 0x77 from field->logical
> [  773.717036] usbhid:hid-pidff: found usage 0x96 from field->logical
> [  773.717075] usbhid:hid-pidff: found usage 0x7d from field->logical
> [  773.717127] usbhid:hid-pidff: found usage 0x7f from field->logical
> [  773.717155] usbhid:hid-pidff: 4 missing
> [  773.717176] usbhid:hid-pidff: reports not ok, aborting
> 
> Definitely, HID descriptor parser has problems, which stops driver
> from reading all reports. Here is hid descriptor of gamepad:
> http://dmitriy.geels.googlepages.com/HIDDescriptor.html
> 
> I added some more debug messages:
> http://dmitriy.geels.googlepages.com/debug.patch
> Then got this:
> 
[...]
> [  773.717155] usbhid:hid-pidff: 4 missing
> [  773.717162] usbhid:hid-pidff: 5 missing
> [  773.717169] usbhid:hid-pidff: 7 missing
> [  773.717176] usbhid:hid-pidff: reports not ok, aborting
> 
> 25, 8b? these are inside of required 0xab and 0x89 reports, and 0x90
> is just ignored...
> 
> I don't understand yet, how to fix this bug, so I'm posting this to
> maillist with hope on someones help.

The error message means that it can't find reports 0xab (create new
effect), 0x89 (pid block load), 0x90 (pid block free), that according to
your html page are in fact there.

Could you provide the output with parameter debug=1 for hid module when
device is initialized? It prints the raw HID descriptor and the kernel's
interpretation of it.
(If you have the hid module built-in, you can use "echo 1 >
/sys/modules/hid/parameters/debug")

-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

Hello!

2009/2/2 Anssi Hannula <anssi.hannula@gmail.com>:
>> I don't understand yet, how to fix this bug, so I'm posting this to
>> maillist with hope on someones help.
>
> The error message means that it can't find reports 0xab (create new
> effect), 0x89 (pid block load), 0x90 (pid block free), that according to
> your html page are in fact there.
>
> Could you provide the output with parameter debug=1 for hid module when
> device is initialized? It prints the raw HID descriptor and the kernel's
> interpretation of it.
> (If you have the hid module built-in, you can use "echo 1 >
> /sys/modules/hid/parameters/debug")

I'm discovering this situation, but haven't got much progress. Now I
consider, that problem is not in hid-pidff, but somewhere deeper, like
in hid_parse_report().

I saw strange things: report for usage 0x90 has empty field->logical
field (should be 0x000f0090, was seen as "checking field->logical ==
0x00000000" earlier), that's why it's ignored. Reports, which are 0x25
and 0x8b, are feature reports and I have no clue yet, why wrong usages
were put into field->logical.

Here is decoded hid descriptor with raw data:
http://docs.google.com/Doc?id=dhk68r8j_2gvttgsr8
Here is just raw data: http://docs.google.com/Doc?id=dhk68r8j_38stwrngm

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

And here is log of hid module with debug=1: http://paste.org/5168

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

Dmitriy Geels wrote:
> 2009/2/2 Anssi Hannula <anssi.hannula@gmail.com>:
>>> I don't understand yet, how to fix this bug, so I'm posting this to
>>> maillist with hope on someones help.
>> The error message means that it can't find reports 0xab (create new
>> effect), 0x89 (pid block load), 0x90 (pid block free), that according to
>> your html page are in fact there.
>>
>> Could you provide the output with parameter debug=1 for hid module when
>> device is initialized? It prints the raw HID descriptor and the kernel's
>> interpretation of it.
>> (If you have the hid module built-in, you can use "echo 1 >
>> /sys/modules/hid/parameters/debug")
> 
> I'm discovering this situation, but haven't got much progress. Now I
> consider, that problem is not in hid-pidff, but somewhere deeper, like
> in hid_parse_report().
> 
> I saw strange things: report for usage 0x90 has empty field->logical
> field (should be 0x000f0090, was seen as "checking field->logical ==
> 0x00000000" earlier), that's why it's ignored. Reports, which are 0x25
> and 0x8b, are feature reports and I have no clue yet, why wrong usages
> were put into field->logical.

It is because those collections are defined as "Report collections"
instead of "Logical collections". Linux doesn't know about "Report
collections", but according to HID spec it is also a Logical collection:

"Defines a logical collection that wraps all the fields in a report. A
unique report ID will be contained in this collection. An
application can easily determine whether a device supports a
certain function. Note that any valid Report ID value can be
declared for a Report collection."

Try the attached patch, which simply changes the collection type for
such collections.

> Here is decoded hid descriptor with raw data:
> http://docs.google.com/Doc?id=dhk68r8j_2gvttgsr8
> Here is just raw data: http://docs.google.com/Doc?id=dhk68r8j_38stwrngm

-- 
Anssi Hannula

[-- Attachment #2: hid-logical-subtypes.diff --]
[-- Type: text/plain, Size: 622 bytes --]

--- linux/drivers/hid/hid-core.c.old	2009-02-07 14:17:49.000000000 +0200
+++ linux/drivers/hid/hid-core.c	2009-02-07 14:22:45.000000000 +0200
@@ -142,6 +142,12 @@
 	parser->collection_stack[parser->collection_stack_ptr++] =
 		parser->device->maxcollection;
 
+	/* Report collections (0x03), Named Array collections (0x04) and Usage
+	 * Switch collections (0x05) are subtypes of logical collections
+	 * according to the HID specification */
+	if (type >= 0x03 && type <= 0x05)
+		type = HID_COLLECTION_LOGICAL;
+
 	collection = parser->device->collection +
 		parser->device->maxcollection++;
 	collection->type = type;

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

Ok, here is what I got (I added few debug messages):
http://paste.org.ru/index.pl?n9joxr

Strange, that it can't find usage 0x25 in create new effect report, it
is actually there. I'll check this.

And something has to be done with missing direction field...

Fwd: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

Thanks Anssi!

Your patch helped, device is passing pidff_find_reports().

Now stuck in pidff_find_fields(), with "unknown set_effect report layout".
Looks like, device doesn't implement PID specs completely, as there is
no gain and direction usages in set_effect report:
>The minimal Effect parameter block must contain (Effect) Parameter Block Index, Effect Type, Duration,
>Sample Period, Gain, Trigger Button, Trigger Repeat Interval, Axis Direction, and Type Specific Block
>Handle values.

But, looking into pidff_set_effect_report(), I think, it's possible to
add two if's to check if these fields present. And need to make check
for these fields optional somehow.
These two usages are not fully utilized anyway, gain is always set to
it's logical_maximum (actually it's used only in pidff_autocenter())
and direction is always set to 1.

There is also some problem with pidff_find_special_fields() ("effect
lists not found"), i'll add more debug messages and tell about that
later.

2009/2/7 Anssi Hannula <anssi.hannula@gmail.com>:
> Try the attached patch, which simply changes the collection type for
> such collections.

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
> Thanks Anssi!
> 
> Your patch helped, device is passing pidff_find_reports().
> 
> Now stuck in pidff_find_fields(), with "unknown set_effect report layout".
> Looks like, device doesn't implement PID specs completely, as there is
> no gain and direction usages in set_effect report:
>> The minimal Effect parameter block must contain (Effect) Parameter Block Index, Effect Type, Duration,
>> Sample Period, Gain, Trigger Button, Trigger Repeat Interval, Axis Direction, and Type Specific Block
>> Handle values.

There is also no Sample Period nor Effect Type values.

> But, looking into pidff_set_effect_report(), I think, it's possible to
> add two if's to check if these fields present. And need to make check
> for these fields optional somehow.
> These two usages are not fully utilized anyway, gain is always set to
> it's logical_maximum (actually it's used only in pidff_autocenter())
> and direction is always set to 1.

Direction enable is set to 1. However, the direction value itself is 
required for the force direction.

I really can't see how the direction could be transmitted, as the Axis 
Enable fields (alternative way of specifying direction) are missing as 
well. Unless you can figure it out with some creative testing, I guess 
we need a usb dump of constant force with a windows driver.

For Effect Type, that can also be made optional in set_effect_report() 
(it has already been sent in the upload request, so it is redundant).

> There is also some problem with pidff_find_special_fields() ("effect
> lists not found"), i'll add more debug messages and tell about that
> later.

See above, it is because of the missing Effect Type field.

-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/2/10 Anssi Hannula <anssi.hannula@gmail.com>:
>> But, looking into pidff_set_effect_report(), I think, it's possible to
>> add two if's to check if these fields present. And need to make check
>> for these fields optional somehow.
>> These two usages are not fully utilized anyway, gain is always set to
>> it's logical_maximum (actually it's used only in pidff_autocenter())
>> and direction is always set to 1.
>
> Direction enable is set to 1. However, the direction value itself is
> required for the force direction.
>
> I really can't see how the direction could be transmitted, as the Axis
> Enable fields (alternative way of specifying direction) are missing as well.
> Unless you can figure it out with some creative testing, I guess we need a
> usb dump of constant force with a windows driver.
I did that already, you can also see described constant effect report
using links, I posted earlier. There is no direction at all. Constant
report has only effect block index and magnitude (3 bytes total: id,
bi, mag). Device doesn't support direction at all.

> For Effect Type, that can also be made optional in set_effect_report() (it
> has already been sent in the upload request, so it is redundant).
>
>> There is also some problem with pidff_find_special_fields() ("effect
>> lists not found"), i'll add more debug messages and tell about that
>> later.
>
> See above, it is because of the missing Effect Type field.
I found that out already and made this ugly fix:
http://paste.org.ru/index.pl?oer4rl
- make pidff->set_effect_type optional
- make pidff->effect_direction optional
- change call to PIDFF_FIND_FIELDS(set_effect...) to non-strict (this
is bad, I know)
After that, driver almost initializes: http://paste.org.ru/index.pl?008sgm
It fails in pidff_check_autocenter(). According to descriptor, effect
1 is constant force. The problem is that block load report receive
fails.
I have no idea yet, why it fails, need to do some debug. Can you tell,
is there some way to monitor reports in kernel?

And, I think, the main question: Anssi, how do you think, what would
be better way to implement support for Saitek devices: make separate
driver for them (which will be copy if pidff or will contain hardcoded
report info) or make more relaxed specification specification support
(like I did in this fix)?

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
> 2009/2/10 Anssi Hannula <anssi.hannula@gmail.com>:
>>> But, looking into pidff_set_effect_report(), I think, it's possible to
>>> add two if's to check if these fields present. And need to make check
>>> for these fields optional somehow.
>>> These two usages are not fully utilized anyway, gain is always set to
>>> it's logical_maximum (actually it's used only in pidff_autocenter())
>>> and direction is always set to 1.
>> Direction enable is set to 1. However, the direction value itself is
>> required for the force direction.
>>
>> I really can't see how the direction could be transmitted, as the Axis
>> Enable fields (alternative way of specifying direction) are missing as well.
>> Unless you can figure it out with some creative testing, I guess we need a
>> usb dump of constant force with a windows driver.
> I did that already, you can also see described constant effect report
> using links, I posted earlier. There is no direction at all. Constant
> report has only effect block index and magnitude (3 bytes total: id,
> bi, mag). Device doesn't support direction at all.

(for clarification, I meant here an actual usb traffic dump when you 
play an effect in windows)

Constant force makes no sense without a direction. What kind of force 
effect does it actually produce?

>> For Effect Type, that can also be made optional in set_effect_report() (it
>> has already been sent in the upload request, so it is redundant).
>>
>>> There is also some problem with pidff_find_special_fields() ("effect
>>> lists not found"), i'll add more debug messages and tell about that
>>> later.
>> See above, it is because of the missing Effect Type field.
> I found that out already and made this ugly fix:
> http://paste.org.ru/index.pl?oer4rl
> - make pidff->set_effect_type optional
> - make pidff->effect_direction optional
> - change call to PIDFF_FIND_FIELDS(set_effect...) to non-strict (this
> is bad, I know)
> After that, driver almost initializes: http://paste.org.ru/index.pl?008sgm
> It fails in pidff_check_autocenter(). According to descriptor, effect
> 1 is constant force. The problem is that block load report receive
> fails.
> I have no idea yet, why it fails, need to do some debug.

Well, actually pidff_check_autocenter should check for support of Spring 
effect, and skip tests if it is not supported (your device doesn't 
support it, which suggests that the autocenter is managed in a different 
way; windows dump would help finding this out as well).

However, pidff_request_effect_upload should still not fail, as it is 
needed for uploading effects. Try changing the effect type, make it e.g.
error = pidff_request_effect_upload(pidff, 2);

You could also try adding some delays after usbhid_wait_io() calls in 
pidff_request_effect_upload(), with e.g. msleep(200) (you should also 
lower the iterations limit 60 when adding such delays).

If nothing else helps, I guess we need a dump from windows for this as well.

> Can you tell,
> is there some way to monitor reports in kernel?

You can set debug=2 for hid module, but it will produce very much output.

> And, I think, the main question: Anssi, how do you think, what would
> be better way to implement support for Saitek devices: make separate
> driver for them (which will be copy if pidff or will contain hardcoded
> report info) or make more relaxed specification specification support
> (like I did in this fix)?

I prefer the relaxed support.

FYI, I may be unavailable until next Tuesday.

-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/2/11 Anssi Hannula <anssi.hannula@gmail.com>:
> Dmitriy Geels wrote:
>> I did that already, you can also see described constant effect report
>> using links, I posted earlier. There is no direction at all. Constant
>> report has only effect block index and magnitude (3 bytes total: id,
>> bi, mag). Device doesn't support direction at all.
>
> (for clarification, I meant here an actual usb traffic dump when you play an
> effect in windows)
Ok, here is usb sniffer report:
http://dmitriy.geels.googlepages.com/HIDtrafficdump.html
Only reports data is raw, everything else decoded.
What is there: I opened control panel applet for game devices, there
is a test page, each effect played after button was pressed. All
effects are periodic. There is pretty much information there.
Here is just constant effect dump:
http://dmitriy.geels.googlepages.com/constanteffectdump.html
I changed magnitude and direction in test program. Changing direction
doesn't send anything to device.

> Constant force makes no sense without a direction. What kind of force effect
> does it actually produce?
Just rumble. There is only one motor inside. For constant effect motor
spins with constant speed, as I understood, this speed is controlled
by magnitude. Also it is a place for report fixup: magnitude logical
values are -127/127, but actually 1/255 -- 255 is strongest.

>> After that, driver almost initializes: http://paste.org.ru/index.pl?008sgm
>> It fails in pidff_check_autocenter(). According to descriptor, effect
>> 1 is constant force. The problem is that block load report receive
>> fails.
>> I have no idea yet, why it fails, need to do some debug.
...
> However, pidff_request_effect_upload should still not fail, as it is needed
> for uploading effects. Try changing the effect type, make it e.g.
> error = pidff_request_effect_upload(pidff, 2);
Actually I tried to comment out check_autocenter() - then driver
initializes successfully. But fftest fails to upload effect with io
error.

> You could also try adding some delays after usbhid_wait_io() calls in
> pidff_request_effect_upload(), with e.g. msleep(200) (you should also lower
> the iterations limit 60 when adding such delays).
I tried this, no result.
I think, this problem is connected somehow to log message about
maxusage and report_count do not match.

> If nothing else helps, I guess we need a dump from windows for this as well.
>
>> Can you tell,
>> is there some way to monitor reports in kernel?
>
> You can set debug=2 for hid module, but it will produce very much output.
I found where to get outgoing data, added 2 dbg_hid() loggers, now
have to find right place for incoming reports.

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
> 2009/2/11 Anssi Hannula <anssi.hannula@gmail.com>:
>> Dmitriy Geels wrote:
>>> I did that already, you can also see described constant effect report
>>> using links, I posted earlier. There is no direction at all. Constant
>>> report has only effect block index and magnitude (3 bytes total: id,
>>> bi, mag). Device doesn't support direction at all.
>> (for clarification, I meant here an actual usb traffic dump when you play an
>> effect in windows)
> Ok, here is usb sniffer report:
> http://dmitriy.geels.googlepages.com/HIDtrafficdump.html
> Only reports data is raw, everything else decoded.
> What is there: I opened control panel applet for game devices, there
> is a test page, each effect played after button was pressed. All
> effects are periodic. There is pretty much information there.
> Here is just constant effect dump:
> http://dmitriy.geels.googlepages.com/constanteffectdump.html
> I changed magnitude and direction in test program. Changing direction
> doesn't send anything to device.
> 
>> Constant force makes no sense without a direction. What kind of force effect
>> does it actually produce?
> Just rumble. There is only one motor inside. For constant effect motor
> spins with constant speed, as I understood, this speed is controlled
> by magnitude. Also it is a place for report fixup: magnitude logical
> values are -127/127, but actually 1/255 -- 255 is strongest.

Hmm, so it is just rumble ( = periodic), not constant force. But what do 
the periodic effects do then, if constant is rumble? Normally periodic 
effects represent various types of rumble.

>>> After that, driver almost initializes: http://paste.org.ru/index.pl?008sgm
>>> It fails in pidff_check_autocenter(). According to descriptor, effect
>>> 1 is constant force. The problem is that block load report receive
>>> fails.
>>> I have no idea yet, why it fails, need to do some debug.
> ...
>> However, pidff_request_effect_upload should still not fail, as it is needed
>> for uploading effects. Try changing the effect type, make it e.g.
>> error = pidff_request_effect_upload(pidff, 2);
> Actually I tried to comment out check_autocenter() - then driver
> initializes successfully. But fftest fails to upload effect with io
> error.
> 
>> You could also try adding some delays after usbhid_wait_io() calls in
>> pidff_request_effect_upload(), with e.g. msleep(200) (you should also lower
>> the iterations limit 60 when adding such delays).
> I tried this, no result.
> I think, this problem is connected somehow to log message about
> maxusage and report_count do not match.

It is not related to those. I see two likely reasons:
1) Device needs more initialization; in the dump we see a "Actuators 
Enable" command sent first, and then the vendor report 64 three times 
with various data.
2) Reports 21+22 are transmitted as control transfers in the dump. I'll 
have to check whether we are doing the same.

>> If nothing else helps, I guess we need a dump from windows for this as well.
>>
>>> Can you tell,
>>> is there some way to monitor reports in kernel?
>> You can set debug=2 for hid module, but it will produce very much output.
> I found where to get outgoing data, added 2 dbg_hid() loggers, now
> have to find right place for incoming reports.
> 


-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/2/12 Anssi Hannula <anssi.hannula@gmail.com>:
>> Just rumble. There is only one motor inside. For constant effect motor
>> spins with constant speed, as I understood, this speed is controlled
>> by magnitude. Also it is a place for report fixup: magnitude logical
>> values are -127/127, but actually 1/255 -- 255 is strongest.
>
> Hmm, so it is just rumble ( = periodic), not constant force. But what do the
> periodic effects do then, if constant is rumble? Normally periodic effects
> represent various types of rumble.
Not exactly periodic, but same effect may be achieved using periodic
effect with 0 period.
This constant effect sets constant motor speed (and also could be more
powerful, than periodic).
Periodic effects control motor speed using effect type and parameters,
so speed amplitude graph is sinusoidal or saw-like.
Envelope for constant effect controls effect playback begin and end.
For periodic effects it controls every period, as I understood.

>> I tried this, no result.
>> I think, this problem is connected somehow to log message about
>> maxusage and report_count do not match.
>
> It is not related to those. I see two likely reasons:
> 1) Device needs more initialization; in the dump we see a "Actuators Enable"
> command sent first, and then the vendor report 64 three times with various
> data.
May be. I'll try to find some other PID drivers for windows to see if
it will work without vendor reports.

> 2) Reports 21+22 are transmitted as control transfers in the dump. I'll have
> to check whether we are doing the same.
We do: http://paste.org.ru/index.pl?iyvkmg
Got this log with patch: http://paste.org.ru/index.pl?m4wauv

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
> 2009/2/12 Anssi Hannula <anssi.hannula@gmail.com>:
>>> Just rumble. There is only one motor inside. For constant effect motor
>>> spins with constant speed, as I understood, this speed is controlled
>>> by magnitude. Also it is a place for report fixup: magnitude logical
>>> values are -127/127, but actually 1/255 -- 255 is strongest.
>> Hmm, so it is just rumble ( = periodic), not constant force. But what do the
>> periodic effects do then, if constant is rumble? Normally periodic effects
>> represent various types of rumble.
> Not exactly periodic, but same effect may be achieved using periodic
> effect with 0 period.
> This constant effect sets constant motor speed (and also could be more
> powerful, than periodic).
> Periodic effects control motor speed using effect type and parameters,
> so speed amplitude graph is sinusoidal or saw-like.

Okay, so periodic effects are also different than what they are supposed
to be. Saw-like etc define the force direction/magnitude (e.g. saw-like:
the joystick increasingly pulls to one direction, then stops suddenly
and starts doing it again; this happens quite quickly of course).

Basically this is a simple rumble device somewhat adapted into the PID
model.

The best we can do is define constant force as FF_RUMBLE and pretend
periodic is FF_PERIODIC.

> Envelope for constant effect controls effect playback begin and end.

That is what it is supposed to do.

> For periodic effects it controls every period, as I understood.

That is not supposed to be the case and seems very strange. Can you
confirm this (maybe after we get it working on linux)?

>>> I tried this, no result.
>>> I think, this problem is connected somehow to log message about
>>> maxusage and report_count do not match.
>> It is not related to those. I see two likely reasons:
>> 1) Device needs more initialization; in the dump we see a "Actuators Enable"
>> command sent first, and then the vendor report 64 three times with various
>> data.
> May be. I'll try to find some other PID drivers for windows to see if
> it will work without vendor reports.
> 
>> 2) Reports 21+22 are transmitted as control transfers in the dump. I'll have
>> to check whether we are doing the same.
> We do: http://paste.org.ru/index.pl?iyvkmg
> Got this log with patch: http://paste.org.ru/index.pl?m4wauv

Notice the difference in length of report 22 on windows and linux. Try
this in pidff_init():
pidff->reports[PID_BLOCK_LOAD]->size += 8;

-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/2/14 Anssi Hannula <anssi.hannula@gmail.com>:
>>>> For periodic effects it controls every period, as I understood.
>>> That is not supposed to be the case and seems very strange. Can you
>>> confirm this (maybe after we get it working on linux)?
>> I just admitted that.
> How did you test it?
I didn't. I just supposed, that it should be so.

>>>>> 2) Reports 21+22 are transmitted as control transfers in the dump. I'll have
>>>>> to check whether we are doing the same.
>>>> We do: http://paste.org.ru/index.pl?iyvkmg
>>>> Got this log with patch: http://paste.org.ru/index.pl?m4wauv
>>> Notice the difference in length of report 22 on windows and linux. Try
>>> this in pidff_init():
>>> pidff->reports[PID_BLOCK_LOAD]->size += 8;
>> That's it!
>> I didn't pay attention to length value. Looks like a missing usage in
>> report descriptor!
>> So, quirk with report fix is needed for this device.
>>
>> Also i found one strange thing: ffmvforce utility brings driver to
>> inconsistent state. http://paste.org.ru/index.pl?1ajtxt
>>
>
> Hmm, this says problems start with "fftest".
> [10054.751832] HID: implement() called with too large value 47113! (fftest)
>
> Could you print all the values set in pidff_set_effect_report() just
> before the usbhid_submit_report() call, and try to reproduce the WARNING
> with fftest.
Got it: http://paste.org.ru/index.pl?fv7wss
Then after launching ffmvforce: http://paste.org.ru/index.pl?3pdotg

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

>>>> Notice the difference in length of report 22 on windows and linux. Try
>>>> this in pidff_init():
>>>> pidff->reports[PID_BLOCK_LOAD]->size += 8;
>>> That's it!
>>> I didn't pay attention to length value. Looks like a missing usage in
>>> report descriptor!
>>> So, quirk with report fix is needed for this device.
I'm trying to understand better way to make this descriptor fix. Can
you give me an advice on this?
Or it's better to wait for patch discussed in "Allow drivers to
replace report descriptors" topic?

>> Hmm, this says problems start with "fftest".
>> [10054.751832] HID: implement() called with too large value 47113! (fftest)
>>
>> Could you print all the values set in pidff_set_effect_report() just
>> before the usbhid_submit_report() call, and try to reproduce the WARNING
>> with fftest.
Problem in fftest is simple: using uninitialized structures. That also
was causing slow path warning.
But running ffmvforce after fftest brings device to unconsistent
state, causing messages "usb 1-1: ctrl urb status -62 received".

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
>>>>> Notice the difference in length of report 22 on windows and linux. Try
>>>>> this in pidff_init():
>>>>> pidff->reports[PID_BLOCK_LOAD]->size += 8;
>>>> That's it!
>>>> I didn't pay attention to length value. Looks like a missing usage in
>>>> report descriptor!
>>>> So, quirk with report fix is needed for this device.
> I'm trying to understand better way to make this descriptor fix. Can
> you give me an advice on this?
> Or it's better to wait for patch discussed in "Allow drivers to
> replace report descriptors" topic?

I see there are two options:
a) Add a device-specific quirk in pidff_init(), or
b) Add hid-saitek.c that does the adjustment.

Jiri (CCd, HID maintainer), WDYT?

>>> Hmm, this says problems start with "fftest".
>>> [10054.751832] HID: implement() called with too large value 47113! (fftest)
>>>
>>> Could you print all the values set in pidff_set_effect_report() just
>>> before the usbhid_submit_report() call, and try to reproduce the WARNING
>>> with fftest.
> Problem in fftest is simple: using uninitialized structures. That also
> was causing slow path warning.
> But running ffmvforce after fftest brings device to unconsistent
> state, causing messages "usb 1-1: ctrl urb status -62 received".

Does this happen also with fixed fftest?

Of course hid-pidff should also check the values provided, I'll fix this 
as well.

I'll try to provide the discussed patch(es) next weekend.

-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/2/18 Anssi Hannula <anssi.hannula@gmail.com>:
>> Problem in fftest is simple: using uninitialized structures. That also
>> was causing slow path warning.
>> But running ffmvforce after fftest brings device to unconsistent
>> state, causing messages "usb 1-1: ctrl urb status -62 received".
>
> Does this happen also with fixed fftest?
Yes. That's the problem.
Exactly this sequence: run fftest, play all effects, then run
ffmvforce. ffmvforce itself is working well.

> Of course hid-pidff should also check the values provided, I'll fix this as
> well.
I think it'd be also good to look what could cause slow path warning.

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/2/19 Anssi Hannula <anssi.hannula@gmail.com>
>
> Dmitriy Geels wrote:
>>
>> Yes. That's the problem.
>> Exactly this sequence: run fftest, play all effects, then run
>> ffmvforce. ffmvforce itself is working well.
>
> Could you provide the log with fixed fftest+ffmvforce?

Sorry for making you waiting so long, here is it:
http://paste.org.ru/index.pl?p584s9

Nothing special -- running fftest, playing 0, 1, 2, 3, 4, 5 (2 and 3
does nothing), then -1, then run ffmvforce, click somewhere inside
it's window -- then first "pid_block_load requested" appears, after
"pid_block_load failed 20 times" (I decreased retries counter)
ffmvforce continues to work, but ioctl() call always return -1

That's all. Running fftest again repeats "pid_block_load requested"
scenario. Device (or driver?) stuck.

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
> 2009/2/19 Anssi Hannula <anssi.hannula@gmail.com>
>> Dmitriy Geels wrote:
>>> Yes. That's the problem.
>>> Exactly this sequence: run fftest, play all effects, then run
>>> ffmvforce. ffmvforce itself is working well.
>> Could you provide the log with fixed fftest+ffmvforce?
> 
> Sorry for making you waiting so long, here is it:
> http://paste.org.ru/index.pl?p584s9
> 
> Nothing special -- running fftest, playing 0, 1, 2, 3, 4, 5 (2 and 3
> does nothing), then -1, then run ffmvforce, click somewhere inside
> it's window -- then first "pid_block_load requested" appears, after
> "pid_block_load failed 20 times" (I decreased retries counter)
> ffmvforce continues to work, but ioctl() call always return -1
> 
> That's all. Running fftest again repeats "pid_block_load requested"
> scenario. Device (or driver?) stuck.

These seem strange:
[ 5108.143241] usbhid:hid-pidff: create_new_effect sent, type: 4
[ 5108.161645] usbhid:hid-pidff: create_new_effect sent, type: 1
[ 5108.251905] usbhid:hid-pidff: create_new_effect sent, type: 4
[ 5108.338133] usbhid:hid-pidff: create_new_effect sent, type: 4

Type 4 means FF_SINE for your device. However, fftest does test only one
FF_SINE effect. Have you altered fftest? If not, could you post the
output of fftest?

-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/2/27 Anssi Hannula <anssi.hannula@gmail.com>:
> These seem strange:
> [ 5108.143241] usbhid:hid-pidff: create_new_effect sent, type: 4
> [ 5108.161645] usbhid:hid-pidff: create_new_effect sent, type: 1
> [ 5108.251905] usbhid:hid-pidff: create_new_effect sent, type: 4
> [ 5108.338133] usbhid:hid-pidff: create_new_effect sent, type: 4
>
> Type 4 means FF_SINE for your device. However, fftest does test only one
> FF_SINE effect. Have you altered fftest? If not, could you post the
> output of fftest?
No i didn't, fftest plays 4 different effect types, of which only 2
are supported,
Here is output: http://paste.org.ru/index.pl?jc3999
Here is source of this fftest: http://paste.org.ru/index.pl?jjkw01

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
> 2009/2/27 Anssi Hannula <anssi.hannula@gmail.com>:
>> These seem strange:
>> [ 5108.143241] usbhid:hid-pidff: create_new_effect sent, type: 4
>> [ 5108.161645] usbhid:hid-pidff: create_new_effect sent, type: 1
>> [ 5108.251905] usbhid:hid-pidff: create_new_effect sent, type: 4
>> [ 5108.338133] usbhid:hid-pidff: create_new_effect sent, type: 4
>>
>> Type 4 means FF_SINE for your device. However, fftest does test only one
>> FF_SINE effect. Have you altered fftest? If not, could you post the
>> output of fftest?
> No i didn't, fftest plays 4 different effect types, of which only 2
> are supported,
> Here is output: http://paste.org.ru/index.pl?jc3999
> Here is source of this fftest: http://paste.org.ru/index.pl?jjkw01

Oh, right, FF_RUMBLE is mapped to FF_PERIODIC by default. I have a local
patch for your device that maps it to FF_CONSTANT, that's why I was
confused.

So that wasn't the issue.

What about running fftest two times in succession, or just ffmvforce two
times in succession, does it trigger the problem?

Also, try removing some effects from fftest one-by-one, and see if the
problem is still triggered.

-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/3/2 Anssi Hannula <anssi.hannula@gmail.com>:
> Oh, right, FF_RUMBLE is mapped to FF_PERIODIC by default. I have a local
> patch for your device that maps it to FF_CONSTANT, that's why I was
> confused.
>
> So that wasn't the issue.
>
> What about running fftest two times in succession, or just ffmvforce two
> times in succession, does it trigger the problem?
Tried already. No problem. Same with ffmvforce.

> Also, try removing some effects from fftest one-by-one, and see if the
> problem is still triggered.
Will try this later, this evening.

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/3/3 Dmitriy Geels <dmitriy.geels@gmail.com>:
>> What about running fftest two times in succession, or just ffmvforce two
>> times in succession, does it trigger the problem?
> Tried already. No problem. Same with ffmvforce.
I was wrong: running ffmvforce twice leads to stuck device. There
should be some initialization/deinitialization problem in it.

>> Also, try removing some effects from fftest one-by-one, and see if the
>> problem is still triggered.
> Will try this later, this evening.
ffmvforce gets stuck after any effect played in fftest.

Here is it's source, may be you'll see the problem:
http://paste.org.ru/index.pl?s8x9x5

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
> 2009/3/3 Dmitriy Geels <dmitriy.geels@gmail.com>:
>>> What about running fftest two times in succession, or just ffmvforce two
>>> times in succession, does it trigger the problem?
>> Tried already. No problem. Same with ffmvforce.
> I was wrong: running ffmvforce twice leads to stuck device. There
> should be some initialization/deinitialization problem in it.
> 
>>> Also, try removing some effects from fftest one-by-one, and see if the
>>> problem is still triggered.
>> Will try this later, this evening.
> ffmvforce gets stuck after any effect played in fftest.

Does this mean it does not get stuck if you start fftest but do not play
any effects?

> Here is it's source, may be you'll see the problem:
> http://paste.org.ru/index.pl?s8x9x5

Provide the kernel log again, this time with debug=2 set for hid module
and having run "fftest" with only one effect and then "ffmvforce".

-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/3/7 Anssi Hannula <anssi.hannula@gmail.com>:
> Dmitriy Geels wrote:
>> ffmvforce gets stuck after any effect played in fftest.
>
> Does this mean it does not get stuck if you start fftest but do not play
> any effects?
No problem is reproduced in this case also. Looks like, problem is
caused by effect loading in fftest.
this case: http://paste.org.ru/index.pl?cjl2b8

>> Here is it's source, may be you'll see the problem:
>> http://paste.org.ru/index.pl?s8x9x5
>
> Provide the kernel log again, this time with debug=2 set for hid module
> and having run "fftest" with only one effect and then "ffmvforce".
here is it: http://paste.org.ru/index.pl?4bwah8

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

Dmitriy Geels wrote:
> 2009/3/7 Anssi Hannula <anssi.hannula@gmail.com>:
>> Provide the kernel log again, this time with debug=2 set for hid module
>> and having run "fftest" with only one effect and then "ffmvforce".
> here is it: http://paste.org.ru/index.pl?4bwah8

Apply attached patch that adds more debug output and try this again.

-- 
Anssi Hannula

[-- Attachment #2: hid-queue-debugging.diff --]
[-- Type: text/plain, Size: 4509 bytes --]

--- linux-2629-pidff/drivers/hid/usbhid/hid-core.c	2009-02-13 03:47:15.000000000 +0200
+++ linux-2629-pidff/drivers/hid/usbhid/hid-core-clear.c	2009-03-08 12:13:38.000000000 +0200
@@ -244,7 +244,7 @@ static int hid_submit_out(struct hid_dev
 	memcpy(usbhid->outbuf, raw_report, usbhid->urbout->transfer_buffer_length);
 	kfree(raw_report);
 
-	dbg_hid("submitting out urb\n");
+	dbg_hid("submitting out urb %d\n", usbhid->outtail);
 
 	if (usb_submit_urb(usbhid->urbout, GFP_ATOMIC)) {
 		err_hid("usb_submit_urb(out) failed");
@@ -298,6 +298,7 @@ static int hid_submit_ctrl(struct hid_de
 		usbhid->cr->bRequest == HID_REQ_SET_REPORT ? "Set_Report" : "Get_Report",
 		usbhid->cr->wValue, usbhid->cr->wIndex, usbhid->cr->wLength);
 
+	dbg_hid("queue number %d\n", usbhid->ctrltail);
 	if (usb_submit_urb(usbhid->urbctrl, GFP_ATOMIC)) {
 		err_hid("usb_submit_urb(ctrl) failed");
 		return -1;
@@ -333,14 +334,16 @@ static void hid_irq_out(struct urb *urb)
 	}
 
 	spin_lock_irqsave(&usbhid->outlock, flags);
-
+dbg_hid("hid_irq_out callback for %d\n", usbhid->outtail);
 	if (unplug)
 		usbhid->outtail = usbhid->outhead;
 	else
 		usbhid->outtail = (usbhid->outtail + 1) & (HID_OUTPUT_FIFO_SIZE - 1);
 
 	if (usbhid->outhead != usbhid->outtail) {
+		dbg_hid("sending next report, not releasing lock, next is %d\n", usbhid->outtail);
 		if (hid_submit_out(hid)) {
+			dbg_hid("HID_OUT_RUNNING released in hid_irq_out(), hid_submit_out failure\n");
 			clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
 			wake_up(&usbhid->wait);
 		}
@@ -348,6 +351,7 @@ static void hid_irq_out(struct urb *urb)
 		return;
 	}
 
+	dbg_hid("clearing HID_OUT_RUNNING in hid_irq_out(), transfers done\n");
 	clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
 	spin_unlock_irqrestore(&usbhid->outlock, flags);
 	wake_up(&usbhid->wait);
@@ -386,13 +390,16 @@ static void hid_ctrl(struct urb *urb)
 				"received\n", urb->status);
 	}
 
+	dbg_hid("hid_ctrl() for feature report %d\n", usbhid->ctrltail);
 	if (unplug)
 		usbhid->ctrltail = usbhid->ctrlhead;
 	else
 		usbhid->ctrltail = (usbhid->ctrltail + 1) & (HID_CONTROL_FIFO_SIZE - 1);
 
 	if (usbhid->ctrlhead != usbhid->ctrltail) {
+		dbg_hid("submitting next ctrl report, nr is %d\n", usbhid->ctrltail);
 		if (hid_submit_ctrl(hid)) {
+			dbg_hid("HID_CTRL_RUNNING released in hid_ctrl(), hid_submit_ctrl failure\n");
 			clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
 			wake_up(&usbhid->wait);
 		}
@@ -400,6 +407,7 @@ static void hid_ctrl(struct urb *urb)
 		return;
 	}
 
+	dbg_hid("clearing HID_CTRL_RUNNING in hid_ctrl(), transfers done\n");
 	clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
 	spin_unlock_irqrestore(&usbhid->ctrllock, flags);
 	wake_up(&usbhid->wait);
@@ -435,9 +443,14 @@ void usbhid_submit_report(struct hid_dev
 		usbhid->out[usbhid->outhead].report = report;
 		usbhid->outhead = head;
 
-		if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl))
-			if (hid_submit_out(hid))
+		dbg_hid("usbhid_submit_report() done for output report %d\n", head - 1);
+		if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
+			dbg_hid("locked HID_OUT_RUNNING in usbhid_submit_report()\n")
+			if (hid_submit_out(hid)) {
+				dbg_hid("cleared HID_OUT_RUNNING in usbhid_submit_report(), failure in hid_submit_out\n");
 				clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
+			}
+		}
 
 		spin_unlock_irqrestore(&usbhid->outlock, flags);
 		return;
@@ -463,10 +476,14 @@ void usbhid_submit_report(struct hid_dev
 	usbhid->ctrl[usbhid->ctrlhead].report = report;
 	usbhid->ctrl[usbhid->ctrlhead].dir = dir;
 	usbhid->ctrlhead = head;
-
-	if (!test_and_set_bit(HID_CTRL_RUNNING, &usbhid->iofl))
-		if (hid_submit_ctrl(hid))
+	dbg_hid("usbhid_submit_report() done for feature report %d\n", head - 1);
+	if (!test_and_set_bit(HID_CTRL_RUNNING, &usbhid->iofl)) {
+		dbg_hid("locked HID_CTRL_RUNNING in usbhid_submit_report()\n");
+		if (hid_submit_ctrl(hid)) {
+			dbg_hid("cleared HID_CTRL_RUNNING in usbhid_submit_report(), failure in hid_submit_ctrl\n");
 			clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
+		}
+	}
 
 	spin_unlock_irqrestore(&usbhid->ctrllock, flags);
 }
@@ -503,7 +520,7 @@ int usbhid_wait_io(struct hid_device *hi
 				(!test_bit(HID_CTRL_RUNNING, &usbhid->iofl) &&
 				!test_bit(HID_OUT_RUNNING, &usbhid->iofl)),
 					10*HZ)) {
-		dbg_hid("timeout waiting for ctrl or out queue to clear\n");
+		dbg_hid("timeout waiting for ctrl or out queue to clear, %d, %d\n", test_bit(HID_CTRL_RUNNING, &usbhid->iofl), test_bit(HID_OUT_RUNNING, &usbhid->iofl));
 		return -1;
 	}
 

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/3/8 Anssi Hannula <anssi.hannula@gmail.com>:
> Dmitriy Geels wrote:
>> 2009/3/7 Anssi Hannula <anssi.hannula@gmail.com>:
>>> Provide the kernel log again, this time with debug=2 set for hid module
>>> and having run "fftest" with only one effect and then "ffmvforce".
>> here is it: http://paste.org.ru/index.pl?4bwah8
>
> Apply attached patch that adds more debug output and try this again.
Here is log: http://paste.org.ru/index.pl?0mtjuz

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

[-- Attachment #1: Type: text/plain, Size: 667 bytes --]

Dmitriy Geels wrote:
> 2009/3/8 Anssi Hannula <anssi.hannula@gmail.com>:
>> Dmitriy Geels wrote:
>>> 2009/3/7 Anssi Hannula <anssi.hannula@gmail.com>:
>>>> Provide the kernel log again, this time with debug=2 set for hid module
>>>> and having run "fftest" with only one effect and then "ffmvforce".
>>> here is it: http://paste.org.ru/index.pl?4bwah8
>> Apply attached patch that adds more debug output and try this again.
> Here is log: http://paste.org.ru/index.pl?0mtjuz
> 

Sorry for the long delay.

Try the attached (untested) patch. It adds a simple timeout to the hid
ctrl and out urbs, discarding the faulty urb instead of getting stuck.

-- 
Anssi Hannula

[-- Attachment #2: hid-usb-add-urb-timeout.diff --]
[-- Type: text/plain, Size: 3375 bytes --]

---
 drivers/hid/usbhid/hid-core.c |   18 ++++++++++++++++++
 drivers/hid/usbhid/usbhid.h   |    2 ++
 2 files changed, 20 insertions(+)

Index: linux-2629-pidff/drivers/hid/usbhid/hid-core.c
===================================================================
--- linux-2629-pidff.orig/drivers/hid/usbhid/hid-core.c	2009-02-13 03:47:15.000000000 +0200
+++ linux-2629-pidff/drivers/hid/usbhid/hid-core.c	2009-05-08 02:40:47.000000000 +0300
@@ -251,6 +251,8 @@ static int hid_submit_out(struct hid_dev
 		return -1;
 	}
 
+	add_timer(&usbhid->urb_out_timeout, jiffies + msecs_to_jiffies(8000));
+
 	return 0;
 }
 
@@ -303,6 +305,8 @@ static int hid_submit_ctrl(struct hid_de
 		return -1;
 	}
 
+	add_timer(&usbhid->urb_ctrl_timeout, jiffies + msecs_to_jiffies(8000));
+
 	return 0;
 }
 
@@ -317,6 +321,8 @@ static void hid_irq_out(struct urb *urb)
 	unsigned long flags;
 	int unplug = 0;
 
+	del_timer(usbhid->urb_out_timeout);
+
 	switch (urb->status) {
 	case 0:			/* success */
 		break;
@@ -364,6 +370,7 @@ static void hid_ctrl(struct urb *urb)
 	unsigned long flags;
 	int unplug = 0;
 
+	del_timer(usbhid->urb_ctrl_timeout);
 	spin_lock_irqsave(&usbhid->ctrllock, flags);
 
 	switch (urb->status) {
@@ -405,6 +412,13 @@ static void hid_ctrl(struct urb *urb)
 	wake_up(&usbhid->wait);
 }
 
+static void hid_urb_timeout(unsigned long timer_data)
+{
+	struct urb *hid_urb = (struct urb *)timer_data;
+	dev_warn(&hid_urb->dev->dev, "hid urb timeout\n");
+	usb_unlink_urb(hid_urb);
+}
+
 void usbhid_submit_report(struct hid_device *hid, struct hid_report *report, unsigned char dir)
 {
 	int head;
@@ -851,6 +865,8 @@ static int usbhid_start(struct hid_devic
 	init_waitqueue_head(&usbhid->wait);
 	INIT_WORK(&usbhid->reset_work, hid_reset);
 	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
+	setup_timer(&usbhid->urb_out_timeout, hid_urb_timeout, (unsigned long) usbhid->urbout);
+	setup_timer(&usbhid->urb_ctrl_timeout, hid_urb_timeout, (unsigned long) usbhid->urbctrl);
 
 	spin_lock_init(&usbhid->inlock);
 	spin_lock_init(&usbhid->outlock);
@@ -915,6 +931,8 @@ static void usbhid_stop(struct hid_devic
 
 	del_timer_sync(&usbhid->io_retry);
 	cancel_work_sync(&usbhid->reset_work);
+	del_timer_sync(&usbhid->hid_out_timeout);
+	del_timer_sync(&usbhid->hid_ctrl_timeout);
 
 	if (hid->claimed & HID_CLAIMED_INPUT)
 		hidinput_disconnect(hid);
Index: linux-2629-pidff/drivers/hid/usbhid/usbhid.h
===================================================================
--- linux-2629-pidff.orig/drivers/hid/usbhid/usbhid.h	2009-05-08 01:53:33.000000000 +0300
+++ linux-2629-pidff/drivers/hid/usbhid/usbhid.h	2009-05-08 01:54:02.000000000 +0300
@@ -85,6 +85,8 @@ struct usbhid_device {
 	spinlock_t outlock;                                             /* Output fifo spinlock */
 
 	unsigned long iofl;                                             /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
+	struct timer_list urb_out_timeout;                              /* Timeout for out urb */
+	struct timer_list urb_ctrl_timeout;                             /* Timeout for ctrl urb */
 	struct timer_list io_retry;                                     /* Retry timer */
 	unsigned long stop_retry;                                       /* Time to give up, in jiffies */
 	unsigned int retry_delay;                                       /* Delay length in ms */

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

Anssi Hannula wrote:
> Dmitriy Geels wrote:
>> 2009/3/8 Anssi Hannula <anssi.hannula@gmail.com>:
>>> Dmitriy Geels wrote:
>>>> 2009/3/7 Anssi Hannula <anssi.hannula@gmail.com>:
>>>>> Provide the kernel log again, this time with debug=2 set for hid module
>>>>> and having run "fftest" with only one effect and then "ffmvforce".
>>>> here is it: http://paste.org.ru/index.pl?4bwah8
>>> Apply attached patch that adds more debug output and try this again.
>> Here is log: http://paste.org.ru/index.pl?0mtjuz
>>
> 
> Sorry for the long delay.
> 
> Try the attached (untested) patch. It adds a simple timeout to the hid
> ctrl and out urbs, discarding the faulty urb instead of getting stuck.

And of course there was an error, attached is a fixed patch :)

-- 
Anssi Hannula

[-- Attachment #2: hid-usb-add-urb-timeout.diff --]
[-- Type: text/plain, Size: 3377 bytes --]

---
 drivers/hid/usbhid/hid-core.c |   18 ++++++++++++++++++
 drivers/hid/usbhid/usbhid.h   |    2 ++
 2 files changed, 20 insertions(+)

Index: linux-2629-pidff/drivers/hid/usbhid/hid-core.c
===================================================================
--- linux-2629-pidff.orig/drivers/hid/usbhid/hid-core.c	2009-02-13 03:47:15.000000000 +0200
+++ linux-2629-pidff/drivers/hid/usbhid/hid-core.c	2009-05-08 02:55:59.000000000 +0300
@@ -251,6 +251,8 @@ static int hid_submit_out(struct hid_dev
 		return -1;
 	}
 
+	add_timer(&usbhid->urb_out_timeout, jiffies + msecs_to_jiffies(8000));
+
 	return 0;
 }
 
@@ -303,6 +305,8 @@ static int hid_submit_ctrl(struct hid_de
 		return -1;
 	}
 
+	add_timer(&usbhid->urb_ctrl_timeout, jiffies + msecs_to_jiffies(8000));
+
 	return 0;
 }
 
@@ -317,6 +321,8 @@ static void hid_irq_out(struct urb *urb)
 	unsigned long flags;
 	int unplug = 0;
 
+	del_timer(&usbhid->urb_out_timeout);
+
 	switch (urb->status) {
 	case 0:			/* success */
 		break;
@@ -364,6 +370,7 @@ static void hid_ctrl(struct urb *urb)
 	unsigned long flags;
 	int unplug = 0;
 
+	del_timer(&usbhid->urb_ctrl_timeout);
 	spin_lock_irqsave(&usbhid->ctrllock, flags);
 
 	switch (urb->status) {
@@ -405,6 +412,13 @@ static void hid_ctrl(struct urb *urb)
 	wake_up(&usbhid->wait);
 }
 
+static void hid_urb_timeout(unsigned long timer_data)
+{
+	struct urb *hid_urb = (struct urb *)timer_data;
+	dev_warn(&hid_urb->dev->dev, "hid urb timeout\n");
+	usb_unlink_urb(hid_urb);
+}
+
 void usbhid_submit_report(struct hid_device *hid, struct hid_report *report, unsigned char dir)
 {
 	int head;
@@ -851,6 +865,8 @@ static int usbhid_start(struct hid_devic
 	init_waitqueue_head(&usbhid->wait);
 	INIT_WORK(&usbhid->reset_work, hid_reset);
 	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
+	setup_timer(&usbhid->urb_out_timeout, hid_urb_timeout, (unsigned long) usbhid->urbout);
+	setup_timer(&usbhid->urb_ctrl_timeout, hid_urb_timeout, (unsigned long) usbhid->urbctrl);
 
 	spin_lock_init(&usbhid->inlock);
 	spin_lock_init(&usbhid->outlock);
@@ -915,6 +931,8 @@ static void usbhid_stop(struct hid_devic
 
 	del_timer_sync(&usbhid->io_retry);
 	cancel_work_sync(&usbhid->reset_work);
+	del_timer_sync(&usbhid->hid_out_timeout);
+	del_timer_sync(&usbhid->hid_ctrl_timeout);
 
 	if (hid->claimed & HID_CLAIMED_INPUT)
 		hidinput_disconnect(hid);
Index: linux-2629-pidff/drivers/hid/usbhid/usbhid.h
===================================================================
--- linux-2629-pidff.orig/drivers/hid/usbhid/usbhid.h	2009-05-08 01:53:33.000000000 +0300
+++ linux-2629-pidff/drivers/hid/usbhid/usbhid.h	2009-05-08 01:54:02.000000000 +0300
@@ -85,6 +85,8 @@ struct usbhid_device {
 	spinlock_t outlock;                                             /* Output fifo spinlock */
 
 	unsigned long iofl;                                             /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
+	struct timer_list urb_out_timeout;                              /* Timeout for out urb */
+	struct timer_list urb_ctrl_timeout;                             /* Timeout for ctrl urb */
 	struct timer_list io_retry;                                     /* Retry timer */
 	unsigned long stop_retry;                                       /* Time to give up, in jiffies */
 	unsigned int retry_delay;                                       /* Delay length in ms */

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

Dmitriy Geels wrote:
> 2009/5/8 Anssi Hannula <anssi.hannula@gmail.com>:
>> Anssi Hannula wrote:
>>> Dmitriy Geels wrote:
>>>> 2009/3/8 Anssi Hannula <anssi.hannula@gmail.com>:
>>>>> Dmitriy Geels wrote:
>>>>>> 2009/3/7 Anssi Hannula <anssi.hannula@gmail.com>:
>>>>>>> Provide the kernel log again, this time with debug=2 set for hid module
>>>>>>> and having run "fftest" with only one effect and then "ffmvforce".
>>>>>> here is it: http://paste.org.ru/index.pl?4bwah8
>>>>> Apply attached patch that adds more debug output and try this again.
>>>> Here is log: http://paste.org.ru/index.pl?0mtjuz
>>>>
>>> Sorry for the long delay.
>>>
>>> Try the attached (untested) patch. It adds a simple timeout to the hid
>>> ctrl and out urbs, discarding the faulty urb instead of getting stuck.
>> And of course there was an error, attached is a fixed patch :)
> Just tried to build module with your patch. It doesn't build.
> 

Fixed patch attached.

-- 
Anssi Hannula

[-- Attachment #2: hid-usb-add-urb-timeout.diff --]
[-- Type: text/plain, Size: 3604 bytes --]

---
 drivers/hid/usbhid/hid-core.c |   21 +++++++++++++++++++++
 drivers/hid/usbhid/usbhid.h   |    2 ++
 2 files changed, 23 insertions(+)

Index: linux-2629-pidff/drivers/hid/usbhid/hid-core.c
===================================================================
--- linux-2629-pidff.orig/drivers/hid/usbhid/hid-core.c	2009-02-13 03:47:15.000000000 +0200
+++ linux-2629-pidff/drivers/hid/usbhid/hid-core.c	2009-06-06 15:08:09.000000000 +0300
@@ -27,6 +27,7 @@
 #include <asm/byteorder.h>
 #include <linux/input.h>
 #include <linux/wait.h>
+#include <linux/timer.h>
 
 #include <linux/usb.h>
 
@@ -251,6 +252,9 @@ static int hid_submit_out(struct hid_dev
 		return -1;
 	}
 
+	usbhid->urb_out_timeout.expires = jiffies + msecs_to_jiffies(8000);
+	add_timer(&usbhid->urb_out_timeout);
+
 	return 0;
 }
 
@@ -303,6 +307,9 @@ static int hid_submit_ctrl(struct hid_de
 		return -1;
 	}
 
+	usbhid->urb_ctrl_timeout.expires = jiffies + msecs_to_jiffies(8000);
+	add_timer(&usbhid->urb_ctrl_timeout);
+
 	return 0;
 }
 
@@ -317,6 +324,8 @@ static void hid_irq_out(struct urb *urb)
 	unsigned long flags;
 	int unplug = 0;
 
+	del_timer(&usbhid->urb_out_timeout);
+
 	switch (urb->status) {
 	case 0:			/* success */
 		break;
@@ -364,6 +373,7 @@ static void hid_ctrl(struct urb *urb)
 	unsigned long flags;
 	int unplug = 0;
 
+	del_timer(&usbhid->urb_ctrl_timeout);
 	spin_lock_irqsave(&usbhid->ctrllock, flags);
 
 	switch (urb->status) {
@@ -405,6 +415,13 @@ static void hid_ctrl(struct urb *urb)
 	wake_up(&usbhid->wait);
 }
 
+static void hid_urb_timeout(unsigned long timer_data)
+{
+	struct urb *hid_urb = (struct urb *)timer_data;
+	dev_warn(&hid_urb->dev->dev, "hid urb timeout\n");
+	usb_unlink_urb(hid_urb);
+}
+
 void usbhid_submit_report(struct hid_device *hid, struct hid_report *report, unsigned char dir)
 {
 	int head;
@@ -851,6 +868,8 @@ static int usbhid_start(struct hid_devic
 	init_waitqueue_head(&usbhid->wait);
 	INIT_WORK(&usbhid->reset_work, hid_reset);
 	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
+	setup_timer(&usbhid->urb_out_timeout, hid_urb_timeout, (unsigned long) usbhid->urbout);
+	setup_timer(&usbhid->urb_ctrl_timeout, hid_urb_timeout, (unsigned long) usbhid->urbctrl);
 
 	spin_lock_init(&usbhid->inlock);
 	spin_lock_init(&usbhid->outlock);
@@ -915,6 +934,8 @@ static void usbhid_stop(struct hid_devic
 
 	del_timer_sync(&usbhid->io_retry);
 	cancel_work_sync(&usbhid->reset_work);
+	del_timer_sync(&usbhid->urb_out_timeout);
+	del_timer_sync(&usbhid->urb_ctrl_timeout);
 
 	if (hid->claimed & HID_CLAIMED_INPUT)
 		hidinput_disconnect(hid);
Index: linux-2629-pidff/drivers/hid/usbhid/usbhid.h
===================================================================
--- linux-2629-pidff.orig/drivers/hid/usbhid/usbhid.h	2009-05-08 01:53:33.000000000 +0300
+++ linux-2629-pidff/drivers/hid/usbhid/usbhid.h	2009-05-08 01:54:02.000000000 +0300
@@ -85,6 +85,8 @@ struct usbhid_device {
 	spinlock_t outlock;                                             /* Output fifo spinlock */
 
 	unsigned long iofl;                                             /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
+	struct timer_list urb_out_timeout;                              /* Timeout for out urb */
+	struct timer_list urb_ctrl_timeout;                             /* Timeout for ctrl urb */
 	struct timer_list io_retry;                                     /* Retry timer */
 	unsigned long stop_retry;                                       /* Time to give up, in jiffies */
 	unsigned int retry_delay;                                       /* Delay length in ms */

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

Now it built ok, but for some reason kernel doen't like new module:
dmig@dmig-laptop:~/workspace/linux-source-2.6.29$ sudo insmod
drivers/hid/usbhid/usbhid.ko
insmod: error inserting 'drivers/hid/usbhid/usbhid.ko': -1 Invalid module format

Have any idea why?

2009/6/6 Anssi Hannula <anssi.hannula@gmail.com>:
>>>> Try the attached (untested) patch. It adds a simple timeout to the hid
>>>> ctrl and out urbs, discarding the faulty urb instead of getting stuck.
>>> And of course there was an error, attached is a fixed patch :)
>> Just tried to build module with your patch. It doesn't build.

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Alek Du]

Seems your module was not compiled against your running kernel. Are you using the right tree and config?

On Tue, 9 Jun 2009 13:02:45 +0800
Dmitriy Geels <dmitriy.geels@gmail.com> wrote:

> Now it built ok, but for some reason kernel doen't like new module:
> dmig@dmig-laptop:~/workspace/linux-source-2.6.29$ sudo insmod
> drivers/hid/usbhid/usbhid.ko
> insmod: error inserting 'drivers/hid/usbhid/usbhid.ko': -1 Invalid module format
> 
> Have any idea why?
> 
> 2009/6/6 Anssi Hannula <anssi.hannula@gmail.com>:
> >>>> Try the attached (untested) patch. It adds a simple timeout to the hid
> >>>> ctrl and out urbs, discarding the faulty urb instead of getting stuck.
> >>> And of course there was an error, attached is a fixed patch :)
> >> Just tried to build module with your patch. It doesn't build.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

As I understood, since some alpha, ubuntu packagers enabled module
versioning. I used old ubuntu alpha in virtualbox to test changes
(also I used my own built kernel there), now I tried to build module
for current running kernel.
Some discussions, I found about this error, tell that Module.symvers
is somehow causing it. Copying Module.symvers from current kernel
headers didn't help.
But I know that it is possible to build module -- dkms builds 3
modules successfully. I just didn't find out how to do this.

2009/6/9 Alek Du <alek.du@intel.com>:
> Seems your module was not compiled against your running kernel. Are you using the right tree and config?
>
> On Tue, 9 Jun 2009 13:02:45 +0800
> Dmitriy Geels <dmitriy.geels@gmail.com> wrote:
>
>> Now it built ok, but for some reason kernel doen't like new module:
>> dmig@dmig-laptop:~/workspace/linux-source-2.6.29$ sudo insmod
>> drivers/hid/usbhid/usbhid.ko
>> insmod: error inserting 'drivers/hid/usbhid/usbhid.ko': -1 Invalid module format
>>
>> Have any idea why?

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/6/9 Alek Du <alek.du@intel.com>:
> Seems your module was not compiled against your running kernel. Are you using the right tree and config?

Just found solution:
make -C /lib/modules/`uname -r`/build M=drivers/hid/usbhid/
instead of just make drivers/hid/usbhid/usbhid.ko

Will post test results later.

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/6/11 Dmitriy Geels <dmitriy.geels@gmail.com>:
> 2009/6/9 Alek Du <alek.du@intel.com>:
>> Seems your module was not compiled against your running kernel. Are you using the right tree and config?
>
> Just found solution:
> make -C /lib/modules/`uname -r`/build M=drivers/hid/usbhid/
> instead of just make drivers/hid/usbhid/usbhid.ko
>
> Will post test results later.
>

Hmm... first try failed. After running fftest, then ffmvforce got
system hang in virtual machine. X is completely unresponsive. May be
kernel is still alive, have to find out, how to press ctrl-alt-fx in
virtualbox.

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

2009/6/12 Dmitriy Geels <dmitriy.geels@gmail.com>:
> Hmm... first try failed. After running fftest, then ffmvforce got
> system hang in virtual machine. X is completely unresponsive. May be
> kernel is still alive, have to find out, how to press ctrl-alt-fx in
> virtualbox.

Bad... Can't get something out of driver with last patch. Running
ffmvforce after fftest/ffcstresstest hangs VM after few seconds (looks
like some timeout) with 100% cpu load.

May be it's possible to get logs via serial cable, but will they
contain something useful?

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Anssi Hannula]

Dmitriy Geels wrote:
> 2009/6/12 Dmitriy Geels <dmitriy.geels@gmail.com>:
>> Hmm... first try failed. After running fftest, then ffmvforce got
>> system hang in virtual machine. X is completely unresponsive. May be
>> kernel is still alive, have to find out, how to press ctrl-alt-fx in
>> virtualbox.

You can run "chvt 1" to switch to a virtual console and test there, so
you can see any kernel errors. IIRC "echo 9 > /proc/sysrq-trigger" makes
all kernel messages visible on console.

> Bad... Can't get something out of driver with last patch. Running
> ffmvforce after fftest/ffcstresstest hangs VM after few seconds (looks
> like some timeout) with 100% cpu load.

That looks like there is some bug in the code, then.
The last patch added a 8sec timeout after which the URB is canceled.

> May be it's possible to get logs via serial cable, but will they
> contain something useful?

Possibly. Alternatively you can do as above with virtualbox.

-- 
Anssi Hannula

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

Hello!

It took some time to get back to forcefeedback debugging for me.

2009/7/9 Anssi Hannula <anssi.hannula@gmail.com>:
> Dmitriy Geels wrote:
>> 2009/6/12 Dmitriy Geels <dmitriy.geels@gmail.com>:
>>> Hmm... first try failed. After running fftest, then ffmvforce got
>>> system hang in virtual machine. X is completely unresponsive. May be
>>> kernel is still alive, have to find out, how to press ctrl-alt-fx in
>>> virtualbox.
>
> You can run "chvt 1" to switch to a virtual console and test there, so
> you can see any kernel errors. IIRC "echo 9 > /proc/sysrq-trigger" makes
> all kernel messages visible on console.
>
>> Bad... Can't get something out of driver with last patch. Running
>> ffmvforce after fftest/ffcstresstest hangs VM after few seconds (looks
>> like some timeout) with 100% cpu load.
>
> That looks like there is some bug in the code, then.
> The last patch added a 8sec timeout after which the URB is canceled.
>
>> May be it's possible to get logs via serial cable, but will they
>> contain something useful?
>
> Possibly. Alternatively you can do as above with virtualbox.

Remember your last patches with hid_urb_timeout()?
I got kernel panic log after test: http://paste.org.ru/index.pl?fucaex
Looks like something bad happen inside usb_unlink_urb(hid_urb);

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

I found, what caused kernel crash: accessing urb data in call to
dev_warn(), because pointer usbhid->urbctrl (or urbout) is NULL.
Also, found several bugreports, telling that usb_unlink_urb() is
deprecated and usb_kill_urb() should be used. Second one used in other
places of hid-core.c, so I changed usb_unlink_urb() call to
usb_kill_urb().
Now kernel doesn't crash, and device gets stuck again.

Here is how hid_urb_timeout() looks now: http://paste.org.ru/index.pl?zcx5t1
And there is also part of kern.log, which I got:
http://paste.org.ru/index.pl?iyeyg7

Re: hid-pidff bug: fails to find all required reports of saitek gamepad

[Dmitriy Geels]

Hello!

My decision to replace usb_unlink_urb() with usb_kill_urb() was wrong.
After fixing usbhid->urbctrl initialization I got kernel crashes
again, and according to logs, it was crashing inside irq handling.
I changed that line back to usb_unlink_urb() and got working driver.

I found out reason of device/driver hangs: device doesn't like inplace
effect updates (for any changes effect should be deleted and
reuploaded). Inplace changes also work, but device becomes stuck after
closing and doesn't play anything from ffmvforce anymore until
reconnected.

This describes, why windows driver resets device on each open (report
52 01). Also windows driver sends enable/disable actuators on device
open/close.

Actually, it would be good to do reset and enable actuators on device
open and disable actuators on close, but that will break access of
several applications to one device.