On Fri, 9 Dec 2005, Greg KH wrote:
> On Fri, Dec 09, 2005 at 09:40:50PM -0500, Alan Stern wrote:
> > - if (usb_submit_urb(usblp->readurb, GFP_KERNEL) < 0)
> > + if (usblp->suspended || usb_submit_urb(usblp->readurb,
> > + GFP_KERNEL) < 0)
> > dbg("error submitting urb");
>
> Why is this needed? Every driver should not have to check to see if it
> is suspended, the usb core should handle this for us, like it does for
> removed devices.
>
> If it does not, then it needs to be fixed :)
On Fri, 9 Dec 2005, David Brownell wrote:
> There's already a check "is the usb_device suspended".
>
> What's not there is a "is the usb_interface associated with this
> usb_host_endpoint suspended" check. And that's a bit more complicated
> to do, although it does seem at least possible nowadays.
>
> Agreed about not wanting to modify every driver for this. :)
It's not quite so simple. What about URBs for ep0? ep0 isn't associated
with any usb_interface -- or rather, it's associated with all of them.
Ultimately we will need to add a pointer to usb_interface into struct urb.
That will mean modifying every driver also. Unless we decide to punt
and allow suspended drivers to send URBs to ep0 so long as the device
isn't suspended.
And while you're thinking about that problem, ponder this. When usbfs
binds an interface on behalf of a userspace program, how should it
implement suspend/resume?
On Fri, 9 Dec 2005, Greg KH wrote:
> > +#ifdef CONFIG_PM
>
> Do we really need the #ifdef?
It seems to be standard practice. Why include power management code if PM
isn't configured? Look at the hub driver, for example.
> > +static int usblp_suspend(struct usb_interface *intf, pm_message_t message)
> > +{
> > + struct usblp *usblp = usb_get_intfdata(intf);
> > +
> > + down(&usblp->sem);
> > + usblp->suspended = 1;
> > + usblp_unlink_urbs(usblp);
> > + intf->dev.power.power_state.event = message.event;
>
> Shouldn't this line be done by the usb core?
You're right, it is done by the core.
> > +static int usblp_resume(struct usb_interface *intf)
> > +{
> > + struct usblp *usblp = usb_get_intfdata(intf);
> > +
> > + down(&usblp->sem);
> > + usblp->suspended = 0;
> > + intf->dev.power.power_state.event = PM_EVENT_ON;
>
> Same here, the core should do this for us.
Ah, for some odd reason this assignment is missing from the core. The
usb_generic_suspend and usb_generic_resume routines definitely could use a
little going-over.
In fact the routines should be split into two pieces, for handling
devices vs. interfaces. The usb_generic_xxx parts belong in driver.c,
leaving usb_xxx parts behind in usb.c. This would be consistent with
making usb_generic_driver act more like a regular device driver.
> And yes, I know I've been ignoring your and David's pm changes to the
> usb core, sorry. But making all USB drivers handle things this way
> isn't going to work out.
Which aspect bothers you the most? Checking whether the interface is
suspended before submitting URBs? We need to figure out the best solution
to the ep0 problem first.
Alan Stern
-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems? Stop! Download the new AJAX search engine that makes
searching your log files as easy as surfing the web. DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
