Re: [PATCH] media: fix au0828 dvb suspend/resume to call dvb_frontend_suspend/resume

[Mauro Carvalho Chehab]

Hi Shuah,

Em Mon, 11 Aug 2014 21:13:55 -0600
Shuah Khan <shuah.kh@xxxxxxxxxxx> escreveu:

> au0828 doesn't resume correctly and TV tuning fails with
> xc_set_signal_source(0) failed message. Change au0828 dvb
> suspend and resume interfaces to suspend and resume frontend
> during suspend and resume respectively. au0828_dvb_suspend()
> calls dvb_frontend_suspend() which in turn invokes tuner ops
> sleep followed by fe ops sleep. au0828_dvb_resume() calls
> dvb_frontend_resume() which in turn calls fe ops ini follwed
> by tuner ops ini before waking up the frontend. With this change
> HVR950Q suspend and resume work when system gets suspended when
> digital function is tuned to a channel and with active TV stream,
> and after resume it went right back to active TV stream.
> 
> Signed-off-by: Shuah Khan <shuah.kh@xxxxxxxxxxx>
> ---
>  drivers/media/usb/au0828/au0828-dvb.c |   37 ++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/usb/au0828/au0828-dvb.c b/drivers/media/usb/au0828/au0828-dvb.c
> index 821f86e..50e7c82 100644
> --- a/drivers/media/usb/au0828/au0828-dvb.c
> +++ b/drivers/media/usb/au0828/au0828-dvb.c
> @@ -619,35 +619,30 @@ int au0828_dvb_register(struct au0828_dev *dev)
>  
>  void au0828_dvb_suspend(struct au0828_dev *dev)
>  {
> -	struct au0828_dvb *dvb = &dev->dvb;
> -
> -	if (dvb->frontend && dev->urb_streaming) {
> -		pr_info("stopping DVB\n");
> +	struct au0828_dvb *dvb;
> +	int rc;
>  
> -		cancel_work_sync(&dev->restart_streaming);

The restart streaming delayed work still needs to be canceled if
active. This work can be active while DVB is streaming, to workaround
on a chipset bug.

The best is to first call cancel the pending URBs and stop_urb_transfer(),
in order to avoid race conditions.

> +	if (dev == NULL)
> +		return;

In this specific device driver, I think that this condition will never
happen:

	$ git grep "dev = NULL" drivers/media/usb/au0828/au0828*
	drivers/media/usb/au0828/au0828-core.c: dev->usbdev = NULL;

>  
> -		/* Stop transport */
> -		mutex_lock(&dvb->lock);
> -		stop_urb_transfer(dev);

My understanding is that we still need to cancel the pending URBs before
suspending, in order to stop the DMA engine.

> -		au0828_stop_transport(dev, 1);
> -		mutex_unlock(&dvb->lock);
> -		dev->need_urb_start = 1;
> +	dvb = &dev->dvb;
> +	if (dvb->frontend) {
> +		rc = dvb_frontend_suspend(dvb->frontend);
> +		pr_info("au0828_dvb_suspend(): Suspending DVB fe %d\n", rc);
>  	}
>  }
>  
>  void au0828_dvb_resume(struct au0828_dev *dev)
>  {
> -	struct au0828_dvb *dvb = &dev->dvb;
> -
> -	if (dvb->frontend && dev->need_urb_start) {
> -		pr_info("resuming DVB\n");
> +	struct au0828_dvb *dvb;
> +	int rc;
>  
> -		au0828_set_frontend(dvb->frontend);
> +	if (dev == NULL)
> +		return;
>  
> -		/* Start transport */
> -		mutex_lock(&dvb->lock);
> -		au0828_start_transport(dev);
> -		start_urb_transfer(dev);
> -		mutex_unlock(&dvb->lock);

As we need to stop pending URB transfers and stop the DMA engine,
we still need the above code. We don't need to restart a pending
work, because what the pending work would be doing is to stop and
restart the transfer, and the suspend/resume code should do it already.

> +	dvb = &dev->dvb;
> +	if (dvb->frontend) {
> +		rc = dvb_frontend_resume(dvb->frontend);
> +		pr_info("au0828_dvb_resume(): Resuming DVB fe %d\n", rc);
>  	}
>  }

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/