Em Wed, 18 Mar 2020 03:00:17 -0300
"Daniel W. S. Almeida" <dwlsalmeida@xxxxxxxxx> escreveu:
> From: "Daniel W. S. Almeida" <dwlsalmeida@xxxxxxxxx>
>
> Periodically check the signal quality and eventually lose the lock if
> the quality is sub-par. A fake tuner can return a bad quality signal to
> the demod if the frequency is too far off from a valid frequency.
>
> Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@xxxxxxxxx>
> ---
> drivers/media/dvb-frontends/dvb_dummy_fe.c | 149 ++++++++++++++++++++-
> 1 file changed, 144 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/dvb_dummy_fe.c b/drivers/media/dvb-frontends/dvb_dummy_fe.c
> index 9ff1ebaa5e04..726c964a523d 100644
> --- a/drivers/media/dvb-frontends/dvb_dummy_fe.c
> +++ b/drivers/media/dvb-frontends/dvb_dummy_fe.c
> @@ -9,24 +9,155 @@
> #include <linux/init.h>
> #include <linux/string.h>
> #include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <linux/random.h>
>
> #include <media/dvb_frontend.h>
> #include "dvb_dummy_fe.h"
>
>
> +struct dvb_dummy_fe_cnr_to_qual_s {
> + /* attempt to use the same values as libdvbv5 */
> + u32 modulation;
> + u32 fec;
> + u32 cnr_ok, cnr_good;
> +};
> +
> +struct dvb_dummy_fe_cnr_to_qual_s dvb_c_cnr_2_qual[] = {
> + /* from libdvbv5 source code */
> + { QAM_256, FEC_NONE, 34., 38.},
> + { QAM_64, FEC_NONE, 30., 34.},
> +};
> +
> +struct dvb_dummy_fe_cnr_to_qual_s dvb_s_cnr_2_qual[] = {
> + /* from libdvbv5 source code */
> + { QPSK, FEC_1_2, 7., 10.},
> +
> + { QPSK, FEC_2_3, 9., 12.},
> + { QPSK, FEC_3_4, 10., 13.},
> + { QPSK, FEC_5_6, 11., 14.},
> +
> + { QPSK, FEC_7_8, 12., 15.},
> +};
> +
> +struct dvb_dummy_fe_cnr_to_qual_s dvb_s2_cnr_2_qual[] = {
> + /* from libdvbv5 source code */
> + { QPSK, FEC_1_2, 9., 12.},
> + { QPSK, FEC_2_3, 11., 14.},
> + { QPSK, FEC_3_4, 12., 15.},
> + { QPSK, FEC_5_6, 12., 15.},
> + { QPSK, FEC_8_9, 13., 16.},
> + { QPSK, FEC_9_10, 13.5, 16.5},
> + { PSK_8, FEC_2_3, 14.5, 17.5},
> + { PSK_8, FEC_3_4, 16., 19.},
> + { PSK_8, FEC_5_6, 17.5, 20.5},
> + { PSK_8, FEC_8_9, 19., 22.},
> +};
> +
> +static struct dvb_dummy_fe_cnr_to_qual_s dvb_t_cnr_2_qual[] = {
> + /* from libdvbv5 source code */
> + { QPSK, FEC_1_2, 4.1, 5.9},
> + { QPSK, FEC_2_3, 6.1, 9.6},
> + { QPSK, FEC_3_4, 7.2, 12.4},
> + { QPSK, FEC_5_6, 8.5, 15.6},
> + { QPSK, FEC_7_8, 9.2, 17.5},
> +
> + { QAM_16, FEC_1_2, 9.8, 11.8},
> + { QAM_16, FEC_2_3, 12.1, 15.3},
> + { QAM_16, FEC_3_4, 13.4, 18.1},
> + { QAM_16, FEC_5_6, 14.8, 21.3},
> + { QAM_16, FEC_7_8, 15.7, 23.6},
> +
> + { QAM_64, FEC_1_2, 14.0, 16.0},
> + { QAM_64, FEC_2_3, 19.9, 25.4},
> + { QAM_64, FEC_3_4, 24.9, 27.9},
> + { QAM_64, FEC_5_6, 21.3, 23.3},
> + { QAM_64, FEC_7_8, 22.0, 24.0},
> +};
Same comment as before: multiply everything to 1000.
> +
> +struct dvb_dummy_fe_config {
> + /* probability of losing the lock due to low snr */
> + u8 drop_tslock_probability_on_low_snr;
> +};
> +
> struct dvb_dummy_fe_state {
> struct dvb_frontend frontend;
> + struct dvb_dummy_fe_config config;
> + struct delayed_work poll_snr;
> + enum fe_status status;
> };
>
> +void poll_snr_handler(struct work_struct *work)
> +{
> + /* periodically check the signal quality and eventually
> + * lose the TS lock if it dips too low
> + */
We use multi-line comments at the Kernel as:
/*
* foo
* bar
*/
> + struct dvb_dummy_fe_state *state =
> + container_of(work, struct dvb_dummy_fe_state, poll_snr.work);
> + struct dtv_frontend_properties *c = &state->frontend.dtv_property_cache;
> + struct dvb_dummy_fe_cnr_to_qual_s *cnr2qual = NULL;
> + struct dvb_dummy_fe_config *config = &state->config;
> + u32 array_size = 0;
> + u16 snr = 0;
> + u32 i;
Please avoid breaking assignments on multiple lines. It makes harder
to read.
What I would do, instead, is to split it on a different way:
struct dvb_dummy_fe_state *state;
struct dtv_frontend_properties *c;
struct dvb_dummy_fe_config *config;
...
state = container_of(work, struct dvb_dummy_fe_state, poll_snr.work);
c = &state->frontend.dtv_property_cache;
config = &state->config;
> +
> + if (!state->frontend.ops.tuner_ops.get_rf_strength)
> + return;
> +
> + state->frontend.ops.tuner_ops.get_rf_strength(&state->frontend, &snr);
> +
> + switch (c->delivery_system) {
> + case SYS_DVBT:
> + case SYS_DVBT2:
> + cnr2qual = dvb_t_cnr_2_qual;
> + array_size = ARRAY_SIZE(dvb_t_cnr_2_qual);
> + break;
> +
> + case SYS_DVBS:
> + cnr2qual = dvb_s_cnr_2_qual;
> + array_size = ARRAY_SIZE(dvb_s_cnr_2_qual);
> + break;
> +
> + case SYS_DVBS2:
> + cnr2qual = dvb_s2_cnr_2_qual;
> + array_size = ARRAY_SIZE(dvb_s2_cnr_2_qual);
> + break;
> +
> + case SYS_DVBC_ANNEX_A:
> + cnr2qual = dvb_c_cnr_2_qual;
> + array_size = ARRAY_SIZE(dvb_c_cnr_2_qual);
> + break;
> +
> + default:
> + pr_warn("%s: unsupported delivery system: %u\n",
> + __func__,
> + c->delivery_system);
> + break;
> + }
> +
> + for (i = 0; i <= array_size; i++) {
> + if (cnr2qual[i].modulation == c->modulation &&
> + cnr2qual[i].fec == c->fec_inner) {
> +
> + if (snr < cnr2qual[i].cnr_ok) {
> + /* eventually lose the TS lock */
> + if (prandom_u32_max(100) <
> + config->drop_tslock_probability_on_low_snr)
> + state->status = 0;
> + }
Hmm.. what about the reverse: if it lost TS lock, shouldn't it
randomly recover?
> + }
> + }
> +
> + schedule_delayed_work(&(state->poll_snr), msecs_to_jiffies(2000));
> +}
>
> static int dvb_dummy_fe_read_status(struct dvb_frontend *fe,
> enum fe_status *status)
> {
> - *status = FE_HAS_SIGNAL
> - | FE_HAS_CARRIER
> - | FE_HAS_VITERBI
> - | FE_HAS_SYNC
> - | FE_HAS_LOCK;
> +
> + struct dvb_dummy_fe_state *state = fe->demodulator_priv;
> +
> + *status = state->status;
>
> return 0;
> }
> @@ -80,11 +211,18 @@ static int dvb_dummy_fe_set_frontend(struct dvb_frontend *fe)
>
> static int dvb_dummy_fe_sleep(struct dvb_frontend *fe)
> {
> + struct dvb_dummy_fe_state *state = fe->demodulator_priv;
> +
> + cancel_delayed_work_sync(&(state->poll_snr));
> return 0;
> }
>
> static int dvb_dummy_fe_init(struct dvb_frontend *fe)
> {
> + struct dvb_dummy_fe_state *state = fe->demodulator_priv;
> +
> + INIT_DELAYED_WORK(&(state->poll_snr), &poll_snr_handler);
> + schedule_delayed_work(&(state->poll_snr), msecs_to_jiffies(2000));
> return 0;
> }
>
> @@ -104,6 +242,7 @@ static void dvb_dummy_fe_release(struct dvb_frontend *fe)
> {
> struct dvb_dummy_fe_state *state = fe->demodulator_priv;
>
> + cancel_delayed_work_sync(&(state->poll_snr));
> kfree(state);
> }
>
The rest of the code sounds good to me.
Thanks,
Mauro
