* [PATCH] [media] dvb-frontends/mxl5xx: fix lock check order
@ 2017-08-20 10:45 Daniel Scheller
2017-08-27 12:18 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Scheller @ 2017-08-20 10:45 UTC (permalink / raw)
To: linux-media, mchehab, mchehab; +Cc: jasmin, rjkm
From: Daniel Scheller <d.scheller@gmx.net>
Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
---
When the mxl5xx driver together with the ddbridge glue gets merged ([1]),
this one should go in aswell - this fix is part of the dddvb-0.9.31
release.
drivers/media/dvb-frontends/mxl5xx.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/dvb-frontends/mxl5xx.c b/drivers/media/dvb-frontends/mxl5xx.c
index 676c96c216c3..d9136d67f5d4 100644
--- a/drivers/media/dvb-frontends/mxl5xx.c
+++ b/drivers/media/dvb-frontends/mxl5xx.c
@@ -638,13 +638,14 @@ static int tune(struct dvb_frontend *fe, bool re_tune,
state->tune_time = jiffies;
return 0;
}
- if (*status & FE_HAS_LOCK)
- return 0;
r = read_status(fe, status);
if (r)
return r;
+ if (*status & FE_HAS_LOCK)
+ return 0;
+
return 0;
}
--
2.13.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] [media] dvb-frontends/mxl5xx: fix lock check order
2017-08-20 10:45 [PATCH] [media] dvb-frontends/mxl5xx: fix lock check order Daniel Scheller
@ 2017-08-27 12:18 ` Mauro Carvalho Chehab
2017-08-27 12:50 ` Daniel Scheller
0 siblings, 1 reply; 3+ messages in thread
From: Mauro Carvalho Chehab @ 2017-08-27 12:18 UTC (permalink / raw)
To: Daniel Scheller; +Cc: linux-media, mchehab, jasmin, rjkm
Em Sun, 20 Aug 2017 12:45:45 +0200
Daniel Scheller <d.scheller.oss@gmail.com> escreveu:
> From: Daniel Scheller <d.scheller@gmx.net>
>
Always add a description at the patch.
> Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
> ---
> When the mxl5xx driver together with the ddbridge glue gets merged ([1]),
> this one should go in aswell - this fix is part of the dddvb-0.9.31
> release.
>
> drivers/media/dvb-frontends/mxl5xx.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/mxl5xx.c b/drivers/media/dvb-frontends/mxl5xx.c
> index 676c96c216c3..d9136d67f5d4 100644
> --- a/drivers/media/dvb-frontends/mxl5xx.c
> +++ b/drivers/media/dvb-frontends/mxl5xx.c
> @@ -638,13 +638,14 @@ static int tune(struct dvb_frontend *fe, bool re_tune,
> state->tune_time = jiffies;
> return 0;
> }
> - if (*status & FE_HAS_LOCK)
> - return 0;
>
> r = read_status(fe, status);
> if (r)
> return r;
>
> + if (*status & FE_HAS_LOCK)
> + return 0;
> +
> return 0;
That's stupid! it will return 0 on all situations, no matter if FE_HAS_LOCK
or not. So, no need for the if.
Anyway, IMHO, either the original code is right and it needs to
use a previously cached value (with sounds weird to me, although
it is possible), or the logic is utterly broken, and we should,
instead, apply the enclosed patch.
> if (r)
> return r;
> }
>
Thanks,
Mauro
[PATCH RFC] media: mxl5xx: fix tuning logic
The tuning logic is broken with regards to status report:
it relies on a previously-cached value that may not be valid
if retuned.
Change the logic to always read the status.
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
diff --git a/drivers/media/dvb-frontends/mxl5xx.c b/drivers/media/dvb-frontends/mxl5xx.c
index 676c96c216c3..4b449a6943c5 100644
--- a/drivers/media/dvb-frontends/mxl5xx.c
+++ b/drivers/media/dvb-frontends/mxl5xx.c
@@ -636,16 +636,9 @@ static int tune(struct dvb_frontend *fe, bool re_tune,
if (r)
return r;
state->tune_time = jiffies;
- return 0;
}
- if (*status & FE_HAS_LOCK)
- return 0;
- r = read_status(fe, status);
- if (r)
- return r;
-
- return 0;
+ return read_status(fe, status);
}
static enum fe_code_rate conv_fec(enum MXL_HYDRA_FEC_E fec)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] [media] dvb-frontends/mxl5xx: fix lock check order
2017-08-27 12:18 ` Mauro Carvalho Chehab
@ 2017-08-27 12:50 ` Daniel Scheller
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Scheller @ 2017-08-27 12:50 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, mchehab, jasmin, rjkm
Am Sun, 27 Aug 2017 09:18:07 -0300
schrieb Mauro Carvalho Chehab <mchehab@s-opensource.com>:
Thanks for looking at this.
> Em Sun, 20 Aug 2017 12:45:45 +0200
> Daniel Scheller <d.scheller.oss@gmail.com> escreveu:
>
> > From: Daniel Scheller <d.scheller@gmx.net>
> >
>
> Always add a description at the patch.
Sorry. Will remember to do so even for these rather self-explanatory
changes.
> > Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
> > ---
> > When the mxl5xx driver together with the ddbridge glue gets merged
> > ([1]), this one should go in aswell - this fix is part of the
> > dddvb-0.9.31 release.
> >
> > drivers/media/dvb-frontends/mxl5xx.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/dvb-frontends/mxl5xx.c
> > b/drivers/media/dvb-frontends/mxl5xx.c index
> > 676c96c216c3..d9136d67f5d4 100644 ---
> > a/drivers/media/dvb-frontends/mxl5xx.c +++
> > b/drivers/media/dvb-frontends/mxl5xx.c @@ -638,13 +638,14 @@ static
> > int tune(struct dvb_frontend *fe, bool re_tune, state->tune_time =
> > jiffies; return 0;
> > }
> > - if (*status & FE_HAS_LOCK)
> > - return 0;
> >
> > r = read_status(fe, status);
> > if (r)
> > return r;
> >
> > + if (*status & FE_HAS_LOCK)
> > + return 0;
> > +
> > return 0;
>
> That's stupid! it will return 0 on all situations, no matter if
> FE_HAS_LOCK or not. So, no need for the if.
>
> Anyway, IMHO, either the original code is right and it needs to
> use a previously cached value (with sounds weird to me, although
> it is possible), or the logic is utterly broken, and we should,
> instead, apply the enclosed patch.
Well, in fact, this change was straight picked from upstream ([1]), but
I honestly didn't take into account that during cleanup the
#if-0/#endif was removed (which will in turn result in the same
behaviour though - return 0 in all cases; some leftovers?).
[1]
https://github.com/DigitalDevices/dddvb/commit/a44dbc889b7030a100599d2bd2c93c976c011a03
>
> > if (r)
> > return r;
>
> > }
> >
>
> Thanks,
> Mauro
>
> [PATCH RFC] media: mxl5xx: fix tuning logic
>
> The tuning logic is broken with regards to status report:
> it relies on a previously-cached value that may not be valid
> if retuned.
>
> Change the logic to always read the status.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>
>
> diff --git a/drivers/media/dvb-frontends/mxl5xx.c
> b/drivers/media/dvb-frontends/mxl5xx.c index
> 676c96c216c3..4b449a6943c5 100644 ---
> a/drivers/media/dvb-frontends/mxl5xx.c +++
> b/drivers/media/dvb-frontends/mxl5xx.c @@ -636,16 +636,9 @@ static
> int tune(struct dvb_frontend *fe, bool re_tune, if (r)
> return r;
> state->tune_time = jiffies;
> - return 0;
> }
> - if (*status & FE_HAS_LOCK)
> - return 0;
>
> - r = read_status(fe, status);
> - if (r)
> - return r;
> -
> - return 0;
> + return read_status(fe, status);
> }
>
> static enum fe_code_rate conv_fec(enum MXL_HYDRA_FEC_E fec)
>
Indeed that makes more sense in this context (compared to dddvb
upstream), albeit on re_tune=1 read_status() will now also be called,
which I guess is totally fine. Let's do it this way. So:
Acked-by: Daniel Scheller <d.scheller@gmx.net>
Thanks & best regards,
Daniel Scheller
--
https://github.com/herrnst
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-08-27 12:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-20 10:45 [PATCH] [media] dvb-frontends/mxl5xx: fix lock check order Daniel Scheller
2017-08-27 12:18 ` Mauro Carvalho Chehab
2017-08-27 12:50 ` Daniel Scheller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.