linuxconsole-dev

HID issues.

[Bj|rn A. <d3a...@dt...>]

Hello, folks!

Looking thru the code, I found a bunch of things that I think someone
should have a look at.

* There's a race in the HID init code. What happens is

send for all input reports;
send for all feature reports;
block until all feature reports are in;
configure device based on input & feature report data. (hid_init_reports)

Some input reports might not have arrived yet when we call
hid_init_reports.

(And yes, this actually happens in real life.)


* When you do hid_set_field, there's code that checks the value against
logical max/min for that field. That's useful information, but you're
supposed to be able to set a value outside of that interval - a
Null Value, it's called in the HID spec (5.10 in the spec). 
We should probably warn if the value is out of the interval, but only 
error if it won't actually fit in the field. 
Ie: settting an 8-bit field with logical min = 1 and logical 
max = 40 to 0xFF should probably warn (probably only if DEBUG), but 
it should be allowed.


* Why hasn't the hid_report struct got a usage field in it?
This makes it difficult to find a certain report by walking the tree,
and worse, information is lost, at least when we parse arrays:

This is a "PID Block Load Report", as reported by lsusb:
(Yeah, I know, but this was the shortest one that shows this...)

Note how the "Block Load {Success,Full,Error}" fields
have three layers of usages:
* The report usage (PID Block Load Report)
* The logical collection (Block Load Status)
* The field's actual usage.

    Item(Global): Usage Page, data= [ 0x0f ] 15
                    PID Page
    Item(Local ): Usage, data= [ 0x89 ] 137
                    PID Block Load Report
    Item(Main  ): Collection, data= [ 0x02 ] 2
                    Logical
        Item(Global): Report ID, data= [ 0x02 ] 2 (feature)
        Item(Local ): Usage, data= [ 0x22 ] 34
                        Effect Block Index
        Item(Global): Logical Maximum, data= [ 0x28 ] 40
        Item(Global): Logical Minimum, data= [ 0x01 ] 1
        Item(Global): Physical Minimum, data= [ 0x01 ] 1
        Item(Global): Physical Maximum, data= [ 0x28 ] 40
        Item(Global): Report Size, data= [ 0x08 ] 8
        Item(Global): Report Count, data= [ 0x01 ] 1
        Item(Main  ): Feature, data= [ 0x02 ] 2
                    Data Variable Absolute No_Wrap Linear
                    Preferred_State No_Null_Position Non_Volatile Bitfield
        Item(Local ): Usage, data= [ 0x8b ] 139
                    Block Load Status
        Item(Main  ): Collection, data= [ 0x02 ] 2
                        Logical
            Item(Local ): Usage, data= [ 0x8c ] 140
                            Block Load Success
            Item(Local ): Usage, data= [ 0x8d ] 141
                            Block Load Full
            Item(Local ): Usage, data= [ 0x8e ] 142
                            Block Load Error
            Item(Global): Logical Maximum, data= [ 0x03 ] 3
            Item(Global): Logical Minimum, data= [ 0x01 ] 1
            Item(Global): Physical Minimum, data= [ 0x01 ] 1
            Item(Global): Physical Maximum, data= [ 0x03 ] 3
            Item(Global): Report Size, data= [ 0x08 ] 8
            Item(Global): Report Count, data= [ 0x01 ] 1
            Item(Main  ): Feature, data= [ 0x00 ] 0
                            Data Array Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
        Item(Main  ): End Collection, data=none
        Item(Local ): Usage, data= [ 0xac ] 172
                        RAM Pool Available
        Item(Global): Logical Minimum, data= [ 0x00 ] 0
        Item(Global): Logical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
        Item(Global): Physical Minimum, data= [ 0x00 ] 0
        Item(Global): Physical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
        Item(Global): Report Size, data= [ 0x10 ] 16
        Item(Global): Report Count, data= [ 0x01 ] 1
        Item(Main  ): Feature, data= [ 0x00 ] 0
                        Data Array Absolute No_Wrap Linear
                        Preferred_State No_Null_Position Non_Volatile Bitfield
    Item(Main  ): End Collection, data=none

This is how the kernel parses it:

