diff mbox

[media] tuners: One check less in m88rs6000t_get_rf_strength() after error detection

Message ID 5680FDB3.7060305@users.sourceforge.net (mailing list archive)
State New, archived
Headers show

Commit Message

SF Markus Elfring Dec. 28, 2015, 9:15 a.m. UTC 
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 10:10:34 +0100

This issue was detected by using the Coccinelle software.

Move the jump label directly before the desired log statement
so that the variable "ret" will not be checked once more
after it was determined that a function call failed.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/tuners/m88rs6000t.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Julia Lawall Dec. 28, 2015, 9:20 a.m. UTC | #1 
On Mon, 28 Dec 2015, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 28 Dec 2015 10:10:34 +0100
>
> This issue was detected by using the Coccinelle software.
>
> Move the jump label directly before the desired log statement
> so that the variable "ret" will not be checked once more
> after it was determined that a function call failed.

Why not avoid both unnecessary ifs and the enormous ugliness of a label
inside an if by making two returns: a return 0 for success and a dev_dbg
and return ret for failure?

julia


> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/tuners/m88rs6000t.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
> index 504bfbc..b45594e 100644
> --- a/drivers/media/tuners/m88rs6000t.c
> +++ b/drivers/media/tuners/m88rs6000t.c
> @@ -510,27 +510,27 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
>
>  	ret = regmap_read(dev->regmap, 0x5A, &val);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	RF_GC = val & 0x0f;
>
>  	ret = regmap_read(dev->regmap, 0x5F, &val);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	IF_GC = val & 0x0f;
>
>  	ret = regmap_read(dev->regmap, 0x3F, &val);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	TIA_GC = (val >> 4) & 0x07;
>
>  	ret = regmap_read(dev->regmap, 0x77, &val);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	BB_GC = (val >> 4) & 0x0f;
>
>  	ret = regmap_read(dev->regmap, 0x76, &val);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	PGA2_GC = val & 0x3f;
>  	PGA2_cri = PGA2_GC >> 2;
>  	PGA2_crf = PGA2_GC & 0x03;
> @@ -562,9 +562,11 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
>  	/* scale value to 0x0000-0xffff */
>  	gain = clamp_val(gain, 1000U, 10500U);
>  	*strength = (10500 - gain) * 0xffff / (10500 - 1000);
> -err:
> -	if (ret)
> +
> +	if (ret) {
> +report_failure:
>  		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> +	}
>  	return ret;
>  }
>
> --
> 2.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Dec. 28, 2015, 10:30 a.m. UTC | #2 
>> Move the jump label directly before the desired log statement
>> so that the variable "ret" will not be checked once more
>> after it was determined that a function call failed.
> 
> Why not avoid both unnecessary ifs

I would find such a fine-tuning also nice in principle at more source code places.


> and the enormous ugliness of a label inside an if by making two returns:
> a return 0 for success and a dev_dbg and return ret for failure?

How should your suggestion finally work when the desired execution success
can be determined for such functions only after several other calls succeeded?

Is consistent checking of failure predicates usually required?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall Dec. 28, 2015, 10:36 a.m. UTC | #3 
On Mon, 28 Dec 2015, SF Markus Elfring wrote:

> >> Move the jump label directly before the desired log statement
> >> so that the variable "ret" will not be checked once more
> >> after it was determined that a function call failed.
> >
> > Why not avoid both unnecessary ifs
>
> I would find such a fine-tuning also nice in principle at more source code places.
>
>
> > and the enormous ugliness of a label inside an if by making two returns:
> > a return 0 for success and a dev_dbg and return ret for failure?
>
> How should your suggestion finally work when the desired execution success
> can be determined for such functions only after several other calls succeeded?

Not idea what this means, but immediate return 0 followed by various code
for reacting to an error is very common, so it looks like it should be
possible here.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Dec. 28, 2015, 2:36 p.m. UTC | #4 
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 15:32:20 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Better exception handling in five functions
  Refactoring for m88rs6000t_sleep()

 drivers/media/tuners/m88rs6000t.c | 165 +++++++++++++++++++-------------------
 1 file changed, 83 insertions(+), 82 deletions(-)
diff mbox

Patch

diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
index 504bfbc..b45594e 100644
--- a/drivers/media/tuners/m88rs6000t.c
+++ b/drivers/media/tuners/m88rs6000t.c
@@ -510,27 +510,27 @@  static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
 
 	ret = regmap_read(dev->regmap, 0x5A, &val);
 	if (ret)
-		goto err;
+		goto report_failure;
 	RF_GC = val & 0x0f;
 
 	ret = regmap_read(dev->regmap, 0x5F, &val);
 	if (ret)
-		goto err;
+		goto report_failure;
 	IF_GC = val & 0x0f;
 
 	ret = regmap_read(dev->regmap, 0x3F, &val);
 	if (ret)
-		goto err;
+		goto report_failure;
 	TIA_GC = (val >> 4) & 0x07;
 
 	ret = regmap_read(dev->regmap, 0x77, &val);
 	if (ret)
-		goto err;
+		goto report_failure;
 	BB_GC = (val >> 4) & 0x0f;
 
 	ret = regmap_read(dev->regmap, 0x76, &val);
 	if (ret)
-		goto err;
+		goto report_failure;
 	PGA2_GC = val & 0x3f;
 	PGA2_cri = PGA2_GC >> 2;
 	PGA2_crf = PGA2_GC & 0x03;
@@ -562,9 +562,11 @@  static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
 	/* scale value to 0x0000-0xffff */
 	gain = clamp_val(gain, 1000U, 10500U);
 	*strength = (10500 - gain) * 0xffff / (10500 - 1000);
-err:
-	if (ret)
+
+	if (ret) {
+report_failure:
 		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+	}
 	return ret;
 }