On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
> Reverting the patch changes the driver back to useing kzalloc() and
> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
> causes an oops on memcpy()[1]. We are in the process of bisecting that
> one now and will provide the results.
The two bugs are the same it's that the code has shifted a little. Mark
the commit as buggy and continue with the git bisect.
regards,
dan carpenter
On 09/16/2013 04:38 PM, Dan Carpenter wrote:
> On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
>> Reverting the patch changes the driver back to useing kzalloc() and
>> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
>> causes an oops on memcpy()[1]. We are in the process of bisecting that
>> one now and will provide the results.
> The two bugs are the same it's that the code has shifted a little. Mark
> the commit as buggy and continue with the git bisect.
>
> regards,
> dan carpenter
Can you explain a little further? Mark commit a4a23f6 as bad? An
initial bisect already reported that was the first bad commit, so it
can't be marked bad. The oops on memcpy() happens after commit a4a23f6
is reverted. The oops on memcpy() did not happen before a4a23f6 was
committed, so I assume this new oops was introduced by a change later.
Right now I'm bisecting down the oops on memcpy() by updating the bisect
with good or bad, depending if the test kernel hit the oops. I then
revert a4a23f6, so that revert is the HEAD of the tree each time before
building the kernel again(As long as the commit spit out by bisect is
after when a4a23f6 was introduced).
Thanks,
Joe
On Mon, Sep 16, 2013 at 04:49:29PM -0400, Joseph Salisbury wrote:
> On 09/16/2013 04:38 PM, Dan Carpenter wrote:
> > On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
> >> Reverting the patch changes the driver back to useing kzalloc() and
> >> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
> >> causes an oops on memcpy()[1]. We are in the process of bisecting that
> >> one now and will provide the results.
> > The two bugs are the same it's that the code has shifted a little. Mark
> > the commit as buggy and continue with the git bisect.
> >
> > regards,
> > dan carpenter
> Can you explain a little further? Mark commit a4a23f6 as bad? An
> initial bisect already reported that was the first bad commit, so it
> can't be marked bad. The oops on memcpy() happens after commit a4a23f6
> is reverted. The oops on memcpy() did not happen before a4a23f6 was
> committed, so I assume this new oops was introduced by a change later.
>
> Right now I'm bisecting down the oops on memcpy() by updating the bisect
> with good or bad, depending if the test kernel hit the oops. I then
> revert a4a23f6, so that revert is the HEAD of the tree each time before
> building the kernel again(As long as the commit spit out by bisect is
> after when a4a23f6 was introduced).
Yep. Please continue bisecting the memcpy() oops.
kmemdup() is just a kzalloc() followed by a memcpy(). When we split it
apart by reverting the patch then we would expect the oops to move to
the memcpy() part. Somehow "desc" is a bogus pointer, but I don't
immediately see how that is possible.
regards,
dan carpenter
On 09/16/2013 05:05 PM, Dan Carpenter wrote:
> On Mon, Sep 16, 2013 at 04:49:29PM -0400, Joseph Salisbury wrote:
>> On 09/16/2013 04:38 PM, Dan Carpenter wrote:
>>> On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
>>>> Reverting the patch changes the driver back to useing kzalloc() and
>>>> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
>>>> causes an oops on memcpy()[1]. We are in the process of bisecting that
>>>> one now and will provide the results.
>>> The two bugs are the same it's that the code has shifted a little. Mark
>>> the commit as buggy and continue with the git bisect.
>>>
>>> regards,
>>> dan carpenter
>> Can you explain a little further? Mark commit a4a23f6 as bad? An
>> initial bisect already reported that was the first bad commit, so it
>> can't be marked bad. The oops on memcpy() happens after commit a4a23f6
>> is reverted. The oops on memcpy() did not happen before a4a23f6 was
>> committed, so I assume this new oops was introduced by a change later.
>>
>> Right now I'm bisecting down the oops on memcpy() by updating the bisect
>> with good or bad, depending if the test kernel hit the oops. I then
>> revert a4a23f6, so that revert is the HEAD of the tree each time before
>> building the kernel again(As long as the commit spit out by bisect is
>> after when a4a23f6 was introduced).
> Yep. Please continue bisecting the memcpy() oops.
>
> kmemdup() is just a kzalloc() followed by a memcpy(). When we split it
> apart by reverting the patch then we would expect the oops to move to
> the memcpy() part. Somehow "desc" is a bogus pointer, but I don't
> immediately see how that is possible.
>
> regards,
> dan carpenter
Thanks for the details. We'll continue the bisect and let you know how
it goes.
Thanks again,
Joe
On Mon, 16 Sep 2013, Joseph Salisbury wrote:
> >> Can you explain a little further? Mark commit a4a23f6 as bad? An
> >> initial bisect already reported that was the first bad commit, so it
> >> can't be marked bad. The oops on memcpy() happens after commit a4a23f6
> >> is reverted. The oops on memcpy() did not happen before a4a23f6 was
> >> committed, so I assume this new oops was introduced by a change later.
> >>
> >> Right now I'm bisecting down the oops on memcpy() by updating the bisect
> >> with good or bad, depending if the test kernel hit the oops. I then
> >> revert a4a23f6, so that revert is the HEAD of the tree each time before
> >> building the kernel again(As long as the commit spit out by bisect is
> >> after when a4a23f6 was introduced).
> > Yep. Please continue bisecting the memcpy() oops.
> >
> > kmemdup() is just a kzalloc() followed by a memcpy(). When we split it
> > apart by reverting the patch then we would expect the oops to move to
> > the memcpy() part. Somehow "desc" is a bogus pointer, but I don't
> > immediately see how that is possible.
> >
> > regards,
> > dan carpenter
>
> Thanks for the details. We'll continue the bisect and let you know how
> it goes.
Did this please yield any useful result?
Thanks,
--
Jiri Kosina
SUSE Labs
On 09/24/2013 05:29 AM, Jiri Kosina wrote:
> On Mon, 16 Sep 2013, Joseph Salisbury wrote:
>
>>>> Can you explain a little further? Mark commit a4a23f6 as bad? An
>>>> initial bisect already reported that was the first bad commit, so it
>>>> can't be marked bad. The oops on memcpy() happens after commit a4a23f6
>>>> is reverted. The oops on memcpy() did not happen before a4a23f6 was
>>>> committed, so I assume this new oops was introduced by a change later.
>>>>
>>>> Right now I'm bisecting down the oops on memcpy() by updating the bisect
>>>> with good or bad, depending if the test kernel hit the oops. I then
>>>> revert a4a23f6, so that revert is the HEAD of the tree each time before
>>>> building the kernel again(As long as the commit spit out by bisect is
>>>> after when a4a23f6 was introduced).
>>> Yep. Please continue bisecting the memcpy() oops.
>>>
>>> kmemdup() is just a kzalloc() followed by a memcpy(). When we split it
>>> apart by reverting the patch then we would expect the oops to move to
>>> the memcpy() part. Somehow "desc" is a bogus pointer, but I don't
>>> immediately see how that is possible.
>>>
>>> regards,
>>> dan carpenter
>> Thanks for the details. We'll continue the bisect and let you know how
>> it goes.
> Did this please yield any useful result?
>
> Thanks,
>
Thanks for following up, Jiri. It's been a little tricky narrowing this
one down. We bisected a couple of times, and both bisects indicated the
following commits as the first bad:
commit b1a1442a23776756b254b69786848a94d92445ba
Author: Jiri Kosina <[email protected]>
Date: Mon Jun 3 11:27:48 2013 +0200
HID: core: fix reporting of raw events
However, reverting this commit does not stop the system from locking up,
when the wireless trackpad is connected. I was thinking of maybe using
a brute force method and pulling out all the HID changes, added in
3.11-rc1 to ensure the bug goes away. Then add a group back in at a
time to narrow down the commit that introduced this. The bisect should
have done this, but I'm not sure why it didn't. It would be greatly
appreciated if you had any other suggestions on tracking down the cause?
Thanks,
Joe
On 09/24/2013 05:29 AM, Jiri Kosina wrote:
> On Mon, 16 Sep 2013, Joseph Salisbury wrote:
>
>>>> Can you explain a little further? Mark commit a4a23f6 as bad? An
>>>> initial bisect already reported that was the first bad commit, so it
>>>> can't be marked bad. The oops on memcpy() happens after commit a4a23f6
>>>> is reverted. The oops on memcpy() did not happen before a4a23f6 was
>>>> committed, so I assume this new oops was introduced by a change later.
>>>>
>>>> Right now I'm bisecting down the oops on memcpy() by updating the bisect
>>>> with good or bad, depending if the test kernel hit the oops. I then
>>>> revert a4a23f6, so that revert is the HEAD of the tree each time before
>>>> building the kernel again(As long as the commit spit out by bisect is
>>>> after when a4a23f6 was introduced).
>>> Yep. Please continue bisecting the memcpy() oops.
>>>
>>> kmemdup() is just a kzalloc() followed by a memcpy(). When we split it
>>> apart by reverting the patch then we would expect the oops to move to
>>> the memcpy() part. Somehow "desc" is a bogus pointer, but I don't
>>> immediately see how that is possible.
>>>
>>> regards,
>>> dan carpenter
>> Thanks for the details. We'll continue the bisect and let you know how
>> it goes.
> Did this please yield any useful result?
>
> Thanks,
>
We also tested the 3.12-rc2 kernel, but it also produces an oops and
lockup. In case it's of use, the 3.12-rc2 oops can be seen at:
https://launchpadlibrarian.net/151359441/kern.log
On 09/24/2013 05:29 AM, Jiri Kosina wrote:
> On Mon, 16 Sep 2013, Joseph Salisbury wrote:
>
>>>> Can you explain a little further? Mark commit a4a23f6 as bad? An
>>>> initial bisect already reported that was the first bad commit, so it
>>>> can't be marked bad. The oops on memcpy() happens after commit a4a23f6
>>>> is reverted. The oops on memcpy() did not happen before a4a23f6 was
>>>> committed, so I assume this new oops was introduced by a change later.
>>>>
>>>> Right now I'm bisecting down the oops on memcpy() by updating the bisect
>>>> with good or bad, depending if the test kernel hit the oops. I then
>>>> revert a4a23f6, so that revert is the HEAD of the tree each time before
>>>> building the kernel again(As long as the commit spit out by bisect is
>>>> after when a4a23f6 was introduced).
>>> Yep. Please continue bisecting the memcpy() oops.
>>>
>>> kmemdup() is just a kzalloc() followed by a memcpy(). When we split it
>>> apart by reverting the patch then we would expect the oops to move to
>>> the memcpy() part. Somehow "desc" is a bogus pointer, but I don't
>>> immediately see how that is possible.
>>>
>>> regards,
>>> dan carpenter
>> Thanks for the details. We'll continue the bisect and let you know how
>> it goes.
> Did this please yield any useful result?
>
> Thanks,
>
After further testing reverting the following commit does in fact
resolve the bug:
commit b1a1442a23776756b254b69786848a94d92445ba
Author: Jiri Kosina <[email protected]>
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?
Thanks,
Joe
On Wed, 25 Sep 2013, Joseph Salisbury wrote:
> After further testing reverting the following commit does in fact
> resolve the bug:
>
> commit b1a1442a23776756b254b69786848a94d92445ba
> Author: Jiri Kosina <[email protected]>
> 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: <[email protected]>
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,
--
Jiri Kosina
SUSE Labs
On 09/27/2013 06:50 AM, Jiri Kosina wrote:
> On Wed, 25 Sep 2013, Joseph Salisbury wrote:
>
>> After further testing reverting the following commit does in fact
>> resolve the bug:
>>
>> commit b1a1442a23776756b254b69786848a94d92445ba
>> Author: Jiri Kosina <[email protected]>
>> 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: <[email protected]>
> 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.
Thanks,
Joe
On Fri, Sep 27, 2013 at 10:42:10AM -0400, Joseph Salisbury wrote:
> On 09/27/2013 06:50 AM, Jiri Kosina wrote:
> > On Wed, 25 Sep 2013, Joseph Salisbury wrote:
> >
> >> After further testing reverting the following commit does in fact
> >> resolve the bug:
> >>
> >> commit b1a1442a23776756b254b69786848a94d92445ba
> >> Author: Jiri Kosina <[email protected]>
> >> 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: <[email protected]>
> > 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.
>
It looks like magicmouse_raw_event() returns 1 on success and 0 on
failure.
regards,
dan carpenter
On Fri, Sep 27, 2013 at 06:24:12PM +0300, Dan Carpenter wrote:
>
> It looks like magicmouse_raw_event() returns 1 on success and 0 on
> failure.
Fixing the return codes is a good idea but it won't fix the oops.
What's the point of returning 1 and 0? In the current code no one
cares and both are treated the same.
Also if we decide to fix this instead of reverting the we could do this
cleanup as well:
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b8470b1..868ebaa 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1417,10 +1417,8 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
ret = hdrv->raw_event(hid, report, data, size);
- if (ret < 0) {
- ret = ret < 0 ? ret : 0;
+ if (ret < 0)
goto unlock;
- }
}
ret = hid_report_raw_event(hid, type, data, size, interrupt);
On Fri, 27 Sep 2013, Dan Carpenter wrote:
> It looks like magicmouse_raw_event() returns 1 on success and 0 on
> failure.
Good catch indeed.
I am not completely sure whether we are going to fix an oops or not by
this, as I haven't seen the actual oops anywhere in this thread :) But
definitely this looks like a good fix.
Joseph, could you please test with that? Thanks.
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 3b43d1c..c211eb9 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -334,7 +334,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
size - 2 - data[1]);
break;
default:
- return 0;
+ return 1;
}
if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
@@ -347,7 +347,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
}
input_sync(input);
- return 1;
+ return 0;
}
static int magicmouse_setup_input(struct input_dev *input, struct hid_device *hdev)
--
Jiri Kosina
SUSE Labs
On Mon, Sep 30, 2013 at 04:35:47PM +0200, Jiri Kosina wrote:
> On Fri, 27 Sep 2013, Dan Carpenter wrote:
>
> > It looks like magicmouse_raw_event() returns 1 on success and 0 on
> > failure.
>
> Good catch indeed.
>
> I am not completely sure whether we are going to fix an oops or not by
> this, as I haven't seen the actual oops anywhere in this thread :) But
> definitely this looks like a good fix.
>
> Joseph, could you please test with that? Thanks.
In the new code both 0 and 1 are treated the same so this can't fix the
bug.
regards,
dan carpenter