localhost kernel:   FEATURE(2)[FEATURE]
localhost kernel:     Field(0)
localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
localhost kernel:       Usage(1)
localhost kernel:         PhysicalInterfaceDevice.Effect_Block_Index
localhost kernel:       Logical Minimum(1)
localhost kernel:       Logical Maximum(40)
localhost kernel:       Physical Minimum(1)
localhost kernel:       Physical Maximum(40)
localhost kernel:       Report Size(8)
localhost kernel:       Report Count(1)
localhost kernel:       Report Offset(0)
localhost kernel:       Flags( Variable Absolute )
localhost kernel:     Field(1)
localhost kernel:       Logical(PhysicalInterfaceDevice.Block_Load_Status)
localhost kernel:       Usage(3)
localhost kernel:         PhysicalInterfaceDevice.Block_Load_Success
localhost kernel:         PhysicalInterfaceDevice.Block_Load_Full
localhost kernel:         PhysicalInterfaceDevice.Block_Load_Error
localhost kernel:       Logical Minimum(1)
localhost kernel:       Logical Maximum(3)
localhost kernel:       Physical Minimum(1)
localhost kernel:       Physical Maximum(3)
localhost kernel:       Report Size(8)
localhost kernel:       Report Count(1)
localhost kernel:       Report Offset(8)
localhost kernel:       Flags( Array Absolute )
localhost kernel:     Field(2)
localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
localhost kernel:       Usage(1)
localhost kernel:         PhysicalInterfaceDevice.RAM_Pool_Available
localhost kernel:       Logical Minimum(0)
localhost kernel:       Logical Maximum(65535)
localhost kernel:       Physical Minimum(0)
localhost kernel:       Physical Maximum(65535)
localhost kernel:       Report Size(16)
localhost kernel:       Report Count(1)
localhost kernel:       Report Offset(16)
localhost kernel:       Flags( Array Absolute )

Presently, my strategy for finding a specific report is to walk the
tree, looking for a field with a Logical Usage that's the same as the
report Usage I'm looking for, and then return the report pointer from
that field.

This happens to work for my device, since there's always some field in
every report that isn't an array, but that's not neccesarily true for
all devices.

In other words: If we had a report with all array fields, we wouldn't
know which usage that report has/represents.


-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: HID issues.

[Vojtech P. <vo...@su...>]

On Thu, Jan 24, 2002 at 10:33:02PM +0100, Bj|rn Augustsson wrote:
> 
> Hello, folks!
> 
> Looking thru the code, I found a bunch of things that I think someone
> should have a look at.
> 
> * There's a race in the HID init code. What happens is
> 
> send for all input reports;
> send for all feature reports;
> block until all feature reports are in;
> configure device based on input & feature report data. (hid_init_reports)
> 
> Some input reports might not have arrived yet when we call
> hid_init_reports.
> 
> (And yes, this actually happens in real life.)

Ugh? I don't think I understand you here fully. hid_init_reports() is
the function that makes all the request to get the input and feature
data. So *no* reports come before it is called. And it waits for all the
report to arrive then - it waits till the queue is empty. Maybe we are
each reading a different version? I'm looking at the current CVS.

> * When you do hid_set_field, there's code that checks the value against
> logical max/min for that field. That's useful information, but you're
> supposed to be able to set a value outside of that interval - a
> Null Value, it's called in the HID spec (5.10 in the spec). 
> We should probably warn if the value is out of the interval, but only 
> error if it won't actually fit in the field. 
> Ie: settting an 8-bit field with logical min = 1 and logical 
> max = 40 to 0xFF should probably warn (probably only if DEBUG), but 
> it should be allowed.

Correct, feel free to implement a better fix, I'll just remove the check
for now.

> * Why hasn't the hid_report struct got a usage field in it?
> This makes it difficult to find a certain report by walking the tree,
> and worse, information is lost, at least when we parse arrays:

Because it wasn't needed by anything yet. All decisions are made based
on the field usage data for the input-only devices and it is enough. So,
if you need it, add it.

