Re: [PATCH 1/6] HID: magicmouse: don't allow hidinput to initialize

[Chase Douglas <chase.douglas@canonical.com>]

On Tue, 2010-08-31 at 07:30 -0400, Michael Poole wrote:
> On Mon, 2010-08-30 at 23:46 -0400, Michael Poole wrote:
> > On Mon, 2010-08-30 at 13:20 -0400, Chase Douglas wrote:
> > > From: Chase Douglas <chase.douglas@ubuntu.com>
> > > 
> > > The driver listens only for raw events from the device. If we allow
> > > the hidinput layer to initialize, we can hit NULL pointer dereferences
> > > in the hidinput layer because disconnecting only removes the input
> > > devices from the hid device while leaving the hid fields around.
> > > 
> > > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > > ---
> > >  drivers/hid/hid-magicmouse.c |    6 ++----
> > >  1 files changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> > > index ee78787..2d8532d 100644
> > > --- a/drivers/hid/hid-magicmouse.c
> > > +++ b/drivers/hid/hid-magicmouse.c
> > > @@ -404,15 +404,13 @@ static int magicmouse_probe(struct hid_device *hdev,
> > >  		goto err_free;
> > >  	}
> > >  
> > > -	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > > +	/* we are handling the input ourselves */
> > > +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW | HID_CONNECT_HIDDEV);
> > >  	if (ret) {
> > >  		dev_err(&hdev->dev, "magicmouse hw start failed\n");
> > >  		goto err_free;
> > >  	}
> > >  
> > > -	/* we are handling the input ourselves */
> > > -	hidinput_disconnect(hdev);
> > > -
> > >  	report = hid_register_report(hdev, HID_INPUT_REPORT, TOUCH_REPORT_ID);
> > >  	if (!report) {
> > >  		dev_err(&hdev->dev, "unable to register touch report\n");
> > 
> > This effectively reverts commit 23d021167e.  Has the HID core changed so
> > that this won't cause problems when CONFIG_HIDRAW is disabled?
> 
> To answer my own question, it has not changed: If CONFIG_HIDRAW is
> turned off, the device will not get attached with this change, so the
> driver does not get any input to process.  Turning CONFIG_HIDRAW on
> restores the expected functionality.
> 
> Maybe hidinput_disconnect() should be modified instead, to clear the
> fields that were causing null pointer dereferences?

That may be correct, but I don't really know the hid layer well enough
to be sure.

I found that the hid-picolcd driver fakes out hid_hw_start by setting
the hdev->claimed field to HID_CLAIMED_INPUT before calling the
function. It then resets the bit after the function call. This ensures
that the device is initialized.

Even if the hidinput layer is fixed so it doesn't panic after a hidinput
device is disconnected, I like this change because it means we aren't
creating a device and then deleting it immediately. I think this is what
I noticed in my Xorg.0.log file. It's really strange to see a device pop
in and out.

I'll be sending a new set of patches with this change.

Thanks,

-- Chase