Message ID | 1249753572.15160.250.camel@tux.localhost (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Douglas Landgraf |
Headers | show |
On Sat, Aug 8, 2009 at 1:46 PM, Alexey Klimov<klimov.linux@gmail.com> wrote: > Patch fixes suspend/resume procedure. > > Signed-off-by: Alexey Klimov <klimov.linux@gmail.com> > > -- > diff -r 05754a500f80 linux/drivers/media/radio/radio-mr800.c > --- a/linux/drivers/media/radio/radio-mr800.c  Sat Aug 08 20:06:36 2009 +0400 > +++ b/linux/drivers/media/radio/radio-mr800.c  Sat Aug 08 20:22:05 2009 +0400 > @@ -564,11 +564,23 @@ >  static int usb_amradio_suspend(struct usb_interface *intf, pm_message_t message) >  { >     struct amradio_device *radio = usb_get_intfdata(intf); > -    int retval; > > -    retval = amradio_set_mute(radio, AMRADIO_STOP); > -    if (retval < 0) > -        dev_warn(&intf->dev, "amradio_stop failed\n"); > +    if (radio->status == AMRADIO_START) { I think you need to take a good look at the locking in this driver. The above condition should technically be checked with the lock held to ensure that it doesn't change while it's being read. > +        int retval; > +        retval = amradio_set_mute(radio, AMRADIO_STOP); A good place to start would be with amradio_set_mute. In my opinion, you should remove the locks from this function, add a BUG_ON for !mutex_is_locked to the start of the function, and ensure all functions calling it currently hold the lock. > +        if (retval < 0) > +            dev_warn(&intf->dev, "amradio_stop failed\n"); > + > +        /* After stopping radio status set to AMRADIO_STOP. > +         * If we want driver to start radio on resume > +         * we set status equal to AMRADIO_START. > +         * On resume we will check status and run radio if needed. > +         */ > + > +        mutex_lock(&radio->lock); > +        radio->status = AMRADIO_START; > +        mutex_unlock(&radio->lock); > +    } > >     dev_info(&intf->dev, "going into suspend..\n"); > > @@ -579,11 +591,24 @@ >  static int usb_amradio_resume(struct usb_interface *intf) >  { >     struct amradio_device *radio = usb_get_intfdata(intf); > -    int retval; > > -    retval = amradio_set_mute(radio, AMRADIO_START); > -    if (retval < 0) > -        dev_warn(&intf->dev, "amradio_start failed\n"); > +    if (radio->status == AMRADIO_START) { Same as above. > +        int retval; > +        retval = amradio_set_mute(radio, AMRADIO_START); > +        if (retval < 0) > +            dev_warn(&intf->dev, "amradio_start failed\n"); > +        retval = amradio_setfreq(radio); > +        if (retval < 0) > +            dev_warn(&intf->dev, > +                "set frequency failed\n"); > + > +        if (radio->stereo) { Same as above. > +            retval = amradio_set_stereo(radio, WANT_STEREO); amradio_set_stereo should be refactored here as well. > +            if (retval < 0) > +            dev_warn(&intf->dev, "set stereo failed\n"); > +        } > + > +    } > >     dev_info(&intf->dev, "coming out of suspend..\n"); > > > > -- > Best regards, Klimov Alexey > If you properly address the locking in this driver, you should be able to remove the dependency of this driver on the lock_kernel() and unlock_kernel() constructs. Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff -r 05754a500f80 linux/drivers/media/radio/radio-mr800.c --- a/linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:06:36 2009 +0400 +++ b/linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 @@ -564,11 +564,23 @@ static int usb_amradio_suspend(struct usb_interface *intf, pm_message_t message) { struct amradio_device *radio = usb_get_intfdata(intf); - int retval; - retval = amradio_set_mute(radio, AMRADIO_STOP); - if (retval < 0) - dev_warn(&intf->dev, "amradio_stop failed\n"); + if (radio->status == AMRADIO_START) { + int retval; + retval = amradio_set_mute(radio, AMRADIO_STOP); + if (retval < 0) + dev_warn(&intf->dev, "amradio_stop failed\n"); + + /* After stopping radio status set to AMRADIO_STOP. + * If we want driver to start radio on resume + * we set status equal to AMRADIO_START. + * On resume we will check status and run radio if needed. + */ + + mutex_lock(&radio->lock); + radio->status = AMRADIO_START; + mutex_unlock(&radio->lock); + } dev_info(&intf->dev, "going into suspend..\n"); @@ -579,11 +591,24 @@ static int usb_amradio_resume(struct usb_interface *intf) { struct amradio_device *radio = usb_get_intfdata(intf); - int retval; - retval = amradio_set_mute(radio, AMRADIO_START); - if (retval < 0) - dev_warn(&intf->dev, "amradio_start failed\n"); + if (radio->status == AMRADIO_START) { + int retval; + retval = amradio_set_mute(radio, AMRADIO_START); + if (retval < 0) + dev_warn(&intf->dev, "amradio_start failed\n"); + retval = amradio_setfreq(radio); + if (retval < 0) + dev_warn(&intf->dev, + "set frequency failed\n"); + + if (radio->stereo) { + retval = amradio_set_stereo(radio, WANT_STEREO); + if (retval < 0) + dev_warn(&intf->dev, "set stereo failed\n"); + } + + } dev_info(&intf->dev, "coming out of suspend..\n");
Patch fixes suspend/resume procedure. Signed-off-by: Alexey Klimov <klimov.linux@gmail.com> --