> This is a "PID Block Load Report", as reported by lsusb:
> (Yeah, I know, but this was the shortest one that shows this...)
> 
> Note how the "Block Load {Success,Full,Error}" fields
> have three layers of usages:
> * The report usage (PID Block Load Report)
> * The logical collection (Block Load Status)
> * The field's actual usage.
> 
>     Item(Global): Usage Page, data= [ 0x0f ] 15
>                     PID Page
>     Item(Local ): Usage, data= [ 0x89 ] 137
>                     PID Block Load Report
>     Item(Main  ): Collection, data= [ 0x02 ] 2
>                     Logical
>         Item(Global): Report ID, data= [ 0x02 ] 2 (feature)
>         Item(Local ): Usage, data= [ 0x22 ] 34
>                         Effect Block Index
>         Item(Global): Logical Maximum, data= [ 0x28 ] 40
>         Item(Global): Logical Minimum, data= [ 0x01 ] 1
>         Item(Global): Physical Minimum, data= [ 0x01 ] 1
>         Item(Global): Physical Maximum, data= [ 0x28 ] 40
>         Item(Global): Report Size, data= [ 0x08 ] 8
>         Item(Global): Report Count, data= [ 0x01 ] 1
>         Item(Main  ): Feature, data= [ 0x02 ] 2
>                     Data Variable Absolute No_Wrap Linear
>                     Preferred_State No_Null_Position Non_Volatile Bitfield
>         Item(Local ): Usage, data= [ 0x8b ] 139
>                     Block Load Status
>         Item(Main  ): Collection, data= [ 0x02 ] 2
>                         Logical
>             Item(Local ): Usage, data= [ 0x8c ] 140
>                             Block Load Success
>             Item(Local ): Usage, data= [ 0x8d ] 141
>                             Block Load Full
>             Item(Local ): Usage, data= [ 0x8e ] 142
>                             Block Load Error
>             Item(Global): Logical Maximum, data= [ 0x03 ] 3
>             Item(Global): Logical Minimum, data= [ 0x01 ] 1
>             Item(Global): Physical Minimum, data= [ 0x01 ] 1
>             Item(Global): Physical Maximum, data= [ 0x03 ] 3
>             Item(Global): Report Size, data= [ 0x08 ] 8
>             Item(Global): Report Count, data= [ 0x01 ] 1
>             Item(Main  ): Feature, data= [ 0x00 ] 0
>                             Data Array Absolute No_Wrap Linear
>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>         Item(Main  ): End Collection, data=none
>         Item(Local ): Usage, data= [ 0xac ] 172
>                         RAM Pool Available
>         Item(Global): Logical Minimum, data= [ 0x00 ] 0
>         Item(Global): Logical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
>         Item(Global): Physical Minimum, data= [ 0x00 ] 0
>         Item(Global): Physical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
>         Item(Global): Report Size, data= [ 0x10 ] 16
>         Item(Global): Report Count, data= [ 0x01 ] 1
>         Item(Main  ): Feature, data= [ 0x00 ] 0
>                         Data Array Absolute No_Wrap Linear
>                         Preferred_State No_Null_Position Non_Volatile Bitfield
>     Item(Main  ): End Collection, data=none
> 
> This is how the kernel parses it:
> 
> localhost kernel:   FEATURE(2)[FEATURE]
> localhost kernel:     Field(0)
> localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
> localhost kernel:       Usage(1)
> localhost kernel:         PhysicalInterfaceDevice.Effect_Block_Index
> localhost kernel:       Logical Minimum(1)
> localhost kernel:       Logical Maximum(40)
> localhost kernel:       Physical Minimum(1)
> localhost kernel:       Physical Maximum(40)
> localhost kernel:       Report Size(8)
> localhost kernel:       Report Count(1)
> localhost kernel:       Report Offset(0)
> localhost kernel:       Flags( Variable Absolute )
> localhost kernel:     Field(1)
> localhost kernel:       Logical(PhysicalInterfaceDevice.Block_Load_Status)
> localhost kernel:       Usage(3)
> localhost kernel:         PhysicalInterfaceDevice.Block_Load_Success
> localhost kernel:         PhysicalInterfaceDevice.Block_Load_Full
> localhost kernel:         PhysicalInterfaceDevice.Block_Load_Error
> localhost kernel:       Logical Minimum(1)
> localhost kernel:       Logical Maximum(3)
> localhost kernel:       Physical Minimum(1)
> localhost kernel:       Physical Maximum(3)
> localhost kernel:       Report Size(8)
> localhost kernel:       Report Count(1)
> localhost kernel:       Report Offset(8)
> localhost kernel:       Flags( Array Absolute )
> localhost kernel:     Field(2)
> localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
> localhost kernel:       Usage(1)
> localhost kernel:         PhysicalInterfaceDevice.RAM_Pool_Available
> localhost kernel:       Logical Minimum(0)
> localhost kernel:       Logical Maximum(65535)
> localhost kernel:       Physical Minimum(0)
> localhost kernel:       Physical Maximum(65535)
> localhost kernel:       Report Size(16)
> localhost kernel:       Report Count(1)
> localhost kernel:       Report Offset(16)
> localhost kernel:       Flags( Array Absolute )
> 
> Presently, my strategy for finding a specific report is to walk the
> tree, looking for a field with a Logical Usage that's the same as the
> report Usage I'm looking for, and then return the report pointer from
> that field.
> 
> This happens to work for my device, since there's always some field in
> every report that isn't an array, but that's not neccesarily true for
> all devices.
> 
> In other words: If we had a report with all array fields, we wouldn't
> know which usage that report has/represents.

