Re: [PATCH v9 2/4] [media] cxusb: implement Medion MD95700 digital / analog coexistence

[Hans Verkuil]

On 4/24/19 10:02 PM, Maciej S. Szmigiero wrote:
> On 24.04.2019 14:45, Hans Verkuil wrote:
>> On 4/16/19 1:38 AM, Maciej S. Szmigiero wrote:
>>> 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@xxxxxxxxxxxxxxxxxxxxx>
>>>>> ---
> (..)
>>>>> 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.
>>
>> Is there anything wrong with always doing:
>>
>> 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.
> 
> In principle this should be fine - looking at the existing drivers
> doesn't reveal anything in their streaming_ctrl(1) callbacks that could
> depend on URBs submission order.

OK, then please switch to this in v11. I think this is a much cleaner
solution.

Sean, do you have cxusb hardware that you can use to test this when v11
is posted? Just to verify that nothing breaks.

Regards,

	Hans

> 
>> Regards,
>>
>> 	Hans
>>
> 
> Thanks,
> Maciej
>