[linux-dvb] [PATCH] propagating more errors

[Oliver Endriss]

Hi,

I finally managed to dig through your patch. You did a great job.
Hopefully the changed behaviour will not cause too much trouble.

Wolfgang Rohdewald wrote:
> diff -up dvb-cvs/dvb-kernel/linux/drivers/media/dvb/ttpci/av7110_av.c linux/drivers/media/dvb/ttpci/av7110_av.c
> ...
> @@ -974,7 +986,7 @@ static int dvb_video_ioctl(struct inode 
> ...
>  	case VIDEO_PLAY:
> ...
>  		if (av7110->videostate.stream_source == VIDEO_SOURCE_MEMORY) {
>  			if (av7110->playing == RP_AV) {
> -				av7110_fw_cmd(av7110, COMTYPE_REC_PLAY, __Stop, 0);
> +				ret = av7110_fw_cmd(av7110, COMTYPE_REC_PLAY, __Stop, 0);
> +				if (ret)
> +					break;
>  				av7110->playing &= ~RP_VIDEO;
>  			}
> -			av7110_av_start_play(av7110, RP_VIDEO);
> -			vidcom(av7110, VIDEO_CMD_PLAY, 0);
> +			ret = av7110_av_start_play(av7110, RP_VIDEO);
> +			if (!ret)
> +				ret = vidcom(av7110, VIDEO_CMD_PLAY, 0);
>  		} else {
> -			//av7110_av_stop(av7110, RP_VIDEO);
> -			vidcom(av7110, VIDEO_CMD_PLAY, 0);
> +			// ret = av7110_av_stop(av7110, RP_VIDEO);
> +			if (!ret)
> +				ret = vidcom(av7110, VIDEO_CMD_PLAY, 0);
>  		}

@all:
Can we remove the commented-out code?
vidcom() is executed for 'if' and 'else', so the else clause can be
removed.



case VIDEO_CLEAR_BUFFER:
> @@ -1136,18 +1167,19 @@ static int dvb_video_ioctl(struct inode 
>  		av7110_ipack_reset(&av7110->ipack[1]);
>  
>  		if (av7110->playing == RP_AV) {
> -			av7110_fw_cmd(av7110, COMTYPE_REC_PLAY,
> +			ret = av7110_fw_cmd(av7110, COMTYPE_REC_PLAY,
>  				      __Play, 2, AV_PES, 0);

Add something like
			if(ret)
				break;
here.

>  			if (av7110->trickmode == TRICK_FAST)
> -				av7110_fw_cmd(av7110, COMTYPE_REC_PLAY,
> +				ret = av7110_fw_cmd(av7110, COMTYPE_REC_PLAY,
>  					      __Scan_I, 2, AV_PES, 0);
>  			if (av7110->trickmode == TRICK_SLOW) {
> ...





> diff -up dvb-cvs/dvb-kernel/linux/drivers/media/dvb/ttpci/av7110.c linux/drivers/media/dvb/ttpci/av7110.c
> ...
> @@ -856,10 +875,12 @@ static int StopHWFilter(struct dvb_demux
>  	dprintk(4, "%p\n", av7110);
>  
>  	handle = dvbdmxfilter->hw_handle;
> +	if (handle == 0xffff) /*   the filter has not really been started */
> +		return 0;

Hmm - is it normal to get here with handle==0xffff?
Wouldn't it be better to have a log entry as it was before?

> @@ -1119,18 +1166,16 @@ static int av7110_set_tone(struct dvb_fr
>  
>  	switch (tone) {
>  	case SEC_TONE_ON:
> -		Set22K(av7110, 1);
> +		return Set22K(av7110, 1);
>  		break;

You should remove the 'break' here.

>  	case SEC_TONE_OFF:
> -		Set22K(av7110, 0);
> +		return Set22K(av7110, 0);
>  		break;

ditto



av7110_fe_lock_fix() was (and is) broken. It would have never be retried
after -ERESTARTSYS. For a fix see below.

> -static void av7110_fe_lock_fix(struct av7110* av7110, fe_status_t status)
> +static int av7110_fe_lock_fix(struct av7110* av7110, fe_status_t status)
>  {
> +	int ret = 0;
>  	int synced = (status & FE_HAS_LOCK) ? 1 : 0;
>  
>  	av7110->fe_status = status;
>  
>  	if (av7110->fe_synced == synced)
> -		return;
> +		return 0;
>  
>  	av7110->fe_synced = synced;

Delete the line above, and move it to the end.

>  	if (av7110->playing)
> -		return;
> +		return 0;
>  
>  	if (down_interruptible(&av7110->pid_mutex))
> -		return;
> +		return -ERESTARTSYS;
>  
>  	if (av7110->fe_synced) {

replace by 'if (synced) {'

> -		SetPIDs(av7110, av7110->pids[DMX_PES_VIDEO],
> +		ret = SetPIDs(av7110, av7110->pids[DMX_PES_VIDEO],
>  			av7110->pids[DMX_PES_AUDIO],
>  			av7110->pids[DMX_PES_TELETEXT], 0,
>  			av7110->pids[DMX_PES_PCR]);
> -		av7110_fw_cmd(av7110, COMTYPE_PIDFILTER, Scan, 0);
> +		if (!ret)
> +			ret = av7110_fw_cmd(av7110, COMTYPE_PIDFILTER, Scan, 0);
>  	} else {
> -		SetPIDs(av7110, 0, 0, 0, 0, 0);
> -		av7110_fw_cmd(av7110, COMTYPE_PID_FILTER, FlushTSQueue, 0);
> -		av7110_wait_msgstate(av7110, GPMQBusy);
> +		ret = SetPIDs(av7110, 0, 0, 0, 0, 0);
> +		if (!ret) {
> +			ret = av7110_fw_cmd(av7110, COMTYPE_PID_FILTER, FlushTSQueue, 0);
> +			if (!ret)
> +				ret = av7110_wait_msgstate(av7110, GPMQBusy);
> +		}
>  	}
>  

Add here:
	if (!ret)
		av7110->fe_synced = synced;

>  	up(&av7110->pid_mutex);
>
> +	return ret;
>  }





> diff -up dvb-cvs/dvb-kernel/linux/drivers/media/dvb/ttpci/av7110_hw.h linux/drivers/media/dvb/ttpci/av7110_hw.h
> --- dvb-cvs/dvb-kernel/linux/drivers/media/dvb/ttpci/av7110_hw.h	2005-04-24 00:46:40.000000000 +0200
> +++ linux/drivers/media/dvb/ttpci/av7110_hw.h	2005-06-18 00:30:09.000000000 +0200
> @@ -453,14 +453,14 @@ static inline void ARM_ClearIrq(struct a
>   * Firmware commands
>   ****************************************************************************/
>  
> -static inline int SendDAC(struct av7110 *av7110, u8 addr, u8 data)
> +static int inline SendDAC(struct av7110 *av7110, u8 addr, u8 data)

Hmm?

> -static inline void av7710_set_video_mode(struct av7110 *av7110, int mode)
> +static inline int av7710_set_video_mode(struct av7110 *av7110, int mode)
>
> ...
>
> -static inline void Set22K(struct av7110 *av7110, int state)
> +static int inline Set22K(struct av7110 *av7110, int state)

Either use 'inline int' or 'int inline'.

Imho we should keep the changes minimal.

Oliver

-- 
--------------------------------------------------------
VDR Remote Plugin available at
http://www.escape-edv.de/endriss/vdr/
--------------------------------------------------------