-- 
Vojtech Pavlik
SuSE Labs

Re: HID issues.

[Bj|rn A. <d3a...@dt...>]

Quoting Vojtech Pavlik <vo...@su...>:
> On Thu, Jan 24, 2002 at 10:33:02PM +0100, Bj|rn Augustsson wrote:
> > 
> > Hello, folks!
> > 
> > Looking thru the code, I found a bunch of things that I think someone
> > should have a look at.
> > 
> > * There's a race in the HID init code. What happens is
> > 
> > send for all input reports;
> > send for all feature reports;
> > block until all feature reports are in;
> > configure device based on input & feature report data. (hid_init_reports)
> > 
> > Some input reports might not have arrived yet when we call
> > hid_init_reports.
> > 
> > (And yes, this actually happens in real life.)
> 
> Ugh? I don't think I understand you here fully. hid_init_reports() is
> the function that makes all the request to get the input and feature
> data. So *no* reports come before it is called. 

Quite. My mistake. 

> > Some input reports might not have arrived yet when we call
> > hid_init_reports.

Should have been

Some input reports might not have arrived yet when 
hid_init_reports returns.

This means that we call hidinput_connect() before we have all the 
data.

Sorry for the confusion.

> And it waits for all the
> report to arrive then - it waits till the queue is empty. Maybe we are
> each reading a different version? I'm looking at the current CVS.

Well, I'm not, but fairly close (CVS version tag 1.35).

The way I read the code, it waits for all feature reports to come in.
(On the control pipe.)
All the input reports aren't neccessarily in yet though. (On the input
interrupt pipe.)

Maybe there's some interaction here that I don't see/know about, but
that's what it looks like to me.

> > * When you do hid_set_field, there's code that checks the value against
> > logical max/min for that field. That's useful information, but you're
> > supposed to be able to set a value outside of that interval - a
> > Null Value, it's called in the HID spec (5.10 in the spec). 
> > We should probably warn if the value is out of the interval, but only 
> > error if it won't actually fit in the field. 
> > Ie: settting an 8-bit field with logical min = 1 and logical 
> > max = 40 to 0xFF should probably warn (probably only if DEBUG), but 
> > it should be allowed.
> 
> Correct, feel free to implement a better fix, I'll just remove the check
> for now.

Right. This is my next sub-project then. 

> > * Why hasn't the hid_report struct got a usage field in it?
> > This makes it difficult to find a certain report by walking the tree,
> > and worse, information is lost, at least when we parse arrays:
> 
> Because it wasn't needed by anything yet. All decisions are made based
> on the field usage data for the input-only devices and it is enough. So,
> if you need it, add it.

Will do.

/August.
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: HID issues.

