On Wed, 05 May 2010 Bruno Prémont <bonbons@linux-vserver.org> wrote:

quoted text
> On Tue, 04 May 2010 Jiri Kosina <jkosina@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);
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/