dvb-frontends/mxl5xx: fix lock check order
Subsystems (2)
PATCH 1/1 dvb-frontends/mxl5xx: fix lock check order
From: Daniel Scheller <[email protected]>
---
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(-)
drivers/media/dvb-frontends/mxl5xx.c +3 -2
@@ -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 ; }
Signature · 1 line
2.13.0
Em Sun, 20 Aug 2017 12:45:45 +0200
Daniel Scheller <[email protected]> escreveu:
Quoted 2 lines
From: Daniel Scheller <[email protected]>
Always add a description at the patch.
Quoted 18 lines
Signed-off-by Daniel Scheller <[email protected]>---
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(-)drivers/media/dvb-frontends/mxl5xx.c +0
-1@@ -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 )
drivers/media/dvb-frontends/mxl5xx.c static int tune(struct dvb_frontend *fe, bool re_tune, · line 649
Quoted 10 lines
drivers/media/dvb-frontends/mxl5xx.c +3
-1-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.
Quoted 5 lines
drivers/media/dvb-frontends/mxl5xx.c +0
-0if (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.
drivers/media/dvb-frontends/mxl5xx.c +1 -8
@@ -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)
Daniel Scheller
Acked-byAm Sun, 27 Aug 2017 09:18:07 -0300
schrieb Mauro Carvalho Chehab <[email protected]>:
Thanks for looking at this.
Quoted 7 lines
Em Sun, 20 Aug 2017 12:45:45 +0200
Daniel Scheller <[email protected]> escreveu:Quoted 2 lines
From: Daniel Scheller <[email protected]>
Always add a description at the patch.
Sorry. Will remember to do so even for these rather self-explanatory changes.
Quoted 32 lines
Quoted 18 lines
Signed-off-by Daniel Scheller <[email protected]>---
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(-)(quoted diff) +0
-0
b/drivers/media/dvb-frontends/mxl5xx.cindex676c96c216c3..d9136d67f5d4100644 ---a/drivers/media/dvb-frontends/mxl5xx.c+++b/drivers/media/dvb-frontends/mxl5xx.c@@ -638,13 +638,14 @@ static int tune(structdvb_frontend*fe, boolre_tune,state->tune_time= jiffies; return 0;(quoted diff) +0
-1} - if (*status & FE_HAS_LOCK)Review
Quoted 10 lines
(quoted diff) +3
-1- 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_LOCKor not. So, no need for the if.
Quoted 4 lines
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
Quoted 34 lines
drivers/media/dvb-frontends/mxl5xx.c static int tune(struct dvb_frontend *fe, bool re_tune, · line 642
Quoted 6 lines
drivers/media/dvb-frontends/mxl5xx.c +0
-0if (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 <[email protected]>(quoted diff) +0
-0
b/drivers/media/dvb-frontends/mxl5xx.cindex676c96c216c3..4b449a6943c5100644 ---a/drivers/media/dvb-frontends/mxl5xx.c+++b/drivers/media/dvb-frontends/mxl5xx.c@@ -636,16 +636,9 @@ static int tune(structdvb_frontend*fe, boolre_tune, if (r)drivers/media/dvb-frontends/mxl5xx.c +0
-3return r; state->tune_time = jiffies; -return 0 ; } -if (*status &FE_HAS_LOCK ) -return 0 ;
drivers/media/dvb-frontends/mxl5xx.c static int tune(struct dvb_frontend *fe, bool re_tune, · line 644
Quoted 10 lines
drivers/media/dvb-frontends/mxl5xx.c +1
-5- 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:
Thanks & best regards,
Daniel Scheller
Signature · 1 line
https://github.com/herrnst