[Vojtech P. <vo...@su...>]

On Fri, Jan 25, 2002 at 02:03:34AM +0100, Bj|rn Augustsson wrote:
> Quoting Vojtech Pavlik <vo...@su...>:
> > On Thu, Jan 24, 2002 at 10:33:02PM +0100, Bj|rn Augustsson wrote:
> > > 
> > > Hello, folks!
> > > 
> > > Looking thru the code, I found a bunch of things that I think someone
> > > should have a look at.
> > > 
> > > * There's a race in the HID init code. What happens is
> > > 
> > > send for all input reports;
> > > send for all feature reports;
> > > block until all feature reports are in;
> > > configure device based on input & feature report data. (hid_init_reports)
> > > 
> > > Some input reports might not have arrived yet when we call
> > > hid_init_reports.
> > > 
> > > (And yes, this actually happens in real life.)
> > 
> > Ugh? I don't think I understand you here fully. hid_init_reports() is
> > the function that makes all the request to get the input and feature
> > data. So *no* reports come before it is called. 
> 
> Quite. My mistake. 
> 
> > > Some input reports might not have arrived yet when we call
> > > hid_init_reports.
> 
> Should have been
> 
> Some input reports might not have arrived yet when 
> hid_init_reports returns.
> 
> This means that we call hidinput_connect() before we have all the 
> data.
> 
> Sorry for the confusion.

You mean when we time out? Well, yes, then we have a problem. But the
timeout is the error case which shouldn't happen. Otherwise, I don't see
how any pending 'get report' requests could stay in the queue - we wait
for both the control and out queue to clear completely. 

> > And it waits for all the
> > report to arrive then - it waits till the queue is empty. Maybe we are
> > each reading a different version? I'm looking at the current CVS.
> 
> Well, I'm not, but fairly close (CVS version tag 1.35).

Maybe update, there was a serious rewrite of that part.

> The way I read the code, it waits for all feature reports to come in.
> (On the control pipe.)
> All the input reports aren't neccessarily in yet though. (On the input
> interrupt pipe.)
> 
> Maybe there's some interaction here that I don't see/know about, but
> that's what it looks like to me.

Both input and feature reports come over the control pipe. The input
interrupt pipe isn't open at that time yet, and thus no reports are
coming over it.

> > > * When you do hid_set_field, there's code that checks the value against
> > > logical max/min for that field. That's useful information, but you're
> > > supposed to be able to set a value outside of that interval - a
> > > Null Value, it's called in the HID spec (5.10 in the spec). 
> > > We should probably warn if the value is out of the interval, but only 
> > > error if it won't actually fit in the field. 
> > > Ie: settting an 8-bit field with logical min = 1 and logical 
> > > max = 40 to 0xFF should probably warn (probably only if DEBUG), but 
> > > it should be allowed.
> > 
> > Correct, feel free to implement a better fix, I'll just remove the check
> > for now.
> 
> Right. This is my next sub-project then. 
> 
> > > * Why hasn't the hid_report struct got a usage field in it?
> > > This makes it difficult to find a certain report by walking the tree,
> > > and worse, information is lost, at least when we parse arrays:
> > 
> > Because it wasn't needed by anything yet. All decisions are made based
> > on the field usage data for the input-only devices and it is enough. So,
> > if you need it, add it.
> 
> Will do.
> 
> /August.
> -- 
> Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
> being a pointer to const Foo.  const Foo const *foo; would mean the same but is
> illegal (double const).  You are confusing this with Foo * const foo; and const
> Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

-- 
Vojtech Pavlik
SuSE Labs

Re: HID issues.

[Bj|rn A. <d3a...@dt...>]

Quoting Vojtech Pavlik <vo...@su...>:
> On Fri, Jan 25, 2002 at 02:03:34AM +0100, Bj|rn Augustsson wrote:
> > Sorry for the confusion.
> 
> You mean when we time out? Well, yes, then we have a problem. But the
> timeout is the error case which shouldn't happen. Otherwise, I don't see
> how any pending 'get report' requests could stay in the queue - we wait
> for both the control and out queue to clear completely. 

