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

[Oliver Neukum <oliver@neukum.org>]

Am Mittwoch 04 Juni 2008 10:20:21 schrieb Ville Syrjälä:
> 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.

You call it for two reasons

1. To safely enable remote wakeup
2. To make sure the device starts out awake

> > > +
> > > +       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...

Hm. Anybody on the list an expert on locking in the input layer?

> > 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.

Strictly speaking, it shouldn't. We should do it correctly to
a) make sure it works with all versions
b) make sure it can handle the command correctly

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html