[linux-dvb] [Review] Multiproto tree

Manu Abraham abraham.manu at gmail.com
Sun Nov 25 21:03:11 CET 2007


Oliver Endriss wrote:
> Hi,
> 
> now we had bad weather, and I had some time to review the code. ;-)
> 
> 

Had an unstable link last week, got fixed.

> General note
> ------------
> Obviously, the multiproto tree has not been updated from master for a
> very long time. When merging care must be taken that no regressions flow
> back to the main development tree.
> 
> For now I ignored all differences, except for:
> - frontend.h
> - dvb_frontend.[ch]
> - budget-ci.c
> - budget-av.c
> 
> 
> API extensions (frontend.h)
> ---------------------------
> looks fine
> 
> 
> Card drivers (budget-ci, budget-av)
> -----------------------------------
> I didn't check the details, but the extensions look ok.
> You might consider whether parts of the stb0899/stb6100 stuff could be
> factored out into a header file. See bsru6.h for an example.
> It would reduce the file size, and tables etc could be reused.
> 


I did visit this, but there result looked thus, the device is not specific to a tuner 
as it is, and in each of the card configurations, the tables do have some changes
It depends on the card manufacturer.

Thinking thus, i thought of moving all the tables to a header where, something 
such as stb0899_config.h where all the card specific tables are thrown in. This 
doesn't reduce any of the compiled object size, but it does bring in the advantage
that budget_ci.c, budget_av.c are not cluttered with large tables.

What do you think of moving all the config's to a public config file ?

> 
> dvb-core: dvb_frontend.c
> ------------------------
> fe->legacy is turned on/off by various ioctls:
> - FE_SET_FRONTEND, FE_GET_FRONTEND -> 1
> - DVBFE_SET_PARAMS -> 0/1
> - DVBFE_GET_PARAMS, DVBFE_GET_DELSYS, DVBFE_GET_INFO -> 0
> 
> fe->legacy controls how the frontend thread works.
> 
> Since the frontend device can be accessed by multiple readers,
> fe->legacy might be toggled in funny ways.
> This might confuse the fe thread or dvb_frontend_add_event. ;-(

Fixed.

 
> Imho ioctls should not set fe->legacy. All parameter conversions should
> be done within the ioctl. For example:
> - new driver: FE_SET_FRONTEND converts parms to new driver API
> - old driver: DVBFE_SET_PARAMS converts parms to old driver API
> 
> Then the fe thread can simply use the old driver interface
> (fe->legacy==1) or the new one (fe->legacy==0).
> 
> 
> The error msg should display the offending parameter:
> Instead of
> +               printk("%s: Unsupported FEC\n", __func__);
> you might use
> +               printk(KERN_ERR "%s: Unsupported FEC %x\n", __func__, new_fec);
> (same way for other conversion routines)

Done

> 
> 
> +       /* Legacy       */
> +       if (fe->legacy) {
> +               if (fe->ops.set_frontend)
> +                       fe->ops.set_frontend(fe, &fepriv->parameters);
> +       } else {
> +//             if ((dvbfe_sanity_check(fe) == 0)) {
> +                       /* Superseding  */
> +                       if (fe->ops.set_params)
> +                               fe->ops.set_params(fe, &fepriv->fe_params);
> +//             } else
> +//                     return -EINVAL;
> +       }
> 
> Sanity checks should be done in FE_SET_FRONTEND/DVBFE_SET_PARAMS ioctls.
> (See HG master where I added this some time ago.)
> Otherwise the application would not see an error status.

Since you already have a check, i guess i can drop the dvbfe_sanity_check()
  
>        /* don't actually do anything if we're in the LOSTLOCK state,
> -        * the frontend is set to FE_CAN_RECOVER, and the max_drift is 0 */
> -       if ((fepriv->state & FESTATE_LOSTLOCK) &&
> -           (fe->ops.info.caps & FE_CAN_RECOVER) && (fepriv->max_drift == 0)) {
> -               dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> -               return;
> +        * the frontend is set to FE_CAN_RECOVER, and the max_drift is 0
> +        */
> +       /* Legacy       */
> +       if (fe->legacy) {
> +               if ((fepriv->state & FESTATE_LOSTLOCK) && (fepriv->max_drift == 0)) {
> +                       if (fe->ops.get_frontend_algo)
> +                               if (fe->ops.get_frontend_algo(fe) == DVBFE_ALGO_RECOVERY)
> +                                       dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> +
> +                       return 0;
> +               }
> +       } else {
> +               if (fepriv->state & FESTATE_LOSTLOCK) {
> +                       if (fe->ops.get_frontend_algo) {
> +                               if ((fe->ops.get_frontend_algo(fe) == DVBFE_ALGO_RECOVERY) &&
> +                                   (fepriv->max_drift == 0)) {
> +
> +                                       dvb_frontend_swzigzag_update_delay(fepriv, s & DVBFE_HAS_LOCK);
> +                                       return 0;
> +                               }
> +                       }
> +               }
>         }
> 
> The 'if (fe->legacy)' branch looks broken:
> fe->ops.get_frontend_algo is most likely zero, so
> dvb_frontend_swzigzag_update_delay will not be called for old drivers.
> 

This one's fixed although i see no usage of FE_CAN_RECOVER which is used in the 
tree currently, rather than bogus usage

> Finally, I did a quick test with this tree, and it worked. ;-)
> - dvb-ttpci driver with DVB-S Rev 1.3 (ves1x93)
> - budget driver with DVB-S Nova Rev 1.0 (stv0299)
> - VDR (using old API)

Marco pointed to another bug a wrong copy, which breaks backward compatibility
for DVBFE_GET_PARAMS

Any further issues that you see ?

Additionally i did work upon the Multiple TS stuff, haven't got it working yet.
The concept works like this, unlike what we thought would be:

Manu,

The filtering is just like an IP address and subnet mask.
ISI_ENTRY equivalent to IP address.
IS_BIT_EN equivalent to subnet mask.
The incoming Bbheaders are compared to ISI_ENTRY through mask IS_BIT_EN.
Those Bbheaders that pass the test are allowed through to the TS.  Those
that do not are deleted.

 
> CU
> Oliver
> 

Thanks,
Manu



More information about the linux-dvb mailing list