Don't really know, but I do get output from the "hid_dump_input"
function after hidinput_connect() has been called (ie too late) every
now and then, and there's definetly something weird going on there,
because sometimes I get weird data in them as well, as in

Jan 17 20:14:39 as22-3-7 kernel: hid-debug: input 6e69.4c20 = 1

Or some other random-looking value. Oh well.

> > > And it waits for all the
> > > report to arrive then - it waits till the queue is empty. Maybe we are
> > > each reading a different version? I'm looking at the current CVS.
> > 
> > Well, I'm not, but fairly close (CVS version tag 1.35).
> 
> Maybe update, there was a serious rewrite of that part.

I'm trying! :)

I haven't gotten anything since that to build and boot, that's all...
 
> Both input and feature reports come over the control pipe. The input
> interrupt pipe isn't open at that time yet, and thus no reports are
> coming over it.

Ah, OK. Well, that sorts that out. The problem must be somewhere else
then.

/August.
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: HID issues.

[Vojtech P. <vo...@su...>]

On Sat, Jan 26, 2002 at 08:13:26PM +0100, Bj|rn Augustsson wrote:
> Quoting Vojtech Pavlik <vo...@su...>:
> > On Fri, Jan 25, 2002 at 02:03:34AM +0100, Bj|rn Augustsson wrote:
> > > Sorry for the confusion.
> > 
> > You mean when we time out? Well, yes, then we have a problem. But the
> > timeout is the error case which shouldn't happen. Otherwise, I don't see
> > how any pending 'get report' requests could stay in the queue - we wait
> > for both the control and out queue to clear completely. 
> 
> Don't really know, but I do get output from the "hid_dump_input"
> function after hidinput_connect() has been called (ie too late) every
> now and then, and there's definetly something weird going on there,
> because sometimes I get weird data in them as well, as in
> 
> Jan 17 20:14:39 as22-3-7 kernel: hid-debug: input 6e69.4c20 = 1
> 
> Or some other random-looking value. Oh well.
> 
> > > > And it waits for all the
> > > > report to arrive then - it waits till the queue is empty. Maybe we are
> > > > each reading a different version? I'm looking at the current CVS.
> > > 
> > > Well, I'm not, but fairly close (CVS version tag 1.35).
> > 
> > Maybe update, there was a serious rewrite of that part.
> 
> I'm trying! :)
> 
> I haven't gotten anything since that to build and boot, that's all...

Use 2.5.2-dj6 if you can, that's probably the easiest (and working as
well).

> > Both input and feature reports come over the control pipe. The input
> > interrupt pipe isn't open at that time yet, and thus no reports are
> > coming over it.
> 
> Ah, OK. Well, that sorts that out. The problem must be somewhere else
> then.

If you find any possible reason for it, tell me, I'll take a look.

-- 
Vojtech Pavlik
SuSE Labs

Re: HID issues. [ with miniPATCH ]

[Bj|rn A. <d3a...@dt...>]

Quoting Vojtech Pavlik <vo...@su...>:
> On Sat, Jan 26, 2002 at 08:13:26PM +0100, Bj|rn Augustsson wrote:
> > Quoting Vojtech Pavlik <vo...@su...>:
> > > 
> > > Maybe update, there was a serious rewrite of that part.
> > 
> > I'm trying! :)
> > 
> > I haven't gotten anything since that to build and boot, that's all...
> 
> Use 2.5.2-dj6 if you can, that's probably the easiest (and working as
> well).

Good advice, it turns out. A few hours later, I have a working (?)
kernel, by getting dj6 and then replacing _only_ the
linux/drivers/usb/hid* files with the stuff from the CVS. 

Another question!

How come you set the output interrupt pipe to be a bulk pipe? That seems 
obviously wrong, and it wasn't that way in my patch. 
Do you know something I don't?  
(It had a "/* FIXME should we use sndint here? */" after it, which was 
removed by 1.39)

I've attached a mini-patch as well, that fixes another thing. We only
want to send outbound output reports over the output pipe - feature
reports still go over the control pipe.

/August.
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: HID issues. [ with miniPATCH ]

[Vojtech P. <vo...@su...>]

