On Sat, Aug 8, 2009 at 1:46 PM, Alexey Klimov<klimov.linux@xxxxxxxxx> wrote:
> Patch fixes suspend/resume procedure.
>
> Signed-off-by: Alexey Klimov <klimov.linux@xxxxxxxxx>
>
> --
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
