s2ram slow (radeon) / failing (usb)
in [Kernel]
|
Prev: intel_txt: enable VMXON check with SMX in KVM
Next: SCSI: Support HW reset for EH and polling scheme for scsi device
From: Jiri Kosina on 5 May 2010 09:00 On Tue, 4 May 2010, Bruno Pr�mont wrote: > > Bruno, could you, for testing purposes, check, whether the patch below > > changes the behavior you are seeing (and also check what the actual return > > value from device_add() was, see the added printk()). > > Well, it's said to return 0: > > [ 286.872739] generic-usb 0003:058F:9462.0003: input: USB HID v1.10 Keyboard [Multimedia USB Keyboard Multimedia USB Keyboard] on usb-0000:00:13.0-2.1/input0 > [ 286.890136] HID: device_add() returned 0 > > [ 286.919698] generic-usb: probe of 0003:058F:9462.0004 failed with error -22 > [ 286.929363] HID: device_add() returned 0 > > or with debug=1 for HID: > [ 559.982958] /usr/src/linux-2.6-git/drivers/hid/usbhid/hid-core.c: HID probe called for ifnum 1 > [ 560.011698] /usr/src/linux-2.6-git/drivers/hid/hid-core.c: usage index exceeded > [ 560.011704] /usr/src/linux-2.6-git/drivers/hid/hid-core.c: hid_add_usage failed > [ 560.011710] /usr/src/linux-2.6-git/drivers/hid/hid-core.c: item 0 2 2 2 parsing failed > [ 560.011732] /usr/src/linux-2.6-git/drivers/hid/usbhid/hid-core.c: parsing report descriptor failed > [ 560.011747] generic-usb: probe of 0003:058F:9462.0006 failed with error -22 > [ 560.021718] HID: device_add() returned 0 Could you please verify with this patch? Thanks. drivers/hid/hid-core.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 2e2aa75..8df19d3 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1486,8 +1486,10 @@ static int hid_device_probe(struct device *dev) if (!ret) ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); } - if (ret) + if (ret) { hdev->driver = NULL; + ret = -ENODEV; + } } return ret; } -- Jiri Kosina SUSE Labs, Novell Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Bruno Prémont on 5 May 2010 15:20 On Wed, 05 May 2010 Jiri Kosina <jkosina(a)suse.cz> wrote: > On Tue, 4 May 2010, Bruno Prémont wrote: > > > > Bruno, could you, for testing purposes, check, whether the patch below > > > changes the behavior you are seeing (and also check what the actual return > > > value from device_add() was, see the added printk()). > > > > Well, it's said to return 0: > > > > [ 286.872739] generic-usb 0003:058F:9462.0003: input: USB HID v1.10 Keyboard [Multimedia USB Keyboard Multimedia USB Keyboard] on usb-0000:00:13.0-2.1/input0 > > [ 286.890136] HID: device_add() returned 0 > > > > [ 286.919698] generic-usb: probe of 0003:058F:9462.0004 failed with error -22 > > [ 286.929363] HID: device_add() returned 0 > > > > or with debug=1 for HID: > > [ 559.982958] /usr/src/linux-2.6-git/drivers/hid/usbhid/hid-core.c: HID probe called for ifnum 1 > > [ 560.011698] /usr/src/linux-2.6-git/drivers/hid/hid-core.c: usage index exceeded > > [ 560.011704] /usr/src/linux-2.6-git/drivers/hid/hid-core.c: hid_add_usage failed > > [ 560.011710] /usr/src/linux-2.6-git/drivers/hid/hid-core.c: item 0 2 2 2 parsing failed > > [ 560.011732] /usr/src/linux-2.6-git/drivers/hid/usbhid/hid-core.c: parsing report descriptor failed > > [ 560.011747] generic-usb: probe of 0003:058F:9462.0006 failed with error -22 > > [ 560.021718] HID: device_add() returned 0 > > Could you please verify with this patch? Thanks. No difference, the second interface remains registered with HID bus and I don't spot any difference in kernel log. Going to have a look at what probe exactly it's that returns -EINVAL and then attempt to unroll it up to device_add() ... Hopefully I will find the function that does not hand back the failure. Thanks, Bruno > drivers/hid/hid-core.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 2e2aa75..8df19d3 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1486,8 +1486,10 @@ static int hid_device_probe(struct device *dev) > if (!ret) > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > } > - if (ret) > + if (ret) { > hdev->driver = NULL; > + ret = -ENODEV; > + } > } > return ret; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Bruno Prémont on 5 May 2010 16:40 On Tue, 04 May 2010 Jiri Kosina <jkosina(a)suse.cz> wrote: > On Tue, 4 May 2010, Oliver Neukum wrote: > > > > [ 477.543304] usb 1-2.1: usb_autosuspend_device: cnt 1 -> 0 > > > [ 477.543316] usbhid 1-2.1:1.1: __pm_runtime_suspend()! > > > [ 477.543326] usbhid 1-2.1:1.1: __pm_runtime_suspend() returns 0! > > > [ 477.543380] usbcore: registered new interface driver usbhid > > > [ 477.549457] usbhid: USB HID core driver > > > > > > And suspend is freezing inside of hid_cancel_delayed_stuff(usbhid) call > > > from hid_suspend() in drivers/hid/usbhid/hid-core.c ... > > > > > > Is it worth continuing iteration and adding further printk's down there? > > > Jiri, what's your opinion on this? > > > > Ugh. That looks like a bug in usbhid that I introduced. A fix is not trivial. > > In short, I did not think the device could be undergoing a queued resumption > > while suspend() is being called. I wonder why this is happening. > > Hmmm ... seems to me that in this case, the problem might be that there is > a device hanging in the air, for which the parsing of report descriptor > failed (interface .0002), but it's still somehow there on the bus. > > It's a bit strange that we are not seeing > > dev_err(&intf->dev, "can't add hid device: %d\n", ret); > > message from usbhid_probe(), are we? That would mean, that we are > returning ENODEV from the usb_driver->probe routine properly. > > Bruno, could you, for testing purposes, check, whether the patch below > changes the behavior you are seeing (and also check what the actual return > value from device_add() was, see the added printk()). > Thanks. > > > > drivers/hid/hid-core.c | 5 +++-- > drivers/hid/usbhid/hid-core.c | 4 ++-- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 2e2aa75..7186f9f 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1770,10 +1770,11 @@ int hid_add_device(struct hid_device *hdev) > hdev->vendor, hdev->product, atomic_inc_return(&id)); > > ret = device_add(&hdev->dev); > + printk(KERN_DEBUG "HID: device_add() returned %d\n", ret); > if (!ret) > hdev->status |= HID_STAT_ADDED; > - > - hid_debug_register(hdev, dev_name(&hdev->dev)); > + else > + hid_debug_register(hdev, dev_name(&hdev->dev)); > > return ret; > } Ok, I've been digging some further... The hid_device_probe properly returns -ENODEV, but: Call trace: [ 3228.866146] [<ffffffffa01a00e6>] hid_device_probe+0xd6/0x1f0 [hid] return -ENODEV [ 3228.874594] [<ffffffff8130995a>] driver_probe_device+0xaa/0x1d0 calls inlined really_probe from drivers/base/dd.c which ALLWAYS returns 0: dd.c:147 /* 148 * Ignore errors returned by ->probe so that the next driver can try 149 * its luck. 150 */ 151 ret = 0; and has on line 139 (under same failure label): dev->driver = NULL; [ 3228.882758] [<ffffffff81309b20>] ? __device_attach+0x0/0x50 [ 3228.890555] [<ffffffff81309b6b>] __device_attach+0x4b/0x50 lets 0 bubble up [ 3228.898272] [<ffffffff81308d28>] bus_for_each_drv+0x68/0x90 lets 0 bubble up [ 3228.906080] [<ffffffff81309c3b>] device_attach+0x8b/0xa0 lets 0 bubble up [ 3228.913603] [<ffffffff81308b15>] bus_probe_device+0x25/0x40 returns void and does WARN_ON(device_attach() < 0) [ 3228.921356] [<ffffffff81307166>] device_add+0x3d6/0x610 returns 0 here as there was no local error [ 3228.928772] [<ffffffffa019fc53>] hid_add_device+0x183/0x1e0 [hid] [ 3228.937098] [<ffffffffa01b4a77>] usbhid_probe+0x287/0x420 [usbhid] [ 3228.945535] [<ffffffffa005006d>] usb_probe_interface+0x14d/0x230 [usbcore] .... So IMHO in hid_add_device() we should also check for hdev->dev.driver when device_add() returns 0 and consider that one being NULL as a (possible) error. Thanks, Bruno -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Kosina on 5 May 2010 17:00 [ Jiri Slaby added to CC ] On Wed, 5 May 2010, Bruno Prémont wrote: > On Wed, 05 May 2010 Bruno Prémont <bonbons(a)linux-vserver.org> wrote: > > > On Tue, 04 May 2010 Jiri Kosina <jkosina(a)suse.cz> wrote: > > > > > On Tue, 4 May 2010, Oliver Neukum wrote: > > > > > > > > [ 477.543304] usb 1-2.1: usb_autosuspend_device: cnt 1 -> 0 > > > > > [ 477.543316] usbhid 1-2.1:1.1: __pm_runtime_suspend()! > > > > > [ 477.543326] usbhid 1-2.1:1.1: __pm_runtime_suspend() returns 0! > > > > > [ 477.543380] usbcore: registered new interface driver usbhid > > > > > [ 477.549457] usbhid: USB HID core driver > > > > > > > > > > And suspend is freezing inside of hid_cancel_delayed_stuff(usbhid) call > > > > > from hid_suspend() in drivers/hid/usbhid/hid-core.c ... > > > > > > > > > > Is it worth continuing iteration and adding further printk's down there? > > > > > Jiri, what's your opinion on this? > > > > > > > > Ugh. That looks like a bug in usbhid that I introduced. A fix is not trivial. > > > > In short, I did not think the device could be undergoing a queued resumption > > > > while suspend() is being called. I wonder why this is happening. > > > > > > Hmmm ... seems to me that in this case, the problem might be that there is > > > a device hanging in the air, for which the parsing of report descriptor > > > failed (interface .0002), but it's still somehow there on the bus. > > > > > > It's a bit strange that we are not seeing > > > > > > dev_err(&intf->dev, "can't add hid device: %d\n", ret); > > > > > > message from usbhid_probe(), are we? That would mean, that we are > > > returning ENODEV from the usb_driver->probe routine properly. > > > > > > Bruno, could you, for testing purposes, check, whether the patch below > > > changes the behavior you are seeing (and also check what the actual return > > > value from device_add() was, see the added printk()). > > > Thanks. > > > > > > > > > > > > drivers/hid/hid-core.c | 5 +++-- > > > drivers/hid/usbhid/hid-core.c | 4 ++-- > > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > > index 2e2aa75..7186f9f 100644 > > > --- a/drivers/hid/hid-core.c > > > +++ b/drivers/hid/hid-core.c > > > @@ -1770,10 +1770,11 @@ int hid_add_device(struct hid_device *hdev) > > > hdev->vendor, hdev->product, atomic_inc_return(&id)); > > > > > > ret = device_add(&hdev->dev); > > > + printk(KERN_DEBUG "HID: device_add() returned %d\n", ret); > > > if (!ret) > > > hdev->status |= HID_STAT_ADDED; > > > - > > > - hid_debug_register(hdev, dev_name(&hdev->dev)); > > > + else > > > + hid_debug_register(hdev, dev_name(&hdev->dev)); > > > > > > return ret; > > > } > > > > Ok, I've been digging some further... > > > > The hid_device_probe properly returns -ENODEV, but: > > > > Call trace: > > [ 3228.866146] [<ffffffffa01a00e6>] hid_device_probe+0xd6/0x1f0 [hid] > > return -ENODEV > > [ 3228.874594] [<ffffffff8130995a>] driver_probe_device+0xaa/0x1d0 > > calls inlined really_probe from drivers/base/dd.c > > which ALLWAYS returns 0: > > dd.c:147 /* > > 148 * Ignore errors returned by ->probe so that the next driver can try > > 149 * its luck. > > 150 */ > > 151 ret = 0; > > and has on line 139 (under same failure label): > > dev->driver = NULL; > > [ 3228.882758] [<ffffffff81309b20>] ? __device_attach+0x0/0x50 > > [ 3228.890555] [<ffffffff81309b6b>] __device_attach+0x4b/0x50 > > lets 0 bubble up > > [ 3228.898272] [<ffffffff81308d28>] bus_for_each_drv+0x68/0x90 > > lets 0 bubble up > > [ 3228.906080] [<ffffffff81309c3b>] device_attach+0x8b/0xa0 > > lets 0 bubble up > > [ 3228.913603] [<ffffffff81308b15>] bus_probe_device+0x25/0x40 > > returns void and does WARN_ON(device_attach() < 0) > > [ 3228.921356] [<ffffffff81307166>] device_add+0x3d6/0x610 > > returns 0 here as there was no local error > > [ 3228.928772] [<ffffffffa019fc53>] hid_add_device+0x183/0x1e0 [hid] > > [ 3228.937098] [<ffffffffa01b4a77>] usbhid_probe+0x287/0x420 [usbhid] > > [ 3228.945535] [<ffffffffa005006d>] usb_probe_interface+0x14d/0x230 [usbcore] > > ... > > > > So IMHO in hid_add_device() we should also check for hdev->dev.driver > > when device_add() returns 0 and consider that one being NULL as a > > (possible) error. > > Something like the delow diff causes the HID registration to fail > gracefully and `echo devices > pm_test` suspend attempt to pass. > > (note, to be manually applied, is edited copy from console, so does > not preserve tabs and line numbers may not match due to debug printks > added all over the place) > > > I don't know what impact it could have on auto-probing of device > if a specialized HID driver that would fix reports or whatever was > loaded later on when device is already plugged into USB. > > Thanks, > Bruno > > > @@ -1770,11 +1779,13 @@ int hid_add_device(struct hid_device *hdev) > hdev->vendor, hdev->product, atomic_inc_return(&id)); > > ret = device_add(&hdev->dev); > - if (!ret) > + if (ret == 0 && !hdev->dev.driver) { > + device_del(&hdev->dev); > + ret = -ENODEV; > + } else { > hdev->status |= HID_STAT_ADDED; > - > - hid_debug_register(hdev, dev_name(&hdev->dev)); > - > + hid_debug_register(hdev, dev_name(&hdev->dev)); > + } > return ret; > } > EXPORT_SYMBOL_GPL(hid_add_device); > -- Jiri Kosina SUSE Labs, Novell Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Bruno Prémont on 5 May 2010 17:00
On Wed, 05 May 2010 Bruno Prémont <bonbons(a)linux-vserver.org> wrote: > On Tue, 04 May 2010 Jiri Kosina <jkosina(a)suse.cz> wrote: > > > On Tue, 4 May 2010, Oliver Neukum wrote: > > > > > > [ 477.543304] usb 1-2.1: usb_autosuspend_device: cnt 1 -> 0 > > > > [ 477.543316] usbhid 1-2.1:1.1: __pm_runtime_suspend()! > > > > [ 477.543326] usbhid 1-2.1:1.1: __pm_runtime_suspend() returns 0! > > > > [ 477.543380] usbcore: registered new interface driver usbhid > > > > [ 477.549457] usbhid: USB HID core driver > > > > > > > > And suspend is freezing inside of hid_cancel_delayed_stuff(usbhid) call > > > > from hid_suspend() in drivers/hid/usbhid/hid-core.c ... > > > > > > > > Is it worth continuing iteration and adding further printk's down there? > > > > Jiri, what's your opinion on this? > > > > > > Ugh. That looks like a bug in usbhid that I introduced. A fix is not trivial. > > > In short, I did not think the device could be undergoing a queued resumption > > > while suspend() is being called. I wonder why this is happening. > > > > Hmmm ... seems to me that in this case, the problem might be that there is > > a device hanging in the air, for which the parsing of report descriptor > > failed (interface .0002), but it's still somehow there on the bus. > > > > It's a bit strange that we are not seeing > > > > dev_err(&intf->dev, "can't add hid device: %d\n", ret); > > > > message from usbhid_probe(), are we? That would mean, that we are > > returning ENODEV from the usb_driver->probe routine properly. > > > > Bruno, could you, for testing purposes, check, whether the patch below > > changes the behavior you are seeing (and also check what the actual return > > value from device_add() was, see the added printk()). > > Thanks. > > > > > > > > drivers/hid/hid-core.c | 5 +++-- > > drivers/hid/usbhid/hid-core.c | 4 ++-- > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 2e2aa75..7186f9f 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -1770,10 +1770,11 @@ int hid_add_device(struct hid_device *hdev) > > hdev->vendor, hdev->product, atomic_inc_return(&id)); > > > > ret = device_add(&hdev->dev); > > + printk(KERN_DEBUG "HID: device_add() returned %d\n", ret); > > if (!ret) > > hdev->status |= HID_STAT_ADDED; > > - > > - hid_debug_register(hdev, dev_name(&hdev->dev)); > > + else > > + hid_debug_register(hdev, dev_name(&hdev->dev)); > > > > return ret; > > } > > Ok, I've been digging some further... > > The hid_device_probe properly returns -ENODEV, but: > > Call trace: > [ 3228.866146] [<ffffffffa01a00e6>] hid_device_probe+0xd6/0x1f0 [hid] > return -ENODEV > [ 3228.874594] [<ffffffff8130995a>] driver_probe_device+0xaa/0x1d0 > calls inlined really_probe from drivers/base/dd.c > which ALLWAYS returns 0: > dd.c:147 /* > 148 * Ignore errors returned by ->probe so that the next driver can try > 149 * its luck. > 150 */ > 151 ret = 0; > and has on line 139 (under same failure label): > dev->driver = NULL; > [ 3228.882758] [<ffffffff81309b20>] ? __device_attach+0x0/0x50 > [ 3228.890555] [<ffffffff81309b6b>] __device_attach+0x4b/0x50 > lets 0 bubble up > [ 3228.898272] [<ffffffff81308d28>] bus_for_each_drv+0x68/0x90 > lets 0 bubble up > [ 3228.906080] [<ffffffff81309c3b>] device_attach+0x8b/0xa0 > lets 0 bubble up > [ 3228.913603] [<ffffffff81308b15>] bus_probe_device+0x25/0x40 > returns void and does WARN_ON(device_attach() < 0) > [ 3228.921356] [<ffffffff81307166>] device_add+0x3d6/0x610 > returns 0 here as there was no local error > [ 3228.928772] [<ffffffffa019fc53>] hid_add_device+0x183/0x1e0 [hid] > [ 3228.937098] [<ffffffffa01b4a77>] usbhid_probe+0x287/0x420 [usbhid] > [ 3228.945535] [<ffffffffa005006d>] usb_probe_interface+0x14d/0x230 [usbcore] > ... > > So IMHO in hid_add_device() we should also check for hdev->dev.driver > when device_add() returns 0 and consider that one being NULL as a > (possible) error. Something like the delow diff causes the HID registration to fail gracefully and `echo devices > pm_test` suspend attempt to pass. (note, to be manually applied, is edited copy from console, so does not preserve tabs and line numbers may not match due to debug printks added all over the place) I don't know what impact it could have on auto-probing of device if a specialized HID driver that would fix reports or whatever was loaded later on when device is already plugged into USB. Thanks, Bruno @@ -1770,11 +1779,13 @@ int hid_add_device(struct hid_device *hdev) hdev->vendor, hdev->product, atomic_inc_return(&id)); ret = device_add(&hdev->dev); - if (!ret) + if (ret == 0 && !hdev->dev.driver) { + device_del(&hdev->dev); + ret = -ENODEV; + } else { hdev->status |= HID_STAT_ADDED; - - hid_debug_register(hdev, dev_name(&hdev->dev)); - + hid_debug_register(hdev, dev_name(&hdev->dev)); + } return ret; } EXPORT_SYMBOL_GPL(hid_add_device); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |