diff mbox series

HID: quirks: Add Apple Magic Trackpad 2 to hid_have_special_driver list

Message ID 20201119082232.8774-1-felixhaedicke@web.de
State New
Headers show
Series HID: quirks: Add Apple Magic Trackpad 2 to hid_have_special_driver list | expand

Commit Message

Felix Hädicke Nov. 19, 2020, 8:22 a.m. UTC 
The Apple Magic Trackpad 2 is handled by the magicmouse driver. And
there were severe stability issues when both drivers (hid-generic and
hid-magicmouse) were loaded for this device.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=210241

Signed-off-by: Felix Hädicke <felixhaedicke@web.de>
---
 drivers/hid/hid-quirks.c | 2 ++
 1 file changed, 2 insertions(+)

--
2.29.2

Comments

Dmitry Torokhov Dec. 2, 2020, 4:31 a.m. UTC | #1 
On Thu, Nov 19, 2020 at 12:31 AM Felix Hädicke <felixhaedicke@web.de> wrote:
>

> The Apple Magic Trackpad 2 is handled by the magicmouse driver. And

> there were severe stability issues when both drivers (hid-generic and

> hid-magicmouse) were loaded for this device.

>

> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=210241


+Jiri Kosina +Benjamin Tissoires for visibility.

>

> Signed-off-by: Felix Hädicke <felixhaedicke@web.de>

> ---

>  drivers/hid/hid-quirks.c | 2 ++

>  1 file changed, 2 insertions(+)

>

> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c

> index 7a2be0205dfd..0a589d956e5c 100644

> --- a/drivers/hid/hid-quirks.c

> +++ b/drivers/hid/hid-quirks.c

> @@ -473,6 +473,8 @@ static const struct hid_device_id hid_have_special_driver[] = {

>  #if IS_ENABLED(CONFIG_HID_MAGICMOUSE)

>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE) },

>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD) },

> +       { HID_BLUETOOTH_DEVICE(BT_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) },

> +       { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) },

>  #endif

>  #if IS_ENABLED(CONFIG_HID_MAYFLASH)

>         { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },

> --

> 2.29.2

>


Thanks.

-- 
Dmitry
Benjamin Tissoires Dec. 2, 2020, 10:21 a.m. UTC | #2 
Hi Felix,

On Wed, Dec 2, 2020 at 5:31 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>

> On Thu, Nov 19, 2020 at 12:31 AM Felix Hädicke <felixhaedicke@web.de> wrote:

> >

> > The Apple Magic Trackpad 2 is handled by the magicmouse driver. And

> > there were severe stability issues when both drivers (hid-generic and

> > hid-magicmouse) were loaded for this device.

> >

> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=210241


As mentioned in the bug, this hardly looks like the correct solution.

The magicmouse is one of the 2 only drivers that calls
`hid_register_report` and then overwrites the size of the report
manually. I can not figure out immediately if this is wrong, and how
that would impact a free in usbhid, but this is highly suspicious to
me.

Cheers,
Benjamin

>

> +Jiri Kosina +Benjamin Tissoires for visibility.

>

> >

> > Signed-off-by: Felix Hädicke <felixhaedicke@web.de>

> > ---

> >  drivers/hid/hid-quirks.c | 2 ++

> >  1 file changed, 2 insertions(+)

> >

> > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c

> > index 7a2be0205dfd..0a589d956e5c 100644

> > --- a/drivers/hid/hid-quirks.c

> > +++ b/drivers/hid/hid-quirks.c

> > @@ -473,6 +473,8 @@ static const struct hid_device_id hid_have_special_driver[] = {

> >  #if IS_ENABLED(CONFIG_HID_MAGICMOUSE)

> >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE) },

> >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD) },

> > +       { HID_BLUETOOTH_DEVICE(BT_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) },

> > +       { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) },

> >  #endif

> >  #if IS_ENABLED(CONFIG_HID_MAYFLASH)

> >         { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },

> > --

> > 2.29.2

> >

>

> Thanks.

>

> --

> Dmitry

>
Felix Hädicke April 5, 2021, 7:11 p.m. UTC | #3 
Hello Benjamin,

On Wed, 2020-12-02 at 11:21 +0100, Benjamin Tissoires wrote:
> Hi Felix,

>

> On Wed, Dec 2, 2020 at 5:31 AM Dmitry Torokhov

> <dmitry.torokhov@gmail.com> wrote:

> >

> > On Thu, Nov 19, 2020 at 12:31 AM Felix Hädicke <

> > felixhaedicke@web.de> wrote:

> > >

> > > The Apple Magic Trackpad 2 is handled by the magicmouse driver.

> > > And

> > > there were severe stability issues when both drivers (hid-generic

> > > and

> > > hid-magicmouse) were loaded for this device.

> > >

> > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=210241

>

> As mentioned in the bug, this hardly looks like the correct solution.

>

> The magicmouse is one of the 2 only drivers that calls

> `hid_register_report` and then overwrites the size of the report

> manually. I can not figure out immediately if this is wrong, and how

> that would impact a free in usbhid, but this is highly suspicious to

> me.


Sorry for my late reply.

Now I have now found time to investigate this further, and tried to
understand what effect overwriting the size of the report could have.
Without success, I found nothing about this, which could explain a
memory corruption.

But as I commented in bug 210241 (see comment #13), usbhid_stop() is
called twice for the same hid_device instance pointer. The first call
seems to happen when the HID default driver tries to initialise this
device, which failes (probably because the hid-magicmouse driver is
already active for this device).

Adding the Magic Trackpad 2 to the hid_have_special_driver list makes
hid_generic_match() return false (in the hid-generic driver), and the
device is ignored by this driver.

Is it not the right thing to have the Magic Trackpad 2 in the
hid_have_special_driver list anyway?

Regards,
Felix


>

> Cheers,

> Benjamin

>

> >

> > +Jiri Kosina +Benjamin Tissoires for visibility.

> >

> > >

> > > Signed-off-by: Felix Hädicke <felixhaedicke@web.de>

> > > ---

> > >  drivers/hid/hid-quirks.c | 2 ++

> > >  1 file changed, 2 insertions(+)

> > >

> > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c

> > > index 7a2be0205dfd..0a589d956e5c 100644

> > > --- a/drivers/hid/hid-quirks.c

> > > +++ b/drivers/hid/hid-quirks.c

> > > @@ -473,6 +473,8 @@ static const struct hid_device_id

> > > hid_have_special_driver[] = {

> > >  #if IS_ENABLED(CONFIG_HID_MAGICMOUSE)

> > >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,

> > > USB_DEVICE_ID_APPLE_MAGICMOUSE) },

> > >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,

> > > USB_DEVICE_ID_APPLE_MAGICTRACKPAD) },

> > > +       { HID_BLUETOOTH_DEVICE(BT_VENDOR_ID_APPLE,

> > > USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) },

> > > +       { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,

> > > USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) },

> > >  #endif

> > >  #if IS_ENABLED(CONFIG_HID_MAYFLASH)

> > >         { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE,

> > > USB_DEVICE_ID_DRAGONRISE_PS3) },

> > > --

> > > 2.29.2

> > >

> >

> > Thanks.

> >

> > --

> > Dmitry

> >

>
diff mbox series

Patch

diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 7a2be0205dfd..0a589d956e5c 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -473,6 +473,8 @@  static const struct hid_device_id hid_have_special_driver[] = {
 #if IS_ENABLED(CONFIG_HID_MAGICMOUSE)
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE) },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD) },
+	{ HID_BLUETOOTH_DEVICE(BT_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) },
 #endif
 #if IS_ENABLED(CONFIG_HID_MAYFLASH)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },