Date:   Tue, 16 Apr 2019 01:38:38 +0200
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Michael Krufky <mkrufky@...uxtv.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Andy Walls <awalls@...metrocast.net>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-media@...r.kernel.org
Subject: Re: [PATCH v9 2/4] [media] cxusb: implement Medion MD95700 digital /
 analog coexistence

On 12.04.2019 10:56, Hans Verkuil wrote:
> On 3/30/19 12:51 AM, Maciej S. Szmigiero wrote:
>> This patch prepares cxusb driver for supporting the analog part of
>> Medion 95700 (previously only the digital - DVB - mode was supported).
>>
>> Specifically, it adds support for:
>> * switching the device between analog and digital modes of operation,
>> * enforcing that only one mode is active at the same time due to hardware
>> limitations.
>>
>> Actual implementation of the analog mode will be provided by the next
>> commit.
>>
>> Signed-off-by: Maciej S. Szmigiero <mail@...iej.szmigiero.name>
>> ---
(..)
>> @@ -259,7 +293,7 @@ static int cxusb_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
>>  
>>  static u32 cxusb_i2c_func(struct i2c_adapter *adapter)
>>  {
>> -	return I2C_FUNC_I2C;
>> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> 
> Was this change needed for the medion? While it is probably right, I am hesitant to
> apply such a change in case it will break existing boards.

The cx25840 driver checks for these I2C functions at the probe time and
returns -EIO if its i2c host doesn't support them.

The early version of this patch actually removed this SMBus check from
the cx25840 driver, however it was decided that adding such extra
capability to the cxusb driver is actually safer, see the end of this
message:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg37363.html

>> index 8056053c9ab0..01987ec5e0c5 100644
>> --- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
>> +++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
>> @@ -15,6 +15,7 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed *dvbdmxfeed, int onoff)
>>  {
>>  	struct dvb_usb_adapter *adap = dvbdmxfeed->demux->priv;
>>  	int newfeedcount, ret;
>> +	bool streaming_ctrl_no_urb;
>>  
>>  	if (adap == NULL)
>>  		return -ENODEV;
>> @@ -24,12 +25,16 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed *dvbdmxfeed, int onoff)
>>  		return -EINVAL;
>>  	}
>>  
>> +	streaming_ctrl_no_urb = adap->props.fe[adap->active_fe].caps &
>> +		DVB_USB_ADAP_STREAMING_CTRL_NO_URB;
>>  	newfeedcount = adap->feedcount + (onoff ? 1 : -1);
>>  
>>  	/* stop feed before setting a new pid if there will be no pid anymore */
>>  	if (newfeedcount == 0) {
>>  		deb_ts("stop feeding\n");
>> -		usb_urb_kill(&adap->fe_adap[adap->active_fe].stream);
>> +
>> +		if (streaming_ctrl_no_urb)
> 
> Is this test right? Shouldn't it be !streaming_ctrl_no_urb in order to keep the
> current (non-medion) behavior?
> 
>> +			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);
>> @@ -38,6 +43,9 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed *dvbdmxfeed, int onoff)
>>  				return ret;
>>  			}
>>  		}
>> +
>> +		if (!streaming_ctrl_no_urb)
> 
> And then this would have to be inverted as well.

The newly added flag "DVB_USB_ADAP_STREAMING_CTRL_NO_URB" has the
following meaning:
If it is set the order of operations in dvb_usb_ctrl_feed() looks like
this:
1) Call the driver streaming_ctrl(1) callback to enable streaming,
2) Submit DVB data URBs,
(streaming happens)
3) Kill DVB data URBs,
4) Call the driver streaming_ctrl(0) callback to disable streaming.

This is needed for Medion because:
a) The device could already be open in the analog mode when
streaming_ctrl(1) tries to acquire it in the digital mode.
In this case it is important that no DVB URBs are scheduled before
giving the callback chance to report an error,

b) The device could get open in the analog mode as soon as
streaming_ctrl(0) drops the last digital mode reference to it so all
DVB data URB must have already been terminated at this point.

If the aforementioned flag is unset, however, the order of operations in
dvb_usb_ctrl_feed() looks like this:
1) Submit DVB data URBs,
2) Call the driver streaming_ctrl(1) callback to enable streaming,
(streaming happens)
3) Call the driver streaming_ctrl(0) callback to disable streaming,
4) Kill DVB data URBs.

Previously, the order was always like this:
1) Submit DVB data URBs,
2) Call the driver streaming_ctrl(1) callback to enable streaming,
(streaming happens)
3) Kill DVB data URBs,
4) Call the driver streaming_ctrl(0) callback to disable streaming.

You can see that this was asymmetric - streaming_ctrl(1) is called with
data URBs already active but they are killed before streaming_ctrl(0)
gets called, so switching only the submit part on "STREAMING_CTRL_NO_URB"
would make this flag operation only half-correct.

I have audited existing drivers that use dvb-usb framework and none of
them do anything besides a synchronous USB control transfer or a
synchronous USB bulk transfer to an endpoint different from the one that
DVB data uses in streaming_ctrl(0) callback.
Additionally, streaming_ctrl(1) and streaming_ctrl(0) paths are usually
very similar in the driver code, so if streaming_ctrl(1) runs fine with
data URBs being already active there is no reason to think
streaming_ctrl(0) will have a problem with this.

Note that DVB data URBs are handled fully inside the dvb-usb framework
and individual drivers don't access them.

Furthermore, in case the streaming_ctrl(1) callback fails the
dvb_usb_ctrl_feed() function returns an error to its caller while
leaving the already-submited URBs still active.
They will then endlessly resubmit themselves in their completion
callback.
This further points out that streaming_ctrl() callback is not considered
of need to be strongly ordered with respect to the DVB data URBs
submission or cancellation.

>> +			usb_urb_kill(&adap->fe_adap[adap->active_fe].stream);
>>  	}
>>  
>>  	adap->feedcount = newfeedcount;
>> @@ -56,8 +64,10 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed *dvbdmxfeed, int onoff)
>>  	 * for reception.
>>  	 */
>>  	if (adap->feedcount == onoff && adap->feedcount > 0) {
>> -		deb_ts("submitting all URBs\n");
>> -		usb_urb_submit(&adap->fe_adap[adap->active_fe].stream);
>> +		if (!streaming_ctrl_no_urb) {
>> +			deb_ts("submitting all URBs early\n");
>> +			usb_urb_submit(&adap->fe_adap[adap->active_fe].stream);
>> +		}
>>  
>>  		deb_ts("controlling pid parser\n");
>>  		if (adap->props.fe[adap->active_fe].caps & DVB_USB_ADAP_HAS_PID_FILTER &&
>> @@ -80,6 +90,10 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed *dvbdmxfeed, int onoff)
>>  			}
>>  		}
>>  
>> +		if (streaming_ctrl_no_urb) {
>> +			deb_ts("submitting all URBs late\n");
>> +			usb_urb_submit(&adap->fe_adap[adap->active_fe].stream);
>> +		}
>>  	}
>>  	return 0;
>>  }


> 
> Regards,
> 
> 	Hans
> 

Thanks and best regards,
Maciej

Powered by blists - more mailing lists