radio-mr800: update suspend/resume procedure
PATCH 5/6 radio-mr800: update suspend/resume procedure
Patch fixes suspend/resume procedure.
--
linux/drivers/media/radio/radio-mr800.c +33 -8
@@ -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" );
Signature · 1 line
Best regards, Klimov Alexey
Quoted 9 lines · Alexey Klimov
Patch fixes suspend/resume procedure.
Signed-off-by Alexey Klimov <[email protected]>--
diff -r05754a500f80linux/drivers/media/radio/radio-mr800.c
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 line 568
Quoted 9 lines
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:06:36 2009 +0400 +1
-4@@ -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.
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 line 570
Quoted 2 lines
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:06:36 2009 +0400 +2
-0+ 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.
Quoted 17 lines
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:06:36 2009 +0400 +13
-0+ 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");
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 line 595
Quoted 9 lines
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:06:36 2009 +0400 +1
-4@@ -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.
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 line 605
Quoted 10 lines
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:06:36 2009 +0400 +10
-0+ 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.
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 line 606
Quoted 1 line
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:06:36 2009 +0400 +1
-0+ retval = amradio_set_stereo(radio, WANT_STEREO);
amradio_set_stereo should be refactored here as well.
Quoted 11 lines
linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:22:05 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 20:06:36 2009 +0400 +5
-1+ if (retval < 0) + dev_warn(&intf->dev, "set stereo failed\n"); + } + + } dev_info(&intf->dev, "coming out of suspend..\n"); --
Quoted 2 lines
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