Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari <crope@iki.fi>]

On 07/27/2011 11:06 PM, Mauro Carvalho Chehab wrote:
> Em 27-07-2011 16:49, Antti Palosaari escreveu:
>> On 07/27/2011 10:06 PM, Mauro Carvalho Chehab wrote:
>>
>>>> +    for (i = 0; i<= x; i++) {
>>>> +        ret = adap->props.frontend_attach(adap);
>>>> +        if (ret || adap->fe[i] == NULL) {
>>>> +            /* only print error when there is no FE at all */
>>>> +            if (i == 0)
>>>> +                err("no frontend was attached by '%s'",
>>>> +                    adap->dev->desc->name);
>>>
>>> This doesn't seem right. One thing is to accept adap->fe[1] to be
>>> NULL. Another thing is to accept an error at the attach. IMO, the
>>> logic should be something like:
>>>
>>>      if (ret<   0)
>>>          return ret;
>>>
>>>      if (!i&&   !adap->fe[0]) {
>>>          err("no adapter!");
>>>          return -ENODEV;
>>>      }
>>
>> Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps
>> silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
>>
>> So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB
>> callbacks failing silently - like tuner_attach etc.
>>
>> Surely I want also fix many old issues but it is always too risky.
>
> I'm talking about adding another bad behavior: let MFE to have one of the FE's
> failing.

Let the device driver made decision! I can add check that if ret < 0 
then fail as fatal, but if there is ret=0 and fe=NULL is not fatal. But 
since it was discarded earlier I didnt added such check.

There is many devices have like 2 demod places on board. Then it is 
housed as demod#0 + demod#1, #demod0 or #demod1. You will not know that 
before trying to attach demod. Thus it *must* be possible to discard 
missing demod silently. Surely there is soon someone end user making bug 
reports if you print error on that case or even deregister whole device.

>>>> +        if (dvb_register_frontend(&adap->dvb_adap, adap->fe[i])) {
>>>> +            err("Frontend %d registration failed.", i);
>>>> +            dvb_frontend_detach(adap->fe[i]);
>>>
>>> There is a special case here: for DRX-K, we can't call dvb_frontend_detach().
>>> as just one drxk_attach() returns the two pointers. While this is not fixed,
>>> we need to add some logic here to check if the adapter were attached.
>>
>> There is currently no DVB USB driver using DRX-K thus it is not problem. If someone will add such support can fix DRX-K driver.
>
> I agree. I was just rising the question. The glue should be bound together with the first
> dvb-usb DRX-K driver.
>
>> Also take into account it is possible to register only 1 FE using DVB USB and manually register 2nd FE as it have been those days.
>>
>>
>>>> +            adap->fe[i] = NULL;
>>>> +            /* In error case, do not try register more FEs,
>>>> +             * still leaving already registered FEs alive. */
>>>
>>> I think that the proper thing to do is to detach everything, if one of
>>> the attach fails. There isn't much sense on keeping the device partially
>>> initialized.
>
>> It is not attach which fails here, it is dvb_register_frontend registration.
>
> Yes, sure. I've miss-named.
>
>> And I thought that too since it is rather major problem. But as it seems to be very easy to do it like that I did. Not big issue though.
>
> Core functions should be more strict than drivers. So, I think it should really
> enforce an all-or-nothing behavior here.

Lets add it later, like next merge window.

>>>> +        /* use exclusive FE lock if there is multiple shared FEs */
>>>> +        if (adap->fe[1])
>>>> +            adap->dvb_adap.mfe_shared = 1;
>>>
>>> Why? multiple FE doesn't mean that they're mutually exclusive.
>>
>> And again we ran same MFE discussion..... But if you look DVB USB you can see its main idea
>> have been to register multiple adapters in case of not exclusive frontends. That have been
>> most likely implemented like that from day-0 of DVB USB.
>>
>> Another point is that it is not that patch which adds support for MFE. It is old issue and
>> I am not responsible of it. See patch where this decision is made in year 2008:
>>
>> commit 59b1842da1c6f33ad2e8da82d3dfb3445751d964
>> Author: Darron Broad<darron@kewl.org>
>> Date:   Sat Oct 11 11:44:05 2008 -0300
>>
>>      V4L/DVB (9227): MFE: Add multi-frontend mutual exclusion
>>
>>
>> Why to change again old behaviour?
>>
>>
>> Could you now read all with care. As I pointed out almost all your findings are are decisions made *earlier*.
>>
>> Only thing which is not coming from oldies is that you suggest to unregister all frontends if one register fails.
>
> You miss-understood me. I'm not saying that we should drop support for mfe_shared flag.
> I'm just saying that dvb-usb core should not assume that a driver with more than one
> FE have them into a mutually exclusive configuration.
>
> If you look at dvb-core, the default for it is mfe_shared = 0. We should keep the same
> default for dvb-usb.
>
> In other words, it should be up to the drivers to change mfe_shared flag to 1, and not
> to the core.

Peace of cake. Anyhow, we still have 2 adapters for DVB USB for that and 
have been long than I can remember. So I cannot real see point of doing 
that.


regards
Antti


-- 
http://palosaari.fi/