| Message ID | 20201119082232.8774-1-felixhaedicke@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: quirks: Add Apple Magic Trackpad 2 to hid_have_special_driver list | expand |
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.
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 >
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 --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) },
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