Re: [PATCH 0/3] HID: make raw output callback more flexible

[Bastien Nocera <hadess@hadess.net>]

On Tue, 2010-02-02 at 20:25 -0500, Michael Poole wrote:
> Bastien Nocera writes:
> 
> > On Fri, 2010-01-29 at 19:46 -0500, Michael Poole wrote:
> >> > [1]: Comments on the patch at
> >> > http://thread.gmane.org/gmane.linux.bluez.kernel/4279 would be
> >> > appreciated
> >> 
> >> This patch does not work for me.  Before, the first time after each
> >> boot
> >> that I tried to connect to an Apple Magic Mouse, it failed with -14
> >> (EFAULT).  With this patch, it fails with -22 (EINVAL) instead.  The
> >> -EFAULT *was* due to hidp_parse()'s copy_from_user().  I have not
> >> looked
> >> yet to see where the -EINVAL is coming from -- would that help?  (Both
> >> with and without your patch, the second attempt to connect works.) 
> >
> > I don't get -EFAULT anymore (it was failing to copy the rd_data from
> > user-space), but I do get -EINVALs now. I haven't investigated it
> > though. My guess is that the hid parser fails.
> >
> > Could you compare the sizes of the data gathered in user-space?
> 
> The bug mysteriously disappeared when I tried to apply kgdb to the
> problem.  Your patch won't do the trick because the hidp_connadd_req
> structure has also gone out of scope by the time the hid-(whatever)
> probe function is called.  The ioctl has returned to the user, but the
> new hid-(whatever) module is not yet loaded.

Right. I knew that rd_data would already be out of scope, didn't think
about the hidp_connadd_req structure itself.

I guess we should make a copy of the rd_data/rd_size in hidp_setup_hid()
instead.

> That is, the sequence looks like this:
> 
>   - Application triggers hidp_sock_ioctl(socket, HIDPCONNADD, &connadd).
>     - This starts the connection, and returns 0...
>     - ... but also indicates that some other module needs to be loaded.
> 
>   - User-space loads the appropriate module does init_module()...
>     - which calls the module_init() function...
>     - which calls hid_register_driver()...
>     - which eventually attaches the new driver to the device...
>     - triggering the module's probe() function to call hid_parse()...
>     - hid_parse() fails because its hidp_connadd_req is gone.
> 
> Marcel, I think this is a case where the subsystem maintainer should
> make the call on how to fix it.  I can write and locally test a patch,
> but I don't want to assume that (for example) the desired solution is to
> keep a copy of the Report descriptor in the hidp_session.

I would think it would be the cleanest way to do it. Quite a few members
of the hidp_connadd_req structure are already copied into the session in
hidp_setup_hid().

>   The USB HID
> core sends a GET_DESCRIPTOR request for the Report descriptor, which
> isn't practical here because that descriptor is only available through
> SDP.

And we only do SDP queries in user-space...

If you're not going to work on it shortly, I can take a look at cooking
a patch by the end of the week. Otherwise, I'd be happy testing your
patches.

Cheers