On Wednesday 10 April 2013 08:18:57 Dan Williams wrote:
> On Wed, 2013-04-10 at 15:06 +0200, Oliver Neukum wrote:
> > On Wednesday 10 April 2013 07:49:11 Dan Williams wrote:
> > > On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> > > > On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> >
> > > > > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > > > > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > > > > + bool force)
> > > > > +{
> > > > > + int ret = 0;
> > > > > + bool submit = false;
> > > > > +
> > > > > + if (!dev->interrupt)
> > > > > + return 0;
> > > > > +
> > > > > + mutex_lock(&dev->interrupt_mutex);
> > > > > +
> > > > > + if (force) {
> > > >
> > > > That design means that interrupt_count isn't accurate if force is used.
> > > > That is extremely ugly.
> > >
> > > True; the problem here is that the URB isn't always submitted when
> > > suspend is used. For example, in a normal driver that doesn't need the
> > > URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
> > > Then if the system suspends, we can't decrement interrupt_count because
> > > it's zero.
> >
> > We don't need to. You ought to understand interrupt_count as
> > valid only while the device is not suspended.
>
> Ok, so at suspend we just drop the count to zero, force-kill the URB,
No, at suspend() ignore interrupt_count. Just kill.
> and then on resume it's not re-submitted again? That seems odd, since
On resume() evaluate interrupt_count.
> the usbnet driver handles submit/resubmit internally if the interface is
> IFF_UP, but when the interface is !IFF_UP then sub-drivers would have to
> track whether they submitted the urb or not, and then clear that on
> suspend? Having separate behavior for when the sub-driver starts the
> URB and when usbnet does seems inconsistent and error-prone.
>
> What approach would you suggest here?
Religiously use interrupt_count. With one exception.
The start/stop helpers are good. Just don't use them at suspend().
[..]
> See my questions above. Then we'd have to have the sub-drivers
> implement suspend/resume hooks so they'd be able to resubmit the
> interrupt URB on resume, and the whole point of this patch was to handle
> all that in usbnet. The sub-drivers don't know what the core driver's
> suspend/resume count is, because dev->suspend_count isn't exposed to
> subdrivers, and thus they don't know whether the device is actually
> suspended or not.
>
> The core problem is this... the sub-driver submits the URB before
> IFF_UP, and then at IFF_UP time usbnet wants to submit the driver.
> Let's say later the sub-driver doesn't need its private interrupt URB
> submission anymore, but it can't kill the URB because usbnet has
> submitted it too. Hence the refcounting.
The refcounting is very good. Just don't mess around with "force"
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
