Re: DVBv5 test report

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 23-01-2012 14:23, Antti Palosaari escreveu:
> On 01/19/2012 03:31 PM, Mauro Carvalho Chehab wrote:
>> [PATCH] dvb-usb: Don't abort stop on -EAGAIN/-EINTR
>>
>> Note: this patch is not complete. if the DVB demux device is opened on
>> block mode, it should instead be returning -EAGAIN.
>>
>> Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com>
>>
>> diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
>> index ddf282f..215ce75 100644
>> --- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
>> +++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
>> @@ -30,7 +30,9 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed *dvbdmxfeed, int onoff)
>>           usb_urb_kill(&adap->fe_adap[adap->active_fe].stream);
>>
>>           if (adap->props.fe[adap->active_fe].streaming_ctrl != NULL) {
>> -            ret = adap->props.fe[adap->active_fe].streaming_ctrl(adap, 0);
>> +            do {
>> +                ret = adap->props.fe[adap->active_fe].streaming_ctrl(adap, 0);
>> +            } while ((ret == -EAGAIN) || (ret == -EINTR));
>>               if (ret<  0) {
>>                   err("error while stopping stream.");
>>                   return ret;
>>
> 
> That fixes it. But it loops do {...} while around 100 times every I stop zap. Over 100 times is rather much...

Yes, this sounds too much. 

The issue here is caused by the usage of mutex_lock_interruptible() inside the
streaming_ctrl() callbacks, when the stream should stop.

The new wait_queue wakeup inside the code made the issue more visible, but it
could still happen without it, as a break could be hit during stream_ctl()
stop call anyway.

There are two possible fixes for it:

1) The above solution.

Eventually, a schedule() could be added there:
            do {
                ret = adap->props.fe[adap->active_fe].streaming_ctrl(adap, 0);
		if (ret == -EINTR)
			shedule();
            } while (ret == -EINTR);

2)

Don't use mutex_lock_interruptible inside the driver's streaming_ctrl, 
if the second parameter is 0 (stop).


IMHO, (1) is cleaner, due to a few reasons:

	- inside the drivers, the code will be symmetrical: it will call the same function for
both onoff = 1 and onoff = 0.

	- The patch is on just one place;

	- with (2), extra care is needed when merging patches, as regressions and
broken drivers could pass unnoticed.

> 
> And I think -EINTR is the only code to look, -EAGAIN is maybe for I2C and can be switched to native -EINTR also.

Drivers need to be checked, if only -EINTR is added there, as drivers may
be doing things like:

	if (mutex_lock_interruptible(...))
		return -EAGAIN;

Regards,
Mauro