| Message ID | alpine.LNX.2.00.1312161401100.12882@pobox.suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi On Mon, Dec 16, 2013 at 2:01 PM, Jiri Kosina <jkosina@suse.cz> wrote: > On Fri, 27 Sep 2013, Joseph Salisbury wrote: > >> >> commit b1a1442a23776756b254b69786848a94d92445ba >> >> Author: Jiri Kosina <jkosina@suse.cz> >> >> Date: Mon Jun 3 11:27:48 2013 +0200 >> >> >> >> HID: core: fix reporting of raw events >> >> >> >> Reverting this commit in v3.12-rc2 prevents the system from locking up, >> >> which happens when connecting a bluetooth trackpad. >> >> >> >> Jiri, do you think we should revert this patch, or is there some further >> >> debugging/data collecting you would like to do? >> > Hi Joseph, >> > >> > in this mail: >> > >> > Message-ID: <5241992E.3090805@canonical.com> >> > Date: Tue, 24 Sep 2013 09:52:46 -0400 >> > >> > you said that reverting this commit doesn't prevent the lockups, so I am >> > rather confused ... ? >> > >> > Thanks, >> > >> The testing was invalid. Reverting commit b1a1442 does resolve the bug >> and stop the lockups. > > Okay, I finally got some sense of this, sorry for the delay. > > Could you please test with the patch below, instead of reverting > b1a1442a23? Thanks a lot. > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 253fe23..81eacd3 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, > csize--; > } > > - rsize = ((report->size - 1) >> 3) + 1; > + rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7; Isn't "report->id" already covered by "if (report_enum->numbered)" above? The test for "id > 0" won't work here as in this case "report_enum->numbered" must already be set to true by the hid-desc parser, doesn't it? Where exactly did you get the +7 from? What worries me here is that we write over the @data buffer from the hid-ll-driver if the report is too short. I don't think the BT driver accounts for that, mhh. David > if (rsize > HID_MAX_BUFFER_SIZE) > rsize = HID_MAX_BUFFER_SIZE; > > -- > Jiri Kosina > SUSE Labs > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 19 Dec 2013, David Herrmann wrote: > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 253fe23..81eacd3 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, > > csize--; > > } > > > > - rsize = ((report->size - 1) >> 3) + 1; > > + rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7; > > Isn't "report->id" already covered by "if (report_enum->numbered)" > above? The test for "id > 0" won't work here as in this case > "report_enum->numbered" must already be set to true by the hid-desc > parser, doesn't it? Right, that one is not correct here, thanks. > Where exactly did you get the +7 from? Please see commit (the one I am not really proud of) 27ce405039bfe6d3.
Hi On Thu, Dec 19, 2013 at 10:59 AM, Jiri Kosina <jkosina@suse.cz> wrote: > On Thu, 19 Dec 2013, David Herrmann wrote: > >> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c >> > index 253fe23..81eacd3 100644 >> > --- a/drivers/hid/hid-core.c >> > +++ b/drivers/hid/hid-core.c >> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, >> > csize--; >> > } >> > >> > - rsize = ((report->size - 1) >> 3) + 1; >> > + rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7; >> >> Isn't "report->id" already covered by "if (report_enum->numbered)" >> above? The test for "id > 0" won't work here as in this case >> "report_enum->numbered" must already be set to true by the hid-desc >> parser, doesn't it? > > Right, that one is not correct here, thanks. > >> Where exactly did you get the +7 from? > > Please see commit (the one I am not really proud of) 27ce405039bfe6d3. Eh, I remember.. Ok, but the magic-mouse is BT right? So this commit really breaks BT drivers: commit b1a1442a23776756b254b69786848a94d92445ba Author: Jiri Kosina <jkosina@suse.cz> Date: Mon Jun 3 11:27:48 2013 +0200 HID: core: fix reporting of raw events Problem is, if the raw_event() callback returned 0 earlier, we just skipped raw input reports. However, we now always call the hid_report_raw_event() helper. Which is normally fine, but the helper expects the input buffer to be of size HID_MAX_REPORT_SIZE, which is *not* true for HIDP. So the memset() writes over some random memory. I don't know exactly how to fix it. HID_MAX_BUFFER_SIZE is too big to be allocated on the stack, but we're in atomic-context here so a kzalloc(rsize, GFP_ATOMIC) seems overkill. So I guess we'd have to look into HIDP to make the skb big enough, but I'm not sure how we can achieve that. So off the top of my head, the best idea is to add "char inbuf[HID_MAX_BUFFER_SIZE];" to the hidp_session object in HIDP and copy every input-report into the buffer before passing to hid_input_report(). Ideas? David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On Thu, Dec 19, 2013 at 11:08 AM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Thu, Dec 19, 2013 at 10:59 AM, Jiri Kosina <jkosina@suse.cz> wrote: >> On Thu, 19 Dec 2013, David Herrmann wrote: >> >>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c >>> > index 253fe23..81eacd3 100644 >>> > --- a/drivers/hid/hid-core.c >>> > +++ b/drivers/hid/hid-core.c >>> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, >>> > csize--; >>> > } >>> > >>> > - rsize = ((report->size - 1) >> 3) + 1; >>> > + rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7; >>> >>> Isn't "report->id" already covered by "if (report_enum->numbered)" >>> above? The test for "id > 0" won't work here as in this case >>> "report_enum->numbered" must already be set to true by the hid-desc >>> parser, doesn't it? >> >> Right, that one is not correct here, thanks. >> >>> Where exactly did you get the +7 from? >> >> Please see commit (the one I am not really proud of) 27ce405039bfe6d3. > > Eh, I remember.. Ok, but the magic-mouse is BT right? So this commit > really breaks BT drivers: > > commit b1a1442a23776756b254b69786848a94d92445ba > Author: Jiri Kosina <jkosina@suse.cz> > Date: Mon Jun 3 11:27:48 2013 +0200 > > HID: core: fix reporting of raw events > > Problem is, if the raw_event() callback returned 0 earlier, we just > skipped raw input reports. However, we now always call the > hid_report_raw_event() helper. Which is normally fine, but the helper > expects the input buffer to be of size HID_MAX_REPORT_SIZE, which is > *not* true for HIDP. So the memset() writes over some random memory. > > I don't know exactly how to fix it. HID_MAX_BUFFER_SIZE is too big to > be allocated on the stack, but we're in atomic-context here so a > kzalloc(rsize, GFP_ATOMIC) seems overkill. So I guess we'd have to > look into HIDP to make the skb big enough, but I'm not sure how we can > achieve that. > > So off the top of my head, the best idea is to add "char > inbuf[HID_MAX_BUFFER_SIZE];" to the hidp_session object in HIDP and > copy every input-report into the buffer before passing to > hid_input_report(). As this thread doesn't really contain any oops message nor the exact driver name (except mentioning hyperv and magicmouse), I fixed a related bug for HIDP: http://thread.gmane.org/gmane.linux.bluez.kernel/41969 A similar patch is *really* required for hid-hyperv.c as it also does not provide a properly sized buffer to hid_input_report(). David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 19 Dec 2013, David Herrmann wrote: > As this thread doesn't really contain any oops message nor the exact > driver name (except mentioning hyperv and magicmouse), FWIW I recall the oopses being present somewhere in the ubuntu bug tracker, referenced in this thread. Thanks,
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 253fe23..81eacd3 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, csize--; } - rsize = ((report->size - 1) >> 3) + 1; + rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7; if (rsize > HID_MAX_BUFFER_SIZE) rsize = HID_MAX_BUFFER_SIZE;