Re: [PATCH 2/2] ati_remote2: Add autosuspend support

["Ville Syrjälä" <syrjala-ORSVBvAovxo@public.gmane.org>]

On Tue, Jun 03, 2008 at 10:11:09PM +0200, Oliver Neukum wrote:
> Am Dienstag 03 Juni 2008 20:45:47 schrieb Ville Syrjala:
> > +static int ati_remote2_open(struct input_dev *idev)
> > +{
> > +       struct ati_remote2 *ar2 = input_get_drvdata(idev);
> > +       int r;
> > +
> > +       dev_dbg(&ar2->intf[0]->dev, "%s()\n", __FUNCTION__);
> > +
> > +       r = usb_autopm_get_interface(ar2->intf[0]);
> > +       if (r) {
> > +               dev_err(&ar2->intf[0]->dev,
> > +                       "%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
> > +               goto fail1;
> > +       }
> > +       r = usb_autopm_get_interface(ar2->intf[1]);
> > +       if (r) {
> > +               dev_err(&ar2->intf[1]->dev,
> > +                       "%s(): usb_autopm_get_interface() = %d\n", __FUNCTION__, r);
> > +               goto fail2;
> > +       }
> 
> If you have two interfaces on one device upping the count for one of them
> is enough.

OK. But now I wonder why do I even bother calling this function. It
doesn't seem to do anything particularly useful.

> > +
> > +       mutex_lock(&ati_remote2_mutex);
> 
> Too late. You can race with disconnect()

Hmm. Do you mean open() vs. disconnect()? Doesn't the input_dev's locking
take care of that? ati_remote2_mutex is there just to make ar2->flags
handling and urb submitting/killing atomic, it didn't even exist before
this autosuspend patch. Or perhaps I'm missing something...

> And you should set needs_remote_wakeup.

What exactly does that do? As I said remote wakeup isn't enabled on the
device but it still wakes up just fine.

-- 
Ville Syrjälä
syrjala-ORSVBvAovxo@public.gmane.org
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html