On Sat, Jan 26, 2002 at 11:55:51PM +0100, Bj|rn Augustsson wrote:
> Quoting Vojtech Pavlik <vo...@su...>:
> > On Sat, Jan 26, 2002 at 08:13:26PM +0100, Bj|rn Augustsson wrote:
> > > Quoting Vojtech Pavlik <vo...@su...>:
> > > > 
> > > > Maybe update, there was a serious rewrite of that part.
> > > 
> > > I'm trying! :)
> > > 
> > > I haven't gotten anything since that to build and boot, that's all...
> > 
> > Use 2.5.2-dj6 if you can, that's probably the easiest (and working as
> > well).
> 
> Good advice, it turns out. A few hours later, I have a working (?)
> kernel, by getting dj6 and then replacing _only_ the
> linux/drivers/usb/hid* files with the stuff from the CVS. 
> 
> Another question!
> 
> How come you set the output interrupt pipe to be a bulk pipe? That seems 
> obviously wrong, and it wasn't that way in my patch. 

I'll have to research this further, but for me it was the only working
way to generate an one-shot out interrupt. Actually it's the same on the
USB bus. But I agree that it's non-obvious and that there may be a
better way to do it.

> Do you know something I don't?  
> (It had a "/* FIXME should we use sndint here? */" after it, which was 
> removed by 1.39)

See above. You can try with the sndintpipe - if it works, OK. I can't
check with any HID device right now. Perhaps Johann Deneux can check
with I-Force - there is also bulk used instead of int and it works OK.

> I've attached a mini-patch as well, that fixes another thing. We only
> want to send outbound output reports over the output pipe - feature
> reports still go over the control pipe.

Ahh, correct. Thanks. In the CVS now.

> 
> /August.
> -- 
> Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
> being a pointer to const Foo.  const Foo const *foo; would mean the same but is
> illegal (double const).  You are confusing this with Foo * const foo; and const
> Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

> --- /usr/src/joy/ruby/linux/drivers/usb/hid-core.c	Sat Jan 26 21:08:47 2002
> +++ hid-core.c	Sat Jan 26 23:50:53 2002
> @@ -1093,7 +1093,7 @@
>  	int head;
>  	unsigned long flags;
>  
> -	if (dir == USB_DIR_OUT && hid->urbout) {
> +	if (dir == USB_DIR_OUT && hid->urbout && report->type == HID_OUTPUT_REPORT) {
>  
>  		spin_lock_irqsave(&hid->outlock, flags);
>  


-- 
Vojtech Pavlik
SuSE Labs

Re: HID issues. [ with miniPATCH ]

[Johann D. <jo...@Do...>]

On Sun, 27 Jan 2002, Vojtech Pavlik wrote:

> On Sat, Jan 26, 2002 at 11:55:51PM +0100, Bj|rn Augustsson wrote:
> > 
> > How come you set the output interrupt pipe to be a bulk pipe? That seems 
> > obviously wrong, and it wasn't that way in my patch. 
> 
> I'll have to research this further, but for me it was the only working
> way to generate an one-shot out interrupt. Actually it's the same on the
> USB bus. But I agree that it's non-obvious and that there may be a
> better way to do it.
> 
> > Do you know something I don't?  
> > (It had a "/* FIXME should we use sndint here? */" after it, which was 
> > removed by 1.39)
> 
> See above. You can try with the sndintpipe - if it works, OK. I can't
> check with any HID device right now. Perhaps Johann Deneux can check
> with I-Force - there is also bulk used instead of int and it works OK.

I tried to replace:
FILL_BULK_URB(&iforce->out, dev, usb_sndbulkpipe(dev,  
epout->bEndpointAddress), iforce + 1, 32, iforce_usb_out, iforce);

by:
FILL_INT_URB(&iforce->out, dev, usb_sndintpipe(dev,
epout->bEndpointAddress), iforce + 1, 32, iforce_usb_out, iforce, 0);

It did not work. Maybe there are other changes to do, but I do not see
which ones. The last parameter should tell the driver to use one-shot out
interrupts, which I guess is what we want.

-- 
Johann Deneux