[PATCH] [media] si2165: Refactoring for si2165_writereg_mask8()

[PATCH] [media] si2165: Refactoring for si2165_writereg_mask8()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 27 Dec 2015 18:23:57 +0100

This issue was detected by using the Coccinelle software.

1. Let us return directly if a call of the si2165_readreg8()
   function failed.

2. Reduce the scope for the local variables "ret" and "tmp" to one branch
   of an if statement.

3. Delete the jump label "err" then.

4. Return the value from a call of the si2165_writereg8() function
   without using an extra assignment for the variable "ret" at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/dvb-frontends/si2165.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/media/dvb-frontends/si2165.c b/drivers/media/dvb-frontends/si2165.c
index 2b93241..e8518ae 100644
--- a/drivers/media/dvb-frontends/si2165.c
+++ b/drivers/media/dvb-frontends/si2165.c
@@ -225,22 +225,18 @@ static int si2165_writereg32(struct si2165_state *state, const u16 reg, u32 val)
 static int si2165_writereg_mask8(struct si2165_state *state, const u16 reg,
 				 u8 val, u8 mask)
 {
-	int ret;
-	u8 tmp;
-
 	if (mask != 0xff) {
-		ret = si2165_readreg8(state, reg, &tmp);
+		u8 tmp;
+		int ret = si2165_readreg8(state, reg, &tmp);
+
 		if (ret < 0)
-			goto err;
+			return ret;
 
 		val &= mask;
 		tmp &= ~mask;
 		val |= tmp;
 	}
-
-	ret = si2165_writereg8(state, reg, val);
-err:
-	return ret;
+	return si2165_writereg8(state, reg, val);
 }
 
 #define REG16(reg, val) { (reg), (val) & 0xff }, { (reg)+1, (val)>>8 & 0xff }
-- 
2.6.3

[PATCH] [media] bttv: Returning only value constants in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 27 Dec 2015 22:02:21 +0100

Return constant integer values without storing them in the local
variable "err" or "rc".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/pci/bt8xx/bttv-driver.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index 9400e99..cd7d6ef 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -1726,22 +1726,15 @@ static int bttv_s_std(struct file *file, void *priv, v4l2_std_id id)
 	struct bttv_fh *fh  = priv;
 	struct bttv *btv = fh->btv;
 	unsigned int i;
-	int err = 0;
 
 	for (i = 0; i < BTTV_TVNORMS; i++)
 		if (id & bttv_tvnorms[i].v4l2_id)
 			break;
-	if (i == BTTV_TVNORMS) {
-		err = -EINVAL;
-		goto err;
-	}
-
+	if (i == BTTV_TVNORMS)
+		return -EINVAL;
 	btv->std = id;
 	set_tvnorm(btv, i);
-
-err:
-
-	return err;
+	return 0;
 }
 
 static int bttv_g_std(struct file *file, void *priv, v4l2_std_id *id)
@@ -1770,12 +1763,9 @@ static int bttv_enum_input(struct file *file, void *priv,
 {
 	struct bttv_fh *fh = priv;
 	struct bttv *btv = fh->btv;
-	int rc = 0;
 
-	if (i->index >= bttv_tvcards[btv->c.type].video_inputs) {
-		rc = -EINVAL;
-		goto err;
-	}
+	if (i->index >= bttv_tvcards[btv->c.type].video_inputs)
+		return -EINVAL;
 
 	i->type     = V4L2_INPUT_TYPE_CAMERA;
 	i->audioset = 0;
@@ -1799,10 +1789,7 @@ static int bttv_enum_input(struct file *file, void *priv,
 	}
 
 	i->std = BTTV_NORMS;
-
-err:
-
-	return rc;
+	return 0;
 }
 
 static int bttv_g_input(struct file *file, void *priv, unsigned int *i)
-- 
2.6.3

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

[SF Markus Elfring]

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(-)

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

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

[Julia Lawall]

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
>

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

[SF Markus Elfring]

>> 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

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

[Julia Lawall]

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

[PATCH 0/2] [media] m88rs6000t: Fine-tuning for some function implementations

[SF Markus Elfring]

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(-)

-- 
2.6.3

[PATCH 1/2] [media] m88rs6000t: Better exception handling in five functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 15:10:30 +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 a function call.
Use the identifier "report_failure" instead of "err".

Suggested-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/tuners/m88rs6000t.c | 154 +++++++++++++++++++-------------------
 1 file changed, 78 insertions(+), 76 deletions(-)

diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
index 504bfbc..7e59a9f 100644
--- a/drivers/media/tuners/m88rs6000t.c
+++ b/drivers/media/tuners/m88rs6000t.c
@@ -44,7 +44,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
 	/* select demod main mclk */
 	ret = regmap_read(dev->regmap, 0x15, &utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 	reg15 = utmp;
 	if (c->symbol_rate > 45010000) {
 		reg11 = 0x0E;
@@ -106,7 +106,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
 
 	ret = regmap_read(dev->regmap, 0x1D, &utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 	reg1D = utmp;
 	reg1D &= ~0x03;
 	reg1D |= N - 1;
@@ -116,42 +116,42 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
 	/* program and recalibrate demod PLL */
 	ret = regmap_write(dev->regmap, 0x05, 0x40);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x11, 0x08);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x15, reg15);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x16, reg16);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x1D, reg1D);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x1E, reg1E);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x1F, reg1F);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x17, 0xc1);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x17, 0x81);
 	if (ret)
-		goto err;
+		goto report_failure;
 	usleep_range(5000, 50000);
 	ret = regmap_write(dev->regmap, 0x05, 0x00);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x11, reg11);
 	if (ret)
-		goto err;
+		goto report_failure;
 	usleep_range(5000, 50000);
-err:
-	if (ret)
-		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+	return 0;
+report_failure:
+	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
 	return ret;
 }
 
@@ -169,13 +169,13 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
 
 	ret = regmap_write(dev->regmap, 0x36, (refDiv - 8));
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x31, 0x00);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x2c, 0x02);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	if (tuner_freq_MHz >= 1550) {
 		ucLoDiv1 = 2;
@@ -227,105 +227,105 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
 	reg27 = (((ulNDiv1 >> 8) & 0x0F) + ucLomod1) & 0x7F;
 	ret = regmap_write(dev->regmap, 0x27, reg27);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x28, (u8)(ulNDiv1 & 0xFF));
 	if (ret)
-		goto err;
+		goto report_failure;
 	reg29 = (((ulNDiv2 >> 8) & 0x0F) + ucLomod2) & 0x7f;
 	ret = regmap_write(dev->regmap, 0x29, reg29);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x2a, (u8)(ulNDiv2 & 0xFF));
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x2F, 0xf5);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x30, 0x05);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x08, 0x1f);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x08, 0x3f);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x09, 0x20);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x09, 0x00);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x3e, 0x11);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x08, 0x2f);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x08, 0x3f);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x09, 0x10);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x09, 0x00);
 	if (ret)
-		goto err;
+		goto report_failure;
 	usleep_range(2000, 50000);
 
 	ret = regmap_read(dev->regmap, 0x42, &utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 	reg42 = utmp;
 
 	ret = regmap_write(dev->regmap, 0x3e, 0x10);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x08, 0x2f);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x08, 0x3f);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x09, 0x10);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x09, 0x00);
 	if (ret)
-		goto err;
+		goto report_failure;
 	usleep_range(2000, 50000);
 
 	ret = regmap_read(dev->regmap, 0x42, &utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 	reg42buf = utmp;
 	if (reg42buf < reg42) {
 		ret = regmap_write(dev->regmap, 0x3e, 0x11);
 		if (ret)
-			goto err;
+			goto report_failure;
 	}
 	usleep_range(5000, 50000);
 
 	ret = regmap_read(dev->regmap, 0x2d, &utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x2d, utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_read(dev->regmap, 0x2e, &utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x2e, utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	ret = regmap_read(dev->regmap, 0x27, &utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 	reg27 = utmp & 0x70;
 	ret = regmap_read(dev->regmap, 0x83, &utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 	if (reg27 == (utmp & 0x70)) {
 		ucLoDiv	= ucLoDiv1;
 		ulNDiv = ulNDiv1;
@@ -340,7 +340,7 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
 		refDiv = 18;
 		ret = regmap_write(dev->regmap, 0x36, (refDiv - 8));
 		if (ret)
-			goto err;
+			goto report_failure;
 		ulNDiv = ((tuner_freq_MHz * ucLoDiv * 1000) * refDiv
 				/ fcry_KHz - 1024) / 2;
 	}
@@ -349,16 +349,16 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
 			+ ((ulNDiv >> 8) & 0x0F)) & 0xFF;
 	ret = regmap_write(dev->regmap, 0x27, reg27);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x28, (u8)(ulNDiv & 0xFF));
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x29, 0x80);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x31, 0x03);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	if (ucLoDiv == 3)
 		utmp = 0xCE;
@@ -366,15 +366,15 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
 		utmp = 0x8A;
 	ret = regmap_write(dev->regmap, 0x3b, utmp);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	dev->frequency_khz = fcry_KHz * (ulNDiv * 2 + 1024) / refDiv / ucLoDiv;
 
 	dev_dbg(&dev->client->dev,
 		"actual tune frequency=%d\n", dev->frequency_khz);
-err:
-	if (ret)
-		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+	return 0;
+report_failure:
+	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
 	return ret;
 }
 
@@ -413,21 +413,23 @@ static int m88rs6000t_set_params(struct dvb_frontend *fe)
 	freq_MHz = (realFreq + 500) / 1000;
 	ret = m88rs6000t_set_pll_freq(dev, freq_MHz);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = m88rs6000t_set_bb(dev, c->symbol_rate / 1000, lpf_offset_KHz);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x00, 0x01);
 	if (ret)
-		goto err;
+		goto report_failure;
 	ret = regmap_write(dev->regmap, 0x00, 0x00);
 	if (ret)
-		goto err;
+		goto report_failure;
 	/* set demod mlck */
 	ret = m88rs6000t_set_demod_mclk(fe);
-err:
 	if (ret)
-		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+		goto report_failure;
+	return 0;
+report_failure:
+	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
 	return ret;
 }
 
@@ -440,16 +442,16 @@ static int m88rs6000t_init(struct dvb_frontend *fe)
 
 	ret = regmap_update_bits(dev->regmap, 0x11, 0x08, 0x08);
 	if (ret)
-		goto err;
+		goto report_failure;
 	usleep_range(5000, 50000);
 	ret = regmap_update_bits(dev->regmap, 0x10, 0x01, 0x01);
 	if (ret)
-		goto err;
+		goto report_failure;
 	usleep_range(10000, 50000);
 	ret = regmap_write(dev->regmap, 0x07, 0x7d);
-err:
-	if (ret)
-		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+	return 0;
+report_failure:
+	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
 	return ret;
 }
 
@@ -510,27 +512,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 +564,9 @@ 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)
-		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+	return 0;
+report_failure:
+	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
 	return ret;
 }
 
-- 
2.6.3

Re: [PATCH 1/2] [media] m88rs6000t: Better exception handling in five functions

[Julia Lawall]

On Mon, 28 Dec 2015, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 28 Dec 2015 15:10:30 +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 a function call.

This commit message fits with the previous change.

It could be nice to put a blank line before the error handling code.  See
what is done elsewhere in the file.

julia

>
> Suggested-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/tuners/m88rs6000t.c | 154 +++++++++++++++++++-------------------
>  1 file changed, 78 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
> index 504bfbc..7e59a9f 100644
> --- a/drivers/media/tuners/m88rs6000t.c
> +++ b/drivers/media/tuners/m88rs6000t.c
> @@ -44,7 +44,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
>  	/* select demod main mclk */
>  	ret = regmap_read(dev->regmap, 0x15, &utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	reg15 = utmp;
>  	if (c->symbol_rate > 45010000) {
>  		reg11 = 0x0E;
> @@ -106,7 +106,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
>
>  	ret = regmap_read(dev->regmap, 0x1D, &utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	reg1D = utmp;
>  	reg1D &= ~0x03;
>  	reg1D |= N - 1;
> @@ -116,42 +116,42 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
>  	/* program and recalibrate demod PLL */
>  	ret = regmap_write(dev->regmap, 0x05, 0x40);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x11, 0x08);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x15, reg15);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x16, reg16);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x1D, reg1D);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x1E, reg1E);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x1F, reg1F);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x17, 0xc1);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x17, 0x81);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	usleep_range(5000, 50000);
>  	ret = regmap_write(dev->regmap, 0x05, 0x00);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x11, reg11);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	usleep_range(5000, 50000);
> -err:
> -	if (ret)
> -		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> +	return 0;
> +report_failure:
> +	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
>  	return ret;
>  }
>
> @@ -169,13 +169,13 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
>
>  	ret = regmap_write(dev->regmap, 0x36, (refDiv - 8));
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x31, 0x00);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x2c, 0x02);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>
>  	if (tuner_freq_MHz >= 1550) {
>  		ucLoDiv1 = 2;
> @@ -227,105 +227,105 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
>  	reg27 = (((ulNDiv1 >> 8) & 0x0F) + ucLomod1) & 0x7F;
>  	ret = regmap_write(dev->regmap, 0x27, reg27);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x28, (u8)(ulNDiv1 & 0xFF));
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	reg29 = (((ulNDiv2 >> 8) & 0x0F) + ucLomod2) & 0x7f;
>  	ret = regmap_write(dev->regmap, 0x29, reg29);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x2a, (u8)(ulNDiv2 & 0xFF));
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x2F, 0xf5);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x30, 0x05);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x08, 0x1f);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x08, 0x3f);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x09, 0x20);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x09, 0x00);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x3e, 0x11);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x08, 0x2f);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x08, 0x3f);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x09, 0x10);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x09, 0x00);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	usleep_range(2000, 50000);
>
>  	ret = regmap_read(dev->regmap, 0x42, &utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	reg42 = utmp;
>
>  	ret = regmap_write(dev->regmap, 0x3e, 0x10);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x08, 0x2f);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x08, 0x3f);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x09, 0x10);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x09, 0x00);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	usleep_range(2000, 50000);
>
>  	ret = regmap_read(dev->regmap, 0x42, &utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	reg42buf = utmp;
>  	if (reg42buf < reg42) {
>  		ret = regmap_write(dev->regmap, 0x3e, 0x11);
>  		if (ret)
> -			goto err;
> +			goto report_failure;
>  	}
>  	usleep_range(5000, 50000);
>
>  	ret = regmap_read(dev->regmap, 0x2d, &utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x2d, utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_read(dev->regmap, 0x2e, &utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x2e, utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>
>  	ret = regmap_read(dev->regmap, 0x27, &utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	reg27 = utmp & 0x70;
>  	ret = regmap_read(dev->regmap, 0x83, &utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	if (reg27 == (utmp & 0x70)) {
>  		ucLoDiv	= ucLoDiv1;
>  		ulNDiv = ulNDiv1;
> @@ -340,7 +340,7 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
>  		refDiv = 18;
>  		ret = regmap_write(dev->regmap, 0x36, (refDiv - 8));
>  		if (ret)
> -			goto err;
> +			goto report_failure;
>  		ulNDiv = ((tuner_freq_MHz * ucLoDiv * 1000) * refDiv
>  				/ fcry_KHz - 1024) / 2;
>  	}
> @@ -349,16 +349,16 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
>  			+ ((ulNDiv >> 8) & 0x0F)) & 0xFF;
>  	ret = regmap_write(dev->regmap, 0x27, reg27);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x28, (u8)(ulNDiv & 0xFF));
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x29, 0x80);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x31, 0x03);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>
>  	if (ucLoDiv == 3)
>  		utmp = 0xCE;
> @@ -366,15 +366,15 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
>  		utmp = 0x8A;
>  	ret = regmap_write(dev->regmap, 0x3b, utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>
>  	dev->frequency_khz = fcry_KHz * (ulNDiv * 2 + 1024) / refDiv / ucLoDiv;
>
>  	dev_dbg(&dev->client->dev,
>  		"actual tune frequency=%d\n", dev->frequency_khz);
> -err:
> -	if (ret)
> -		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> +	return 0;
> +report_failure:
> +	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
>  	return ret;
>  }
>
> @@ -413,21 +413,23 @@ static int m88rs6000t_set_params(struct dvb_frontend *fe)
>  	freq_MHz = (realFreq + 500) / 1000;
>  	ret = m88rs6000t_set_pll_freq(dev, freq_MHz);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = m88rs6000t_set_bb(dev, c->symbol_rate / 1000, lpf_offset_KHz);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x00, 0x01);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	ret = regmap_write(dev->regmap, 0x00, 0x00);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	/* set demod mlck */
>  	ret = m88rs6000t_set_demod_mclk(fe);
> -err:
>  	if (ret)
> -		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> +		goto report_failure;
> +	return 0;
> +report_failure:
> +	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
>  	return ret;
>  }
>
> @@ -440,16 +442,16 @@ static int m88rs6000t_init(struct dvb_frontend *fe)
>
>  	ret = regmap_update_bits(dev->regmap, 0x11, 0x08, 0x08);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	usleep_range(5000, 50000);
>  	ret = regmap_update_bits(dev->regmap, 0x10, 0x01, 0x01);
>  	if (ret)
> -		goto err;
> +		goto report_failure;
>  	usleep_range(10000, 50000);
>  	ret = regmap_write(dev->regmap, 0x07, 0x7d);
> -err:
> -	if (ret)
> -		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> +	return 0;
> +report_failure:
> +	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
>  	return ret;
>  }
>
> @@ -510,27 +512,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 +564,9 @@ 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)
> -		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> +	return 0;
> +report_failure:
> +	dev_dbg(&dev->client->dev, "failed=%d\n", ret);
>  	return ret;
>  }
>
> --
> 2.6.3
>
>

[PATCH 2/2] [media] tuners: Refactoring for m88rs6000t_sleep()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 15:20:45 +0100

This issue was detected by using the Coccinelle software.

1. Let us return directly if a call of the regmap_write() function failed.

2. Delete the jump label "err" then.

3. Return zero as a constant at the end.

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

diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
index 7e59a9f..8d10798 100644
--- a/drivers/media/tuners/m88rs6000t.c
+++ b/drivers/media/tuners/m88rs6000t.c
@@ -463,13 +463,12 @@ static int m88rs6000t_sleep(struct dvb_frontend *fe)
 	dev_dbg(&dev->client->dev, "%s:\n", __func__);
 
 	ret = regmap_write(dev->regmap, 0x07, 0x6d);
-	if (ret)
-		goto err;
-	usleep_range(5000, 10000);
-err:
-	if (ret)
+	if (ret) {
 		dev_dbg(&dev->client->dev, "failed=%d\n", ret);
-	return ret;
+		return ret;
+	}
+	usleep_range(5000, 10000);
+	return 0;
 }
 
 static int m88rs6000t_get_frequency(struct dvb_frontend *fe, u32 *frequency)
-- 
2.6.3

Re: [media] m88rs6000t: Better exception handling in five functions

[SF Markus Elfring]

>> Move the jump label directly before the desired log statement
>> so that the variable "ret" will not be checked once more
>> after a function call.
> 
> This commit message fits with the previous change.

Do you prefer an other wording?


> It could be nice to put a blank line before the error handling code.

Is it really a coding style requirement to insert another blank line between
the suggested placement of the statement "return 0;" and the jump label?

Regards,
Markus

Re: [media] m88rs6000t: Better exception handling in five functions

[Julia Lawall]

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 a function call.
> >
> > This commit message fits with the previous change.
>
> Do you prefer an other wording?

Something like "Split the return into success and error cases, to avoid
the need for testing before logging."

The concept of the return being duplicated didn't come across in your
message.

> > It could be nice to put a blank line before the error handling code.
>
> Is it really a coding style requirement to insert another blank line between
> the suggested placement of the statement "return 0;" and the jump label?

I don't think it is a requirement.  But some files do it, and if other
functions in this file do it, then it would be nice to do the same.

julia

[PATCH 0/2] [media] r820t: Fine-tuning for generic_set_freq()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 17:18:34 +0100

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

Markus Elfring (2):
  Delete an unnecessary variable initialisation
  Better exception handling

 drivers/media/tuners/r820t.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

-- 
2.6.3

[PATCH 1/2] [media] r820t: Delete an unnecessary variable initialisation in generic_set_freq()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 16:36:44 +0100

The variable "rc" will be set to an appropriate value from a call of
the r820t_set_tv_standard() function.
Thus let us omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/tuners/r820t.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
index a7a8452..6ab35e3 100644
--- a/drivers/media/tuners/r820t.c
+++ b/drivers/media/tuners/r820t.c
@@ -1295,7 +1295,7 @@ static int generic_set_freq(struct dvb_frontend *fe,
 			    v4l2_std_id std, u32 delsys)
 {
 	struct r820t_priv		*priv = fe->tuner_priv;
-	int				rc = -EINVAL;
+	int				rc;
 	u32				lo_freq;
 
 	tuner_dbg("should set frequency to %d kHz, bw %d MHz\n",
-- 
2.6.3

[PATCH 2/2] [media] r820t: Better exception handling in generic_set_freq()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 17:13:02 +0100

This issue was detected by using the Coccinelle software.

Move the jump label directly before the desired log statement
so that the variable "rc" will not be checked once more
after a function call.
Use the identifier "report_failure" instead of "err".

The error logging is performed in a separate section at the end now.

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

diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
index 6ab35e3..f71642e 100644
--- a/drivers/media/tuners/r820t.c
+++ b/drivers/media/tuners/r820t.c
@@ -1303,7 +1303,7 @@ static int generic_set_freq(struct dvb_frontend *fe,
 
 	rc = r820t_set_tv_standard(priv, bw, type, std, delsys);
 	if (rc < 0)
-		goto err;
+		goto report_failure;
 
 	if ((type == V4L2_TUNER_ANALOG_TV) && (std == V4L2_STD_SECAM_LC))
 		lo_freq = freq - priv->int_freq;
@@ -1312,23 +1312,21 @@ static int generic_set_freq(struct dvb_frontend *fe,
 
 	rc = r820t_set_mux(priv, lo_freq);
 	if (rc < 0)
-		goto err;
+		goto report_failure;
 
 	rc = r820t_set_pll(priv, type, lo_freq);
 	if (rc < 0 || !priv->has_lock)
-		goto err;
+		goto report_failure;
 
 	rc = r820t_sysfreq_sel(priv, freq, type, std, delsys);
 	if (rc < 0)
-		goto err;
+		goto report_failure;
 
 	tuner_dbg("%s: PLL locked on frequency %d Hz, gain=%d\n",
 		  __func__, freq, r820t_read_gain(priv));
-
-err:
-
-	if (rc < 0)
-		tuner_dbg("%s: failed=%d\n", __func__, rc);
+	return 0;
+report_failure:
+	tuner_dbg("%s: failed=%d\n", __func__, rc);
 	return rc;
 }
 
-- 
2.6.3

[PATCH] [media] xc5000: Faster result reporting in xc_load_fw_and_init_tuner()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 20:10:30 +0100

This issue was detected by using the Coccinelle software.

Split the previous if statement at the end so that each final log statement
will eventually be performed by a direct jump to these labels.
* report_failure
* report_success

A check repetition can be excluded for the variable "ret" at the end then.


Apply also two recommendations from the script "checkpatch.pl".

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

diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index e6e5e90..1360677 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -1166,7 +1166,7 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
 
 		ret = xc5000_fwupload(fe, desired_fw, fw);
 		if (ret != 0)
-			goto err;
+			goto report_failure;
 
 		msleep(20);
 
@@ -1229,18 +1229,16 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
 		/* Default to "CABLE" mode */
 		ret = xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE);
 		if (!ret)
-			break;
+			goto report_success;
 		printk(KERN_ERR "xc5000: can't set to cable mode.");
 	}
 
-err:
-	if (!ret)
-		printk(KERN_INFO "xc5000: Firmware %s loaded and running.\n",
-		       desired_fw->name);
-	else
-		printk(KERN_CONT " - too many retries. Giving up\n");
-
+report_failure:
+	pr_cont(" - too many retries. Giving up\n");
 	return ret;
+report_success:
+	pr_info("xc5000: Firmware %s loaded and running.\n", desired_fw->name);
+	return 0;
 }
 
 static void xc5000_do_timer_sleep(struct work_struct *timer_sleep)
-- 
2.6.3

[PATCH] [media] airspy: Better exception handling in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 22:10:28 +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 a function call.
Use the identifier "report_failure" instead of "err".

The error logging is performed in a separate section at the end now.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/usb/airspy/airspy.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/airspy/airspy.c b/drivers/media/usb/airspy/airspy.c
index 0d4ac59..cf2444a 100644
--- a/drivers/media/usb/airspy/airspy.c
+++ b/drivers/media/usb/airspy/airspy.c
@@ -889,18 +889,17 @@ static int airspy_set_lna_gain(struct airspy *s)
 	ret = airspy_ctrl_msg(s, CMD_SET_LNA_AGC, 0, s->lna_gain_auto->val,
 			&u8tmp, 1);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	if (s->lna_gain_auto->val == false) {
 		ret = airspy_ctrl_msg(s, CMD_SET_LNA_GAIN, 0, s->lna_gain->val,
 				&u8tmp, 1);
 		if (ret)
-			goto err;
+			goto report_failure;
 	}
-err:
-	if (ret)
-		dev_dbg(s->dev, "failed=%d\n", ret);
-
+	return 0;
+report_failure:
+	dev_dbg(s->dev, "failed=%d\n", ret);
 	return ret;
 }
 
@@ -916,18 +915,17 @@ static int airspy_set_mixer_gain(struct airspy *s)
 	ret = airspy_ctrl_msg(s, CMD_SET_MIXER_AGC, 0, s->mixer_gain_auto->val,
 			&u8tmp, 1);
 	if (ret)
-		goto err;
+		goto report_failure;
 
 	if (s->mixer_gain_auto->val == false) {
 		ret = airspy_ctrl_msg(s, CMD_SET_MIXER_GAIN, 0,
 				s->mixer_gain->val, &u8tmp, 1);
 		if (ret)
-			goto err;
+			goto report_failure;
 	}
-err:
-	if (ret)
-		dev_dbg(s->dev, "failed=%d\n", ret);
-
+	return 0;
+report_failure:
+	dev_dbg(s->dev, "failed=%d\n", ret);
 	return ret;
 }
 
-- 
2.6.3

[PATCH] [media] au0828: Refactoring for start_urb_transfer()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 22:52:48 +0100

This issue was detected by using the Coccinelle software.

1. Let us return directly if a buffer allocation failed.

2. Delete the jump label "err" then.

3. Drop the explicit initialisation for the variable "ret"
   at the beginning.

4. Return zero as a constant at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/usb/au0828/au0828-dvb.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-dvb.c b/drivers/media/usb/au0828/au0828-dvb.c
index cd542b4..e5f1e20 100644
--- a/drivers/media/usb/au0828/au0828-dvb.c
+++ b/drivers/media/usb/au0828/au0828-dvb.c
@@ -181,7 +181,7 @@ static int stop_urb_transfer(struct au0828_dev *dev)
 static int start_urb_transfer(struct au0828_dev *dev)
 {
 	struct urb *purb;
-	int i, ret = -ENOMEM;
+	int i, ret;
 
 	dprintk(2, "%s()\n", __func__);
 
@@ -194,7 +194,7 @@ static int start_urb_transfer(struct au0828_dev *dev)
 
 		dev->urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
 		if (!dev->urbs[i])
-			goto err;
+			return -ENOMEM;
 
 		purb = dev->urbs[i];
 
@@ -207,9 +207,10 @@ static int start_urb_transfer(struct au0828_dev *dev)
 		if (!purb->transfer_buffer) {
 			usb_free_urb(purb);
 			dev->urbs[i] = NULL;
+			ret = -ENOMEM;
 			pr_err("%s: failed big buffer allocation, err = %d\n",
 			       __func__, ret);
-			goto err;
+			return ret;
 		}
 
 		purb->status = -EINPROGRESS;
@@ -235,10 +236,7 @@ static int start_urb_transfer(struct au0828_dev *dev)
 	}
 
 	dev->urb_streaming = true;
-	ret = 0;
-
-err:
-	return ret;
+	return 0;
 }
 
 static void au0828_start_transport(struct au0828_dev *dev)
-- 
2.6.3

[PATCH] [media] hdpvr: Refactoring for hdpvr_read()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 29 Dec 2015 11:02:43 +0100

Let us return directly if the element "status" of the variable "buf"
indicates "BUFSTAT_READY".
A check repetition can be excluded for the variable "ret" at the end then.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/usb/hdpvr/hdpvr-video.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c b/drivers/media/usb/hdpvr/hdpvr-video.c
index 7dee22d..ba7f022 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -462,10 +462,8 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
 			}
 
 			if (wait_event_interruptible(dev->wait_data,
-					      buf->status == BUFSTAT_READY)) {
-				ret = -ERESTARTSYS;
-				goto err;
-			}
+					      buf->status == BUFSTAT_READY))
+				return -ERESTARTSYS;
 		}
 
 		if (buf->status != BUFSTAT_READY)
-- 
2.6.3

[PATCH] [media] msi2500: Delete an unnecessary check in msi2500_set_usb_adc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 29 Dec 2015 12:32:41 +0100

This issue was detected by using the Coccinelle software.

Return the value from a call of the msi2500_ctrl_msg() function
without using an extra check for the variable "ret" at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/usb/msi2500/msi2500.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/usb/msi2500/msi2500.c b/drivers/media/usb/msi2500/msi2500.c
index c104315..2d33033 100644
--- a/drivers/media/usb/msi2500/msi2500.c
+++ b/drivers/media/usb/msi2500/msi2500.c
@@ -839,8 +839,6 @@ static int msi2500_set_usb_adc(struct msi2500_dev *dev)
 		goto err;
 
 	ret = msi2500_ctrl_msg(dev, CMD_WREG, reg3);
-	if (ret)
-		goto err;
 err:
 	return ret;
 }
-- 
2.6.3

Re: [PATCH] [media] si2165: Refactoring for si2165_writereg_mask8()

[Matthias Schwarzott]

Am 27.12.2015 um 18:33 schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 27 Dec 2015 18:23:57 +0100
> 
> This issue was detected by using the Coccinelle software.
> 
> 1. Let us return directly if a call of the si2165_readreg8()
>    function failed.
> 
> 2. Reduce the scope for the local variables "ret" and "tmp" to one branch
>    of an if statement.
> 
> 3. Delete the jump label "err" then.
> 
> 4. Return the value from a call of the si2165_writereg8() function
>    without using an extra assignment for the variable "ret" at the end.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

The patch looks fine.

Signed-off-by: Matthias Schwarzott <zzam@gentoo.org>

Regards
Matthias

PS: I am going to switch to regmap, but this change is not yet polished
and until now does not touch this function.

Re: [PATCH 1/2] [media] m88rs6000t: Better exception handling in five functions

[Mauro Carvalho Chehab]

Em Mon, 28 Dec 2015 15:38:54 +0100
SF Markus Elfring <elfring@users.sourceforge.net> escreveu:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 28 Dec 2015 15:10:30 +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 a function call.
> Use the identifier "report_failure" instead of "err".
> 
> Suggested-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/tuners/m88rs6000t.c | 154 +++++++++++++++++++-------------------
>  1 file changed, 78 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
> index 504bfbc..7e59a9f 100644
> --- a/drivers/media/tuners/m88rs6000t.c
> +++ b/drivers/media/tuners/m88rs6000t.c
> @@ -44,7 +44,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
>  	/* select demod main mclk */
>  	ret = regmap_read(dev->regmap, 0x15, &utmp);
>  	if (ret)
> -		goto err;
> +		goto report_failure;

Why to be so verbose? Calling it as "err" is enough, and it means less
code to type if we need to add another goto.

Regards,
Mauro

Re: [PATCH 2/2] [media] r820t: Better exception handling in generic_set_freq()

[Mauro Carvalho Chehab]

Em Mon, 28 Dec 2015 17:32:22 +0100
SF Markus Elfring <elfring@users.sourceforge.net> escreveu:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 28 Dec 2015 17:13:02 +0100
> 
> This issue was detected by using the Coccinelle software.
> 
> Move the jump label directly before the desired log statement
> so that the variable "rc" will not be checked once more
> after a function call.
> Use the identifier "report_failure" instead of "err".
> 
> The error logging is performed in a separate section at the end now.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/tuners/r820t.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
> index 6ab35e3..f71642e 100644
> --- a/drivers/media/tuners/r820t.c
> +++ b/drivers/media/tuners/r820t.c
> @@ -1303,7 +1303,7 @@ static int generic_set_freq(struct dvb_frontend *fe,
>  
>  	rc = r820t_set_tv_standard(priv, bw, type, std, delsys);
>  	if (rc < 0)
> -		goto err;
> +		goto report_failure;

Same thing as my previous comment: just "err" please.

Same applies to other patches you sent with similar hunks.

>  
>  	if ((type == V4L2_TUNER_ANALOG_TV) && (std == V4L2_STD_SECAM_LC))
>  		lo_freq = freq - priv->int_freq;
> @@ -1312,23 +1312,21 @@ static int generic_set_freq(struct dvb_frontend *fe,
>  
>  	rc = r820t_set_mux(priv, lo_freq);
>  	if (rc < 0)
> -		goto err;
> +		goto report_failure;
>  
>  	rc = r820t_set_pll(priv, type, lo_freq);
>  	if (rc < 0 || !priv->has_lock)
> -		goto err;
> +		goto report_failure;
>  
>  	rc = r820t_sysfreq_sel(priv, freq, type, std, delsys);
>  	if (rc < 0)
> -		goto err;
> +		goto report_failure;
>  
>  	tuner_dbg("%s: PLL locked on frequency %d Hz, gain=%d\n",
>  		  __func__, freq, r820t_read_gain(priv));
> -
> -err:
> -
> -	if (rc < 0)
> -		tuner_dbg("%s: failed=%d\n", __func__, rc);
> +	return 0;
> +report_failure:
> +	tuner_dbg("%s: failed=%d\n", __func__, rc);
>  	return rc;
>  }
>  

Re: [PATCH] [media] xc5000: Faster result reporting in xc_load_fw_and_init_tuner()

[Mauro Carvalho Chehab]

Em Mon, 28 Dec 2015 20:20:27 +0100
SF Markus Elfring <elfring@users.sourceforge.net> escreveu:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 28 Dec 2015 20:10:30 +0100
> 
> This issue was detected by using the Coccinelle software.
> 
> Split the previous if statement at the end so that each final log statement
> will eventually be performed by a direct jump to these labels.
> * report_failure
> * report_success
> 
> A check repetition can be excluded for the variable "ret" at the end then.
> 
> 
> Apply also two recommendations from the script "checkpatch.pl".
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/tuners/xc5000.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
> index e6e5e90..1360677 100644
> --- a/drivers/media/tuners/xc5000.c
> +++ b/drivers/media/tuners/xc5000.c
> @@ -1166,7 +1166,7 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
>  
>  		ret = xc5000_fwupload(fe, desired_fw, fw);
>  		if (ret != 0)
> -			goto err;
> +			goto report_failure;
>  
>  		msleep(20);
>  
> @@ -1229,18 +1229,16 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
>  		/* Default to "CABLE" mode */
>  		ret = xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE);
>  		if (!ret)
> -			break;
> +			goto report_success;
>  		printk(KERN_ERR "xc5000: can't set to cable mode.");

It sounds worth to avoid adding a goto here.

>  	}
>  
> -err:
> -	if (!ret)
> -		printk(KERN_INFO "xc5000: Firmware %s loaded and running.\n",
> -		       desired_fw->name);
> -	else
> -		printk(KERN_CONT " - too many retries. Giving up\n");
> -
> +report_failure:
> +	pr_cont(" - too many retries. Giving up\n");
>  	return ret;
> +report_success:
> +	pr_info("xc5000: Firmware %s loaded and running.\n", desired_fw->name);
> +	return 0;
>  }
>  
>  static void xc5000_do_timer_sleep(struct work_struct *timer_sleep)

Re: [PATCH 1/2] [media] m88rs6000t: Better exception handling in five functions

[SF Markus Elfring]

>> 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 a function call.
>> Use the identifier "report_failure" instead of "err".
>>
>> Suggested-by: Julia Lawall <julia.lawall@lip6.fr>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>  drivers/media/tuners/m88rs6000t.c | 154 +++++++++++++++++++-------------------
>>  1 file changed, 78 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
>> index 504bfbc..7e59a9f 100644
>> --- a/drivers/media/tuners/m88rs6000t.c
>> +++ b/drivers/media/tuners/m88rs6000t.c
>> @@ -44,7 +44,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
>>  	/* select demod main mclk */
>>  	ret = regmap_read(dev->regmap, 0x15, &utmp);
>>  	if (ret)
>> -		goto err;
>> +		goto report_failure;
> 
> Why to be so verbose?

Does the document "CodingStyle" give an indication in the section "Chapter 7:
Centralized exiting of functions"?


> Calling it as "err" is enough,

It seems that some short identifiers are popular during software development.


> and it means less code to type if we need to add another goto.

Would you like to increase the usage of jump labels which will contain
only a single character?

Regards,
Markus

Re: [PATCH] [media] xc5000: Faster result reporting in xc_load_fw_and_init_tuner()

[SF Markus Elfring]

>> This issue was detected by using the Coccinelle software.
>>
>> Split the previous if statement at the end so that each final log statement
>> will eventually be performed by a direct jump to these labels.
>> * report_failure
>> * report_success
>>
>> A check repetition can be excluded for the variable "ret" at the end then.
>>
>>
>> Apply also two recommendations from the script "checkpatch.pl".
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>  drivers/media/tuners/xc5000.c | 16 +++++++---------
>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
>> index e6e5e90..1360677 100644
>> --- a/drivers/media/tuners/xc5000.c
>> +++ b/drivers/media/tuners/xc5000.c
>> @@ -1166,7 +1166,7 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
>>  
>>  		ret = xc5000_fwupload(fe, desired_fw, fw);
>>  		if (ret != 0)
>> -			goto err;
>> +			goto report_failure;
>>  
>>  		msleep(20);
>>  
>> @@ -1229,18 +1229,16 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
>>  		/* Default to "CABLE" mode */
>>  		ret = xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE);
>>  		if (!ret)
>> -			break;
>> +			goto report_success;
>>  		printk(KERN_ERR "xc5000: can't set to cable mode.");
> 
> It sounds worth to avoid adding a goto here.

Are you interested in a bit of software optimisation for the implementation
of the function "xc_load_fw_and_init_tuner"?


>>  	}
>>  
>> -err:
>> -	if (!ret)
>> -		printk(KERN_INFO "xc5000: Firmware %s loaded and running.\n",
>> -		       desired_fw->name);
>> -	else
>> -		printk(KERN_CONT " - too many retries. Giving up\n");
>> -
>> +report_failure:
>> +	pr_cont(" - too many retries. Giving up\n");
>>  	return ret;
>> +report_success:
>> +	pr_info("xc5000: Firmware %s loaded and running.\n", desired_fw->name);
>> +	return 0;
>>  }
>>  
>>  static void xc5000_do_timer_sleep(struct work_struct *timer_sleep)


Is the proposed source code restructuring interesting?

Regards,
Markus

Re: [PATCH] [media] xc5000: Faster result reporting in xc_load_fw_and_init_tuner()

[Devin Heitmueller]

> Are you interested in a bit of software optimisation for the implementation
> of the function "xc_load_fw_and_init_tuner"?

To be clear, absolutely none of the code in question is performance
sensitive (i.e. saving a couple of extra CPU cycles has no value in
this case).  Hence given that I'm assuming you have no intention to
actually test any of these patches with a real device I would
recommend you do the bare minimum to prevent Coccinelle from
complaining and not restructure any of the core business logic unless
you plan to also do actual testing.

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[PATCH 0/2] uvc_v4l2: Fine-tuning for uvc_ioctl_ctrl_map()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 19 Aug 2016 11:11:01 +0200

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

Markus Elfring (2):
  Use memdup_user() rather than duplicating its implementation
  One function call less after error detection

 drivers/media/usb/uvc/uvc_v4l2.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

-- 
2.9.3

[PATCH 1/2] uvc_v4l2: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 19 Aug 2016 10:50:05 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 05eed4b..a7e12fd 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -70,14 +70,9 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
 		}
 
 		size = xmap->menu_count * sizeof(*map->menu_info);
-		map->menu_info = kmalloc(size, GFP_KERNEL);
-		if (map->menu_info == NULL) {
-			ret = -ENOMEM;
-			goto done;
-		}
-
-		if (copy_from_user(map->menu_info, xmap->menu_info, size)) {
-			ret = -EFAULT;
+		map->menu_info = memdup_user(xmap->menu_info, size);
+		if (IS_ERR(map->menu_info)) {
+			ret = PTR_ERR(map->menu_info);
 			goto done;
 		}
 
-- 
2.9.3

[PATCH 2/2] uvc_v4l2: One function call less in uvc_ioctl_ctrl_map() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 19 Aug 2016 11:00:38 +0200

The kfree() function was called in two cases by the uvc_ioctl_ctrl_map()
function during error handling even if the passed data structure element
contained a null pointer.

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index a7e12fd..52a2af8 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
 		if (xmap->menu_count == 0 ||
 		    xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
 			ret = -EINVAL;
-			goto done;
+			goto free_map;
 		}
 
 		size = xmap->menu_count * sizeof(*map->menu_info);
 		map->menu_info = memdup_user(xmap->menu_info, size);
 		if (IS_ERR(map->menu_info)) {
 			ret = PTR_ERR(map->menu_info);
-			goto done;
+			goto free_map;
 		}
 
 		map->menu_count = xmap->menu_count;
@@ -83,13 +83,12 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
 		uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
 			  "%u.\n", xmap->v4l2_type);
 		ret = -ENOTTY;
-		goto done;
+		goto free_map;
 	}
 
 	ret = uvc_ctrl_add_mapping(chain, map);
-
-done:
 	kfree(map->menu_info);
+free_map:
 	kfree(map);
 
 	return ret;
-- 
2.9.3

[PATCH 0/2] [media] dvb-tc90522: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 7 Oct 2016 21:38:12 +0200

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

Markus Elfring (2):
  Use kmalloc_array()
  Rename a jump label

 drivers/media/dvb-frontends/tc90522.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

-- 
2.10.1

[PATCH 1/2] [media] dvb-tc90522: Use kmalloc_array() in tc90522_master_xfer()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 7 Oct 2016 21:07:43 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/dvb-frontends/tc90522.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/tc90522.c b/drivers/media/dvb-frontends/tc90522.c
index 31cd325..c2d45f0 100644
--- a/drivers/media/dvb-frontends/tc90522.c
+++ b/drivers/media/dvb-frontends/tc90522.c
@@ -656,7 +656,7 @@ tc90522_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	for (i = 0; i < num; i++)
 		if (msgs[i].flags & I2C_M_RD)
 			rd_num++;
-	new_msgs = kmalloc(sizeof(*new_msgs) * (num + rd_num), GFP_KERNEL);
+	new_msgs = kmalloc_array(num + rd_num, sizeof(*new_msgs), GFP_KERNEL);
 	if (!new_msgs)
 		return -ENOMEM;
 
-- 
2.10.1

[PATCH 2/2] [media] dvb-tc90522: Rename a jump label in tc90522_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 7 Oct 2016 21:13:57 +0200

Adjust a jump label according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/dvb-frontends/tc90522.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/tc90522.c b/drivers/media/dvb-frontends/tc90522.c
index c2d45f0..4687e15 100644
--- a/drivers/media/dvb-frontends/tc90522.c
+++ b/drivers/media/dvb-frontends/tc90522.c
@@ -794,14 +794,13 @@ static int tc90522_probe(struct i2c_client *client,
 	i2c_set_adapdata(adap, state);
 	ret = i2c_add_adapter(adap);
 	if (ret < 0)
-		goto err;
+		goto free_state;
 	cfg->tuner_i2c = state->cfg.tuner_i2c = adap;
 
 	i2c_set_clientdata(client, &state->cfg);
 	dev_info(&client->dev, "Toshiba TC90522 attached.\n");
 	return 0;
-
-err:
+free_state:
 	kfree(state);
 	return ret;
 }
-- 
2.10.1

Re: [PATCH 2/2] [media] dvb-tc90522: Rename a jump label in tc90522_probe()

[walter harms]

Am 07.10.2016 21:46, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 7 Oct 2016 21:13:57 +0200
> 
> Adjust a jump label according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/dvb-frontends/tc90522.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/tc90522.c b/drivers/media/dvb-frontends/tc90522.c
> index c2d45f0..4687e15 100644
> --- a/drivers/media/dvb-frontends/tc90522.c
> +++ b/drivers/media/dvb-frontends/tc90522.c
> @@ -794,14 +794,13 @@ static int tc90522_probe(struct i2c_client *client,
>  	i2c_set_adapdata(adap, state);
>  	ret = i2c_add_adapter(adap);
>  	if (ret < 0)
> -		goto err;
> +		goto free_state;
>  	cfg->tuner_i2c = state->cfg.tuner_i2c = adap;
>  
>  	i2c_set_clientdata(client, &state->cfg);
>  	dev_info(&client->dev, "Toshiba TC90522 attached.\n");
>  	return 0;
> -
> -err:
> +free_state:
>  	kfree(state);
>  	return ret;
>  }

there is only one user, IMHO this can be moved to the if block.

re,
 wh

[PATCH 00/18] [media] RedRat3: Fine-tuning for several function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 18:06:18 +0200

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

Markus Elfring (18):
  Use kcalloc() in two functions
  Move two assignments in redrat3_transmit_ir()
  Return directly after a failed kcalloc() in redrat3_transmit_ir()
  One function call less in redrat3_transmit_ir() after error detection
  Delete six messages for a failed memory allocation
  Delete an unnecessary variable initialisation in redrat3_get_firmware_rev()
  Improve another size determination in redrat3_reset()
  Improve another size determination in redrat3_send_cmd()
  Move a variable assignment in redrat3_dev_probe()
  Delete an unnecessary variable initialisation in redrat3_init_rc_dev()
  Delete the variable "dev" in redrat3_init_rc_dev()
  Move a variable assignment in redrat3_init_rc_dev()
  Return directly after a failed rc_allocate_device() in redrat3_init_rc_dev()
  Rename a jump label in redrat3_init_rc_dev()
  Delete two variables in redrat3_set_timeout()
  Move a variable assignment in redrat3_set_timeout()
  Adjust two checks for null pointers in redrat3_dev_probe()
  Combine substrings for six messages

 drivers/media/rc/redrat3.c | 146 +++++++++++++++++++++------------------------
 1 file changed, 67 insertions(+), 79 deletions(-)

-- 
2.10.1

[PATCH 01/18] [media] RedRat3: Use kcalloc() in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 08:35:57 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus use the corresponding function "kcalloc".

  This issue was detected by using the Coccinelle software.

* Replace the specification of data types by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 8d7df6d..d89958b 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -549,7 +549,7 @@ static void redrat3_get_firmware_rev(struct redrat3_dev *rr3)
 	int rc = 0;
 	char *buffer;
 
-	buffer = kzalloc(sizeof(char) * (RR3_FW_VERSION_LEN + 1), GFP_KERNEL);
+	buffer = kcalloc(RR3_FW_VERSION_LEN + 1, sizeof(*buffer), GFP_KERNEL);
 	if (!buffer) {
 		dev_err(rr3->dev, "Memory allocation failure\n");
 		return;
@@ -741,7 +741,9 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 	/* rr3 will disable rc detector on transmit */
 	rr3->transmitting = true;
 
-	sample_lens = kzalloc(sizeof(int) * RR3_DRIVER_MAXLENS, GFP_KERNEL);
+	sample_lens = kcalloc(RR3_DRIVER_MAXLENS,
+			      sizeof(*sample_lens),
+			      GFP_KERNEL);
 	if (!sample_lens) {
 		ret = -ENOMEM;
 		goto out;
-- 
2.10.1

[PATCH 02/18] [media] RedRat3: Move two assignments in redrat3_transmit_ir()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 10:25:57 +0200

Move the assignment for the data structure member "transmitting"
and the local variable "curlencheck" behind the source code
for memory allocations by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index d89958b..f5a6850 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -727,7 +727,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 	int ret, ret_len;
 	int lencheck, cur_sample_len, pipe;
 	int *sample_lens = NULL;
-	u8 curlencheck = 0;
+	u8 curlencheck;
 	unsigned i, sendbuf_len;
 
 	if (rr3->transmitting) {
@@ -738,9 +738,6 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 	if (count > RR3_MAX_SIG_SIZE - RR3_TX_TRAILER_LEN)
 		return -EINVAL;
 
-	/* rr3 will disable rc detector on transmit */
-	rr3->transmitting = true;
-
 	sample_lens = kcalloc(RR3_DRIVER_MAXLENS,
 			      sizeof(*sample_lens),
 			      GFP_KERNEL);
@@ -755,6 +752,9 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 		goto out;
 	}
 
+	/* rr3 will disable rc detector on transmit */
+	rr3->transmitting = true;
+	curlencheck = 0;
 	for (i = 0; i < count; i++) {
 		cur_sample_len = redrat3_us_to_len(txbuf[i]);
 		if (cur_sample_len > 0xffff) {
-- 
2.10.1

[PATCH 03/18] [media] RedRat3: Return directly after a failed kcalloc() in redrat3_transmit_ir()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 10:34:29 +0200

* Return directly after a call of the function "kcalloc" failed
  at the beginning.

* Reorder two calls for the function "kfree" at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index f5a6850..7ae2ced 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -741,10 +741,8 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 	sample_lens = kcalloc(RR3_DRIVER_MAXLENS,
 			      sizeof(*sample_lens),
 			      GFP_KERNEL);
-	if (!sample_lens) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!sample_lens)
+		return -ENOMEM;
 
 	irdata = kzalloc(sizeof(*irdata), GFP_KERNEL);
 	if (!irdata) {
@@ -815,8 +813,8 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 		ret = count;
 
 out:
-	kfree(sample_lens);
 	kfree(irdata);
+	kfree(sample_lens);
 
 	rr3->transmitting = false;
 	/* rr3 re-enables rc detector because it was enabled before */
-- 
2.10.1

[PATCH 04/18] [media] RedRat3: One function call less in redrat3_transmit_ir() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 10:50:24 +0200

The kfree() function was called in one case by the
redrat3_transmit_ir() function during error handling
even if the passed variable contained a null pointer.

* Adjust jump targets according to the Linux coding style convention.

* Move the resetting for the data structure member "transmitting"
  at the end.

* Delete initialisations for the variables "irdata" and "sample_lens"
  at the beginning which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 7ae2ced..71e901d 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -723,10 +723,10 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 {
 	struct redrat3_dev *rr3 = rcdev->priv;
 	struct device *dev = rr3->dev;
-	struct redrat3_irdata *irdata = NULL;
+	struct redrat3_irdata *irdata;
 	int ret, ret_len;
 	int lencheck, cur_sample_len, pipe;
-	int *sample_lens = NULL;
+	int *sample_lens;
 	u8 curlencheck;
 	unsigned i, sendbuf_len;
 
@@ -747,7 +747,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 	irdata = kzalloc(sizeof(*irdata), GFP_KERNEL);
 	if (!irdata) {
 		ret = -ENOMEM;
-		goto out;
+		goto free_sample;
 	}
 
 	/* rr3 will disable rc detector on transmit */
@@ -776,7 +776,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 				curlencheck++;
 			} else {
 				ret = -EINVAL;
-				goto out;
+				goto reset_member;
 			}
 		}
 		irdata->sigdata[i] = lencheck;
@@ -811,14 +811,12 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 		dev_err(dev, "Error: control msg send failed, rc %d\n", ret);
 	else
 		ret = count;
-
-out:
-	kfree(irdata);
-	kfree(sample_lens);
-
+reset_member:
 	rr3->transmitting = false;
 	/* rr3 re-enables rc detector because it was enabled before */
-
+	kfree(irdata);
+free_sample:
+	kfree(sample_lens);
 	return ret;
 }
 
-- 
2.10.1

[PATCH 05/18] [media] RedRat3: Delete six messages for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 13:20:19 +0200

The script "checkpatch.pl" pointed information out like the following.

WARNING: Possible unnecessary 'out of memory' message

Thus remove such a logging statement in five functions.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 71e901d..2e31c18 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -453,10 +453,8 @@ static u32 redrat3_get_timeout(struct redrat3_dev *rr3)
 
 	len = sizeof(*tmp);
 	tmp = kzalloc(len, GFP_KERNEL);
-	if (!tmp) {
-		dev_warn(rr3->dev, "Memory allocation faillure\n");
+	if (!tmp)
 		return timeout;
-	}
 
 	pipe = usb_rcvctrlpipe(rr3->udev, 0);
 	ret = usb_control_msg(rr3->udev, pipe, RR3_GET_IR_PARAM,
@@ -518,10 +516,8 @@ static void redrat3_reset(struct redrat3_dev *rr3)
 	txpipe = usb_sndctrlpipe(udev, 0);
 
 	val = kmalloc(len, GFP_KERNEL);
-	if (!val) {
-		dev_err(dev, "Memory allocation failure\n");
+	if (!val)
 		return;
-	}
 
 	*val = 0x01;
 	rc = usb_control_msg(udev, rxpipe, RR3_RESET,
@@ -550,10 +546,8 @@ static void redrat3_get_firmware_rev(struct redrat3_dev *rr3)
 	char *buffer;
 
 	buffer = kcalloc(RR3_FW_VERSION_LEN + 1, sizeof(*buffer), GFP_KERNEL);
-	if (!buffer) {
-		dev_err(rr3->dev, "Memory allocation failure\n");
+	if (!buffer)
 		return;
-	}
 
 	rc = usb_control_msg(rr3->udev, usb_rcvctrlpipe(rr3->udev, 0),
 			     RR3_FW_VERSION,
@@ -866,10 +860,8 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
 	u16 prod = le16_to_cpu(rr3->udev->descriptor.idProduct);
 
 	rc = rc_allocate_device();
-	if (!rc) {
-		dev_err(dev, "remote input dev allocation failed\n");
+	if (!rc)
 		goto out;
-	}
 
 	snprintf(rr3->name, sizeof(rr3->name), "RedRat3%s "
 		 "Infrared Remote Transceiver (%04x:%04x)",
@@ -959,10 +951,8 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 
 	/* allocate memory for our device state and initialize it */
 	rr3 = kzalloc(sizeof(*rr3), GFP_KERNEL);
-	if (rr3 == NULL) {
-		dev_err(dev, "Memory allocation failure\n");
+	if (!rr3)
 		goto no_endpoints;
-	}
 
 	rr3->dev = &intf->dev;
 
@@ -974,10 +964,8 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	rr3->ep_in = ep_in;
 	rr3->bulk_in_buf = usb_alloc_coherent(udev,
 		le16_to_cpu(ep_in->wMaxPacketSize), GFP_KERNEL, &rr3->dma_in);
-	if (!rr3->bulk_in_buf) {
-		dev_err(dev, "Read buffer allocation failure\n");
+	if (!rr3->bulk_in_buf)
 		goto error;
-	}
 
 	pipe = usb_rcvbulkpipe(udev, ep_in->bEndpointAddress);
 	usb_fill_bulk_urb(rr3->read_urb, udev, pipe, rr3->bulk_in_buf,
-- 
2.10.1

[PATCH 06/18] [media] RedRat3: Delete an unnecessary variable initialisation in redrat3_get_firmware_rev()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 13:21:55 +0200

The local variable "rc" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 2e31c18..0ac96a4 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -542,7 +542,7 @@ static void redrat3_reset(struct redrat3_dev *rr3)
 
 static void redrat3_get_firmware_rev(struct redrat3_dev *rr3)
 {
-	int rc = 0;
+	int rc;
 	char *buffer;
 
 	buffer = kcalloc(RR3_FW_VERSION_LEN + 1, sizeof(*buffer), GFP_KERNEL);
-- 
2.10.1

[PATCH 07/18] [media] RedRat3: Improve another size determination in redrat3_reset()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 13:23:22 +0200

Replace the specification of a data type by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 0ac96a4..5832e6f 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -510,7 +510,7 @@ static void redrat3_reset(struct redrat3_dev *rr3)
 	struct device *dev = rr3->dev;
 	int rc, rxpipe, txpipe;
 	u8 *val;
-	int len = sizeof(u8);
+	size_t const len = sizeof(*val);
 
 	rxpipe = usb_rcvctrlpipe(udev, 0);
 	txpipe = usb_sndctrlpipe(udev, 0);
-- 
2.10.1

Re: [PATCH 01/18] [media] RedRat3: Use kcalloc() in two functions

[Joe Perches]

On Thu, 2016-10-13 at 18:18 +0200, SF Markus Elfring wrote:
> diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
[]
> @@ -549,7 +549,7 @@ static void redrat3_get_firmware_rev(struct redrat3_dev *rr3)
>  	int rc = 0;
>  	char *buffer;
>  
> -	buffer = kzalloc(sizeof(char) * (RR3_FW_VERSION_LEN + 1), GFP_KERNEL);
> +	buffer = kcalloc(RR3_FW_VERSION_LEN + 1, sizeof(*buffer), GFP_KERNEL);
>  	if (!buffer) {
>  		dev_err(rr3->dev, "Memory allocation failure\n");
>  		return;,

Markus, please stop being _so_ mechanical and use your
brain a little too.  By definition, sizeof(char) == 1.

This _really_ should be kzalloc(RR3_FW_VERSION_LEN + 1,...)

[PATCH 08/18] [media] RedRat3: Improve another size determination in redrat3_send_cmd()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 13:31:17 +0200

Replace the specification of a data type by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 5832e6f..f6c21a1 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -388,7 +388,7 @@ static int redrat3_send_cmd(int cmd, struct redrat3_dev *rr3)
 	u8 *data;
 	int res;
 
-	data = kzalloc(sizeof(u8), GFP_KERNEL);
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-- 
2.10.1

[PATCH 09/18] [media] RedRat3: Move a variable assignment in redrat3_dev_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 14:07:03 +0200

* One local variable was set to an error code before a concrete
  error situation was detected. Thus move the corresponding assignment
  into three if branches to indicate a memory allocation failure there.

* Adjust a jump label according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index f6c21a1..f85117b 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -912,7 +912,7 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	struct usb_endpoint_descriptor *ep_out = NULL;
 	u8 addr, attrs;
 	int pipe, i;
-	int retval = -ENOMEM;
+	int retval;
 
 	uhi = intf->cur_altsetting;
 
@@ -951,21 +951,27 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 
 	/* allocate memory for our device state and initialize it */
 	rr3 = kzalloc(sizeof(*rr3), GFP_KERNEL);
-	if (!rr3)
+	if (!rr3) {
+		retval = -ENOMEM;
 		goto no_endpoints;
+	}
 
 	rr3->dev = &intf->dev;
 
 	/* set up bulk-in endpoint */
 	rr3->read_urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!rr3->read_urb)
-		goto error;
+	if (!rr3->read_urb) {
+		retval = -ENOMEM;
+		goto delete_rr;
+	}
 
 	rr3->ep_in = ep_in;
 	rr3->bulk_in_buf = usb_alloc_coherent(udev,
 		le16_to_cpu(ep_in->wMaxPacketSize), GFP_KERNEL, &rr3->dma_in);
-	if (!rr3->bulk_in_buf)
-		goto error;
+	if (!rr3->bulk_in_buf) {
+		retval = -ENOMEM;
+		goto delete_rr;
+	}
 
 	pipe = usb_rcvbulkpipe(udev, ep_in->bEndpointAddress);
 	usb_fill_bulk_urb(rr3->read_urb, udev, pipe, rr3->bulk_in_buf,
@@ -982,7 +988,7 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	/* might be all we need to do? */
 	retval = redrat3_enable_detector(rr3);
 	if (retval < 0)
-		goto error;
+		goto delete_rr;
 
 	/* store current hardware timeout, in µs */
 	rr3->hw_timeout = redrat3_get_timeout(rr3);
@@ -996,7 +1002,7 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	rr3->led.brightness_set = redrat3_brightness_set;
 	retval = led_classdev_register(&intf->dev, &rr3->led);
 	if (retval)
-		goto error;
+		goto delete_rr;
 
 	atomic_set(&rr3->flash, 0);
 	rr3->flash_urb = usb_alloc_urb(0, GFP_KERNEL);
@@ -1028,7 +1034,7 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 
 led_free_error:
 	led_classdev_unregister(&rr3->led);
-error:
+delete_rr:
 	redrat3_delete(rr3, rr3->udev);
 
 no_endpoints:
-- 
2.10.1

[PATCH 10/18] [media] RedRat3: Delete an unnecessary variable initialisation in redrat3_init_rc_dev()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 14:17:43 +0200

The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index f85117b..c43f43b 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -856,7 +856,7 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
 {
 	struct device *dev = rr3->dev;
 	struct rc_dev *rc;
-	int ret = -ENODEV;
+	int ret;
 	u16 prod = le16_to_cpu(rr3->udev->descriptor.idProduct);
 
 	rc = rc_allocate_device();
-- 
2.10.1

[PATCH 11/18] [media] RedRat3: Delete the variable "dev" in redrat3_init_rc_dev()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 14:40:11 +0200

Use the data structure member "dev" directly without assigning it
to an intermediate variable.
Thus delete the extra variable definition at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index c43f43b..b23a8bb 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -854,7 +854,6 @@ static void redrat3_led_complete(struct urb *urb)
 
 static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
 {
-	struct device *dev = rr3->dev;
 	struct rc_dev *rc;
 	int ret;
 	u16 prod = le16_to_cpu(rr3->udev->descriptor.idProduct);
@@ -873,7 +872,7 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
 	rc->input_name = rr3->name;
 	rc->input_phys = rr3->phys;
 	usb_to_input_id(rr3->udev, &rc->input_id);
-	rc->dev.parent = dev;
+	rc->dev.parent = rr3->dev;
 	rc->priv = rr3;
 	rc->driver_type = RC_DRIVER_IR_RAW;
 	rc->allowed_protocols = RC_BIT_ALL;
@@ -889,7 +888,7 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
 
 	ret = rc_register_device(rc);
 	if (ret < 0) {
-		dev_err(dev, "remote dev registration failed\n");
+		dev_err(rr3->dev, "remote dev registration failed\n");
 		goto out;
 	}
 
-- 
2.10.1

[PATCH 12/18] [media] RedRat3: Move a variable assignment in redrat3_init_rc_dev()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 14:50:05 +0200

Move the assignment for the local variable "prod" behind the source code
for a memory allocation by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index b23a8bb..002030f 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -856,12 +856,13 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
 {
 	struct rc_dev *rc;
 	int ret;
-	u16 prod = le16_to_cpu(rr3->udev->descriptor.idProduct);
+	u16 prod;
 
 	rc = rc_allocate_device();
 	if (!rc)
 		goto out;
 
+	prod = le16_to_cpu(rr3->udev->descriptor.idProduct);
 	snprintf(rr3->name, sizeof(rr3->name), "RedRat3%s "
 		 "Infrared Remote Transceiver (%04x:%04x)",
 		 prod == USB_RR3IIUSB_PRODUCT_ID ? "-II" : "",
-- 
2.10.1

[PATCH 13/18] [media] RedRat3: Return directly after a failed rc_allocate_device() in redrat3_init_rc_dev()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 14:54:46 +0200

Return directly after a call of the function "rc_allocate_device" failed
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 002030f..74d93dd 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -860,7 +860,7 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
 
 	rc = rc_allocate_device();
 	if (!rc)
-		goto out;
+		return NULL;
 
 	prod = le16_to_cpu(rr3->udev->descriptor.idProduct);
 	snprintf(rr3->name, sizeof(rr3->name), "RedRat3%s "
-- 
2.10.1

[PATCH 14/18] [media] RedRat3: Rename a jump label in redrat3_init_rc_dev()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 15:00:12 +0200

Adjust a jump label according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 74d93dd..055f214 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -890,12 +890,11 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
 	ret = rc_register_device(rc);
 	if (ret < 0) {
 		dev_err(rr3->dev, "remote dev registration failed\n");
-		goto out;
+		goto free_device;
 	}
 
 	return rc;
-
-out:
+free_device:
 	rc_free_device(rc);
 	return NULL;
 }
-- 
2.10.1

[PATCH 15/18] [media] RedRat3: Delete two variables in redrat3_set_timeout()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 15:40:33 +0200

* Use the data structure members "dev" and "udev" directly
  without assigning them to intermediate variables.
  Thus delete the extra variable definitions at the beginning.

* Fix indentation for the parameters of two function calls.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 055f214..e46a92a 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -476,8 +476,6 @@ static u32 redrat3_get_timeout(struct redrat3_dev *rr3)
 static int redrat3_set_timeout(struct rc_dev *rc_dev, unsigned int timeoutns)
 {
 	struct redrat3_dev *rr3 = rc_dev->priv;
-	struct usb_device *udev = rr3->udev;
-	struct device *dev = rr3->dev;
 	__be32 *timeout;
 	int ret;
 
@@ -486,13 +484,17 @@ static int redrat3_set_timeout(struct rc_dev *rc_dev, unsigned int timeoutns)
 		return -ENOMEM;
 
 	*timeout = cpu_to_be32(redrat3_us_to_len(timeoutns / 1000));
-	ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), RR3_SET_IR_PARAM,
-		     USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
-		     RR3_IR_IO_SIG_TIMEOUT, 0, timeout, sizeof(*timeout),
-		     HZ * 25);
-	dev_dbg(dev, "set ir parm timeout %d ret 0x%02x\n",
-						be32_to_cpu(*timeout), ret);
-
+	ret = usb_control_msg(rr3->udev,
+			      usb_sndctrlpipe(rr3->udev, 0),
+			      RR3_SET_IR_PARAM,
+			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+			      RR3_IR_IO_SIG_TIMEOUT,
+			      0,
+			      timeout,
+			      sizeof(*timeout),
+			      HZ * 25);
+	dev_dbg(rr3->dev, "set ir parm timeout %d ret 0x%02x\n",
+		be32_to_cpu(*timeout), ret);
 	if (ret == sizeof(*timeout)) {
 		rr3->hw_timeout = timeoutns / 1000;
 		ret = 0;
-- 
2.10.1

[PATCH 16/18] [media] RedRat3: Move a variable assignment in redrat3_set_timeout()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 17:13:41 +0200

Move the assignment for the local variable "rr3" behind the source code
for a memory allocation by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index e46a92a..06c9eea 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -475,7 +475,7 @@ static u32 redrat3_get_timeout(struct redrat3_dev *rr3)
 
 static int redrat3_set_timeout(struct rc_dev *rc_dev, unsigned int timeoutns)
 {
-	struct redrat3_dev *rr3 = rc_dev->priv;
+	struct redrat3_dev *rr3;
 	__be32 *timeout;
 	int ret;
 
@@ -484,6 +484,7 @@ static int redrat3_set_timeout(struct rc_dev *rc_dev, unsigned int timeoutns)
 		return -ENOMEM;
 
 	*timeout = cpu_to_be32(redrat3_us_to_len(timeoutns / 1000));
+	rr3 = rc_dev->priv;
 	ret = usb_control_msg(rr3->udev,
 			      usb_sndctrlpipe(rr3->udev, 0),
 			      RR3_SET_IR_PARAM,
-- 
2.10.1

[PATCH 17/18] [media] RedRat3: Adjust two checks for null pointers in redrat3_dev_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 17:40:34 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script "checkpatch.pl" pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 06c9eea..a09d5cb 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -923,8 +923,7 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 		ep = &uhi->endpoint[i].desc;
 		addr = ep->bEndpointAddress;
 		attrs = ep->bmAttributes;
-
-		if ((ep_in == NULL) &&
+		if (!ep_in &&
 		    ((addr & USB_ENDPOINT_DIR_MASK) == USB_DIR_IN) &&
 		    ((attrs & USB_ENDPOINT_XFERTYPE_MASK) ==
 		     USB_ENDPOINT_XFER_BULK)) {
@@ -935,7 +934,7 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 				ep_in = ep;
 		}
 
-		if ((ep_out == NULL) &&
+		if (!ep_out &&
 		    ((addr & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT) &&
 		    ((attrs & USB_ENDPOINT_XFERTYPE_MASK) ==
 		     USB_ENDPOINT_XFER_BULK)) {
-- 
2.10.1

[PATCH 18/18] [media] RedRat3: Combine substrings for six messages

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Oct 2016 17:50:11 +0200

The script "checkpatch.pl" pointed information out like the following.

WARNING: quoted string split across lines

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/redrat3.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index a09d5cb..b8c4b98 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -233,8 +233,7 @@ static void redrat3_dump_fw_error(struct redrat3_dev *rr3, int code)
 
 	/* Codes 0x20 through 0x2f are IR Firmware Errors */
 	case 0x20:
-		pr_cont("Initial signal pulse not long enough "
-			"to measure carrier frequency\n");
+		pr_cont("Initial signal pulse not long enough to measure carrier frequency\n");
 		break;
 	case 0x21:
 		pr_cont("Not enough length values allocated for signal\n");
@@ -246,18 +245,15 @@ static void redrat3_dump_fw_error(struct redrat3_dev *rr3, int code)
 		pr_cont("Too many signal repeats\n");
 		break;
 	case 0x28:
-		pr_cont("Insufficient memory available for IR signal "
-			"data memory allocation\n");
+		pr_cont("Insufficient memory available for IR signal data memory allocation\n");
 		break;
 	case 0x29:
-		pr_cont("Insufficient memory available "
-			"for IrDa signal data memory allocation\n");
+		pr_cont("Insufficient memory available for IrDa signal data memory allocation\n");
 		break;
 
 	/* Codes 0x30 through 0x3f are USB Firmware Errors */
 	case 0x30:
-		pr_cont("Insufficient memory available for bulk "
-			"transfer structure\n");
+		pr_cont("Insufficient memory available for bulk transfer structure\n");
 		break;
 
 	/*
@@ -269,8 +265,7 @@ static void redrat3_dump_fw_error(struct redrat3_dev *rr3, int code)
 			pr_cont("Signal capture has been terminated\n");
 		break;
 	case 0x41:
-		pr_cont("Attempt to set/get and unknown signal I/O "
-			"algorithm parameter\n");
+		pr_cont("Attempt to set/get and unknown signal I/O algorithm parameter\n");
 		break;
 	case 0x42:
 		pr_cont("Signal capture already started\n");
@@ -866,8 +861,8 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
 		return NULL;
 
 	prod = le16_to_cpu(rr3->udev->descriptor.idProduct);
-	snprintf(rr3->name, sizeof(rr3->name), "RedRat3%s "
-		 "Infrared Remote Transceiver (%04x:%04x)",
+	snprintf(rr3->name, sizeof(rr3->name),
+		 "RedRat3%s Infrared Remote Transceiver (%04x:%04x)",
 		 prod == USB_RR3IIUSB_PRODUCT_ID ? "-II" : "",
 		 le16_to_cpu(rr3->udev->descriptor.idVendor), prod);
 
-- 
2.10.1

Re: [PATCH 12/18] [media] RedRat3: Move a variable assignment in redrat3_init_rc_dev()

[Sean Young]

On Thu, Oct 13, 2016 at 06:39:23PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 13 Oct 2016 14:50:05 +0200
> 
> Move the assignment for the local variable "prod" behind the source code
> for a memory allocation by this function.

The redrat3 driver shouldn't be adding the usb vendor/product id to the
device name. A better patch would be to remove those from the snprintf
completely and to away with the local variable.

Sean

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/rc/redrat3.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
> index b23a8bb..002030f 100644
> --- a/drivers/media/rc/redrat3.c
> +++ b/drivers/media/rc/redrat3.c
> @@ -856,12 +856,13 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
>  {
>  	struct rc_dev *rc;
>  	int ret;
> -	u16 prod = le16_to_cpu(rr3->udev->descriptor.idProduct);
> +	u16 prod;
>  
>  	rc = rc_allocate_device();
>  	if (!rc)
>  		goto out;
>  
> +	prod = le16_to_cpu(rr3->udev->descriptor.idProduct);
>  	snprintf(rr3->name, sizeof(rr3->name), "RedRat3%s "
>  		 "Infrared Remote Transceiver (%04x:%04x)",
>  		 prod == USB_RR3IIUSB_PRODUCT_ID ? "-II" : "",
> -- 
> 2.10.1
> 
> --
> 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

Re: [media] RedRat3: Use kcalloc() in two functions?

[SF Markus Elfring]

> Markus, please stop being _so_ mechanical and use your
> brain a little too.  By definition, sizeof(char) == 1.
> 
> This _really_ should be kzalloc(RR3_FW_VERSION_LEN + 1,...)

Do you expect that function call examples like the following will be equivalent?

	zbuffer = kzalloc(123, …);
	cbuffer = kcalloc(123, 1, …);

Regards,
Markus

Re: [PATCH 02/18] [media] RedRat3: Move two assignments in redrat3_transmit_ir()

[Dan Carpenter]

I have asked you about six or seven times to only send bug fixes and
stop sending clean up patches.  You have refused.  But now I'm asking
you to stop randomly doing things without at least thinking about it for
a bit.

The original code was correct.

regards,
dan carpenter

[PATCH 0/5] [media] winbond-cir: Fine-tuning for four function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 14 Oct 2016 13:24:35 +0200

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

Markus Elfring (5):
  Use kmalloc_array() in wbcir_tx()
  Move a variable assignment in wbcir_tx()
  Move assignments for three variables in wbcir_shutdown()
  One variable and its check less in wbcir_shutdown() after error detection
  Move a variable assignment in two functions

 drivers/media/rc/winbond-cir.c | 95 +++++++++++++++++++-----------------------
 1 file changed, 44 insertions(+), 51 deletions(-)

-- 
2.10.1

[PATCH 1/5] [media] winbond-cir: Use kmalloc_array() in wbcir_tx()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 14 Oct 2016 07:19:00 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/winbond-cir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 95ae60e..59050f5 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -660,7 +660,7 @@ wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count)
 	unsigned i;
 	unsigned long flags;
 
-	buf = kmalloc(count * sizeof(*b), GFP_KERNEL);
+	buf = kmalloc_array(count, sizeof(*b), GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-- 
2.10.1

[PATCH 2/5] [media] winbond-cir: Move a variable assignment in wbcir_tx()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 14 Oct 2016 07:34:46 +0200

Move the assignment for the local variable "data" behind the source code
for a memory allocation by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/winbond-cir.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 59050f5..fd997f0 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -655,7 +655,7 @@ wbcir_txmask(struct rc_dev *dev, u32 mask)
 static int
 wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count)
 {
-	struct wbcir_data *data = dev->priv;
+	struct wbcir_data *data;
 	unsigned *buf;
 	unsigned i;
 	unsigned long flags;
@@ -668,6 +668,7 @@ wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count)
 	for (i = 0; i < count; i++)
 		buf[i] = DIV_ROUND_CLOSEST(b[i], 10);
 
+	data = dev->priv;
 	/* Not sure if this is possible, but better safe than sorry */
 	spin_lock_irqsave(&data->spinlock, flags);
 	if (data->txstate != WBCIR_TXSTATE_INACTIVE) {
-- 
2.10.1

[PATCH 3/5] [media] winbond-cir: Move assignments for three variables in wbcir_shutdown()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 14 Oct 2016 10:40:12 +0200

Move the setting for the local variables "mask", "match" and "rc6_csl"
behind the source code for a condition check by this function
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/winbond-cir.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index fd997f0..9d05e17 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -702,17 +702,17 @@ wbcir_shutdown(struct pnp_dev *device)
 	bool do_wake = true;
 	u8 match[11];
 	u8 mask[11];
-	u8 rc6_csl = 0;
+	u8 rc6_csl;
 	int i;
 
-	memset(match, 0, sizeof(match));
-	memset(mask, 0, sizeof(mask));
-
 	if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev)) {
 		do_wake = false;
 		goto finish;
 	}
 
+	rc6_csl = 0;
+	memset(match, 0, sizeof(match));
+	memset(mask, 0, sizeof(mask));
 	switch (protocol) {
 	case IR_PROTOCOL_RC5:
 		if (wake_sc > 0xFFF) {
-- 
2.10.1

[PATCH 4/5] [media] winbond-cir: One variable and its check less in wbcir_shutdown() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 14 Oct 2016 12:48:41 +0200

The local variable "do_wake" was set to "false" after an invalid system
setting was detected so that a bit of error handling was triggered.

* Replace these assignments by direct jumps to the source code with the
desired exception handling.

* Delete this status variable and a corresponding check which became
  unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/winbond-cir.c | 78 ++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 44 deletions(-)

diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 9d05e17..3d286b9 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -699,16 +699,13 @@ wbcir_shutdown(struct pnp_dev *device)
 {
 	struct device *dev = &device->dev;
 	struct wbcir_data *data = pnp_get_drvdata(device);
-	bool do_wake = true;
 	u8 match[11];
 	u8 mask[11];
 	u8 rc6_csl;
 	int i;
 
-	if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev)) {
-		do_wake = false;
-		goto finish;
-	}
+	if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev))
+		goto clear_bits;
 
 	rc6_csl = 0;
 	memset(match, 0, sizeof(match));
@@ -716,9 +713,8 @@ wbcir_shutdown(struct pnp_dev *device)
 	switch (protocol) {
 	case IR_PROTOCOL_RC5:
 		if (wake_sc > 0xFFF) {
-			do_wake = false;
 			dev_err(dev, "RC5 - Invalid wake scancode\n");
-			break;
+			goto clear_bits;
 		}
 
 		/* Mask = 13 bits, ex toggle */
@@ -735,9 +731,8 @@ wbcir_shutdown(struct pnp_dev *device)
 
 	case IR_PROTOCOL_NEC:
 		if (wake_sc > 0xFFFFFF) {
-			do_wake = false;
 			dev_err(dev, "NEC - Invalid wake scancode\n");
-			break;
+			goto clear_bits;
 		}
 
 		mask[0] = mask[1] = mask[2] = mask[3] = 0xFF;
@@ -757,9 +752,8 @@ wbcir_shutdown(struct pnp_dev *device)
 
 		if (wake_rc6mode == 0) {
 			if (wake_sc > 0xFFFF) {
-				do_wake = false;
 				dev_err(dev, "RC6 - Invalid wake scancode\n");
-				break;
+				goto clear_bits;
 			}
 
 			/* Command */
@@ -813,9 +807,8 @@ wbcir_shutdown(struct pnp_dev *device)
 			} else if (wake_sc <= 0x007FFFFF) {
 				rc6_csl = 60;
 			} else {
-				do_wake = false;
 				dev_err(dev, "RC6 - Invalid wake scancode\n");
-				break;
+				goto clear_bits;
 			}
 
 			/* Header */
@@ -825,49 +818,38 @@ wbcir_shutdown(struct pnp_dev *device)
 			mask[i++] = 0x0F;
 
 		} else {
-			do_wake = false;
 			dev_err(dev, "RC6 - Invalid wake mode\n");
+			goto clear_bits;
 		}
 
 		break;
 
 	default:
-		do_wake = false;
-		break;
+		goto clear_bits;
 	}
 
-finish:
-	if (do_wake) {
-		/* Set compare and compare mask */
-		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
-			       WBCIR_REGSEL_COMPARE | WBCIR_REG_ADDR0,
-			       0x3F);
-		outsb(data->wbase + WBCIR_REG_WCEIR_DATA, match, 11);
-		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
-			       WBCIR_REGSEL_MASK | WBCIR_REG_ADDR0,
-			       0x3F);
-		outsb(data->wbase + WBCIR_REG_WCEIR_DATA, mask, 11);
-
-		/* RC6 Compare String Len */
-		outb(rc6_csl, data->wbase + WBCIR_REG_WCEIR_CSL);
-
-		/* Clear status bits NEC_REP, BUFF, MSG_END, MATCH */
-		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_STS, 0x17, 0x17);
+	/* Set compare and compare mask */
+	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
+		       WBCIR_REGSEL_COMPARE | WBCIR_REG_ADDR0,
+		       0x3F);
+	outsb(data->wbase + WBCIR_REG_WCEIR_DATA, match, 11);
+	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
+		       WBCIR_REGSEL_MASK | WBCIR_REG_ADDR0,
+		       0x3F);
+	outsb(data->wbase + WBCIR_REG_WCEIR_DATA, mask, 11);
 
-		/* Clear BUFF_EN, Clear END_EN, Set MATCH_EN */
-		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x01, 0x07);
+	/* RC6 Compare String Len */
+	outb(rc6_csl, data->wbase + WBCIR_REG_WCEIR_CSL);
 
-		/* Set CEIR_EN */
-		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
-
-	} else {
-		/* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
-		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
+	/* Clear status bits NEC_REP, BUFF, MSG_END, MATCH */
+	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_STS, 0x17, 0x17);
 
-		/* Clear CEIR_EN */
-		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
-	}
+	/* Clear BUFF_EN, Clear END_EN, Set MATCH_EN */
+	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x01, 0x07);
 
+	/* Set CEIR_EN */
+	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
+set_irqmask:
 	/*
 	 * ACPI will set the HW disable bit for SP3 which means that the
 	 * output signals are left in an undefined state which may cause
@@ -876,6 +858,14 @@ wbcir_shutdown(struct pnp_dev *device)
 	 */
 	wbcir_set_irqmask(data, WBCIR_IRQ_NONE);
 	disable_irq(data->irq);
+	return;
+clear_bits:
+	/* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
+	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
+
+	/* Clear CEIR_EN */
+	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
+	goto set_irqmask;
 }
 
 static int
-- 
2.10.1

[PATCH 5/5] [media] winbond-cir: Move a variable assignment in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 14 Oct 2016 13:13:11 +0200

Move the assignment for the local variable "data" behind the source code
for condition checks by these functions.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/rc/winbond-cir.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 3d286b9..716b1fe 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -566,7 +566,7 @@ wbcir_set_carrier_report(struct rc_dev *dev, int enable)
 static int
 wbcir_txcarrier(struct rc_dev *dev, u32 carrier)
 {
-	struct wbcir_data *data = dev->priv;
+	struct wbcir_data *data;
 	unsigned long flags;
 	u8 val;
 	u32 freq;
@@ -592,6 +592,7 @@ wbcir_txcarrier(struct rc_dev *dev, u32 carrier)
 		break;
 	}
 
+	data = dev->priv;
 	spin_lock_irqsave(&data->spinlock, flags);
 	if (data->txstate != WBCIR_TXSTATE_INACTIVE) {
 		spin_unlock_irqrestore(&data->spinlock, flags);
@@ -611,7 +612,7 @@ wbcir_txcarrier(struct rc_dev *dev, u32 carrier)
 static int
 wbcir_txmask(struct rc_dev *dev, u32 mask)
 {
-	struct wbcir_data *data = dev->priv;
+	struct wbcir_data *data;
 	unsigned long flags;
 	u8 val;
 
@@ -637,6 +638,7 @@ wbcir_txmask(struct rc_dev *dev, u32 mask)
 		return -EINVAL;
 	}
 
+	data = dev->priv;
 	spin_lock_irqsave(&data->spinlock, flags);
 	if (data->txstate != WBCIR_TXSTATE_INACTIVE) {
 		spin_unlock_irqrestore(&data->spinlock, flags);
-- 
2.10.1

Re: [media] RedRat3: Move two assignments in redrat3_transmit_ir()?

[SF Markus Elfring]

> The original code was correct.

Your view can be appropriate for this function implementation to some degree.

I got the impression that it contains the specification of assignments
which will happen a bit too early here.
Is this a weakness for which software developers can care about?

Regards,
Markus

Re: [PATCH 4/5] [media] winbond-cir: One variable and its check less in wbcir_shutdown() after error detection

[Sean Young]

On Fri, Oct 14, 2016 at 01:44:02PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 14 Oct 2016 12:48:41 +0200
> 
> The local variable "do_wake" was set to "false" after an invalid system
> setting was detected so that a bit of error handling was triggered.
> 
> * Replace these assignments by direct jumps to the source code with the
> desired exception handling.
> 
> * Delete this status variable and a corresponding check which became
>   unnecessary with this refactoring.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/rc/winbond-cir.c | 78 ++++++++++++++++++------------------------
>  1 file changed, 34 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index 9d05e17..3d286b9 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -699,16 +699,13 @@ wbcir_shutdown(struct pnp_dev *device)
>  {
>  	struct device *dev = &device->dev;
>  	struct wbcir_data *data = pnp_get_drvdata(device);
> -	bool do_wake = true;
>  	u8 match[11];
>  	u8 mask[11];
>  	u8 rc6_csl;
>  	int i;
>  
> -	if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev)) {
> -		do_wake = false;
> -		goto finish;
> -	}
> +	if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev))
> +		goto clear_bits;
>  
>  	rc6_csl = 0;
>  	memset(match, 0, sizeof(match));
> @@ -716,9 +713,8 @@ wbcir_shutdown(struct pnp_dev *device)
>  	switch (protocol) {
>  	case IR_PROTOCOL_RC5:
>  		if (wake_sc > 0xFFF) {
> -			do_wake = false;
>  			dev_err(dev, "RC5 - Invalid wake scancode\n");
> -			break;
> +			goto clear_bits;
>  		}
>  
>  		/* Mask = 13 bits, ex toggle */
> @@ -735,9 +731,8 @@ wbcir_shutdown(struct pnp_dev *device)
>  
>  	case IR_PROTOCOL_NEC:
>  		if (wake_sc > 0xFFFFFF) {
> -			do_wake = false;
>  			dev_err(dev, "NEC - Invalid wake scancode\n");
> -			break;
> +			goto clear_bits;
>  		}
>  
>  		mask[0] = mask[1] = mask[2] = mask[3] = 0xFF;
> @@ -757,9 +752,8 @@ wbcir_shutdown(struct pnp_dev *device)
>  
>  		if (wake_rc6mode == 0) {
>  			if (wake_sc > 0xFFFF) {
> -				do_wake = false;
>  				dev_err(dev, "RC6 - Invalid wake scancode\n");
> -				break;
> +				goto clear_bits;
>  			}
>  
>  			/* Command */
> @@ -813,9 +807,8 @@ wbcir_shutdown(struct pnp_dev *device)
>  			} else if (wake_sc <= 0x007FFFFF) {
>  				rc6_csl = 60;
>  			} else {
> -				do_wake = false;
>  				dev_err(dev, "RC6 - Invalid wake scancode\n");
> -				break;
> +				goto clear_bits;
>  			}
>  
>  			/* Header */
> @@ -825,49 +818,38 @@ wbcir_shutdown(struct pnp_dev *device)
>  			mask[i++] = 0x0F;
>  
>  		} else {
> -			do_wake = false;
>  			dev_err(dev, "RC6 - Invalid wake mode\n");
> +			goto clear_bits;
>  		}
>  
>  		break;
>  
>  	default:
> -		do_wake = false;
> -		break;
> +		goto clear_bits;
>  	}
>  
> -finish:
> -	if (do_wake) {
> -		/* Set compare and compare mask */
> -		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
> -			       WBCIR_REGSEL_COMPARE | WBCIR_REG_ADDR0,
> -			       0x3F);
> -		outsb(data->wbase + WBCIR_REG_WCEIR_DATA, match, 11);
> -		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
> -			       WBCIR_REGSEL_MASK | WBCIR_REG_ADDR0,
> -			       0x3F);
> -		outsb(data->wbase + WBCIR_REG_WCEIR_DATA, mask, 11);
> -
> -		/* RC6 Compare String Len */
> -		outb(rc6_csl, data->wbase + WBCIR_REG_WCEIR_CSL);
> -
> -		/* Clear status bits NEC_REP, BUFF, MSG_END, MATCH */
> -		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_STS, 0x17, 0x17);
> +	/* Set compare and compare mask */
> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
> +		       WBCIR_REGSEL_COMPARE | WBCIR_REG_ADDR0,
> +		       0x3F);
> +	outsb(data->wbase + WBCIR_REG_WCEIR_DATA, match, 11);
> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
> +		       WBCIR_REGSEL_MASK | WBCIR_REG_ADDR0,
> +		       0x3F);
> +	outsb(data->wbase + WBCIR_REG_WCEIR_DATA, mask, 11);
>  
> -		/* Clear BUFF_EN, Clear END_EN, Set MATCH_EN */
> -		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x01, 0x07);
> +	/* RC6 Compare String Len */
> +	outb(rc6_csl, data->wbase + WBCIR_REG_WCEIR_CSL);
>  
> -		/* Set CEIR_EN */
> -		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
> -
> -	} else {
> -		/* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
> -		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
> +	/* Clear status bits NEC_REP, BUFF, MSG_END, MATCH */
> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_STS, 0x17, 0x17);
>  
> -		/* Clear CEIR_EN */
> -		wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
> -	}
> +	/* Clear BUFF_EN, Clear END_EN, Set MATCH_EN */
> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x01, 0x07);
>  
> +	/* Set CEIR_EN */
> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
> +set_irqmask:
>  	/*
>  	 * ACPI will set the HW disable bit for SP3 which means that the
>  	 * output signals are left in an undefined state which may cause
> @@ -876,6 +858,14 @@ wbcir_shutdown(struct pnp_dev *device)
>  	 */
>  	wbcir_set_irqmask(data, WBCIR_IRQ_NONE);
>  	disable_irq(data->irq);
> +	return;
> +clear_bits:
> +	/* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
> +
> +	/* Clear CEIR_EN */
> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
> +	goto set_irqmask;

I'm not convinced that adding a goto which goes backwards is making this
code any more readible, just so that a local variable can be dropped.


Sean

>  }
>  
>  static int
> -- 
> 2.10.1
> 
> --
> 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

Re: [PATCH 04/18] [media] RedRat3: One function call less in redrat3_transmit_ir() after error detection

[Sean Young]

On Thu, Oct 13, 2016 at 06:24:46PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 13 Oct 2016 10:50:24 +0200
> 
> The kfree() function was called in one case by the
> redrat3_transmit_ir() function during error handling
> even if the passed variable contained a null pointer.
> 
> * Adjust jump targets according to the Linux coding style convention.
> 
> * Move the resetting for the data structure member "transmitting"
>   at the end.
> 
> * Delete initialisations for the variables "irdata" and "sample_lens"
>   at the beginning which became unnecessary with this refactoring.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/rc/redrat3.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
> index 7ae2ced..71e901d 100644
> --- a/drivers/media/rc/redrat3.c
> +++ b/drivers/media/rc/redrat3.c
> @@ -723,10 +723,10 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
>  {
>  	struct redrat3_dev *rr3 = rcdev->priv;
>  	struct device *dev = rr3->dev;
> -	struct redrat3_irdata *irdata = NULL;
> +	struct redrat3_irdata *irdata;
>  	int ret, ret_len;
>  	int lencheck, cur_sample_len, pipe;
> -	int *sample_lens = NULL;
> +	int *sample_lens;
>  	u8 curlencheck;
>  	unsigned i, sendbuf_len;
>  
> @@ -747,7 +747,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
>  	irdata = kzalloc(sizeof(*irdata), GFP_KERNEL);
>  	if (!irdata) {
>  		ret = -ENOMEM;
> -		goto out;
> +		goto free_sample;
>  	}
>  
>  	/* rr3 will disable rc detector on transmit */
> @@ -776,7 +776,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
>  				curlencheck++;
>  			} else {
>  				ret = -EINVAL;
> -				goto out;
> +				goto reset_member;
>  			}
>  		}
>  		irdata->sigdata[i] = lencheck;
> @@ -811,14 +811,12 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
>  		dev_err(dev, "Error: control msg send failed, rc %d\n", ret);
>  	else
>  		ret = count;
> -
> -out:
> -	kfree(irdata);
> -	kfree(sample_lens);
> -
> +reset_member:
>  	rr3->transmitting = false;
>  	/* rr3 re-enables rc detector because it was enabled before */
> -
> +	kfree(irdata);
> +free_sample:
> +	kfree(sample_lens);

In this error path, rr3->transmitting is not set to false so now the driver
will never allow you transmit again.

Also this patch does not apply against latest.

Sean

>  	return ret;
>  }
>  
> -- 
> 2.10.1
> 
> --
> 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

Re: [PATCH 4/5] [media] winbond-cir: One variable and its check less in wbcir_shutdown() after error detection

[SF Markus Elfring]

>> +	/* Set CEIR_EN */
>> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
>> +set_irqmask:
>>  	/*
>>  	 * ACPI will set the HW disable bit for SP3 which means that the
>>  	 * output signals are left in an undefined state which may cause
>> @@ -876,6 +858,14 @@ wbcir_shutdown(struct pnp_dev *device)
>>  	 */
>>  	wbcir_set_irqmask(data, WBCIR_IRQ_NONE);
>>  	disable_irq(data->irq);
>> +	return;
>> +clear_bits:
>> +	/* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
>> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
>> +
>> +	/* Clear CEIR_EN */
>> +	wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
>> +	goto set_irqmask;
> 
> I'm not convinced that adding a goto which goes backwards is making this
> code any more readible, just so that a local variable can be dropped.

Thanks for your feedback.

Is such a "backward jump" usual and finally required when you would like
to move a bit of common error handling code to the end without using extra
local variables and a few statements should still be performed after it?

Regards,
Markus

Re: [PATCH 04/18] [media] RedRat3: One function call less in redrat3_transmit_ir() after error detection

[SF Markus Elfring]

>> diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
>> index 7ae2ced..71e901d 100644
>> --- a/drivers/media/rc/redrat3.c
>> +++ b/drivers/media/rc/redrat3.c
>> @@ -723,10 +723,10 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
>>  {
>>  	struct redrat3_dev *rr3 = rcdev->priv;
>>  	struct device *dev = rr3->dev;
>> -	struct redrat3_irdata *irdata = NULL;
>> +	struct redrat3_irdata *irdata;
>>  	int ret, ret_len;
>>  	int lencheck, cur_sample_len, pipe;
>> -	int *sample_lens = NULL;
>> +	int *sample_lens;
>>  	u8 curlencheck;
>>  	unsigned i, sendbuf_len;
>>  
>> @@ -747,7 +747,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
>>  	irdata = kzalloc(sizeof(*irdata), GFP_KERNEL);
>>  	if (!irdata) {
>>  		ret = -ENOMEM;
>> -		goto out;
>> +		goto free_sample;
>>  	}
>>  
>>  	/* rr3 will disable rc detector on transmit */
>> @@ -776,7 +776,7 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
>>  				curlencheck++;
>>  			} else {
>>  				ret = -EINVAL;
>> -				goto out;
>> +				goto reset_member;
>>  			}
>>  		}
>>  		irdata->sigdata[i] = lencheck;
>> @@ -811,14 +811,12 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
>>  		dev_err(dev, "Error: control msg send failed, rc %d\n", ret);
>>  	else
>>  		ret = count;
>> -
>> -out:
>> -	kfree(irdata);
>> -	kfree(sample_lens);
>> -
>> +reset_member:
>>  	rr3->transmitting = false;
>>  	/* rr3 re-enables rc detector because it was enabled before */
>> -
>> +	kfree(irdata);
>> +free_sample:
>> +	kfree(sample_lens);
> 
> In this error path, rr3->transmitting is not set to false

Can it be that this reset is not needed because it should have still got this value already
in the software refactoring I proposed here?


> so now the driver will never allow you transmit again.

I have got an other impression.


> Also this patch does not apply against latest.

Do you want that I rebase my update suggestion for this software module on a published commit
that is more recent than 2016-09-22 (d6ae162bd13998a6511e5efbc7c19ab542ba1555 for example)?

Regards,
Markus

Re: [PATCH 1/5] [media] winbond-cir: Use kmalloc_array() in wbcir_tx()

[David Härdeman]

October 14, 2016 1:41 PM, "SF Markus Elfring" <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 14 Oct 2016 07:19:00 +0200
> 
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Sure...why not...

Signed-off-by: David Härdeman <david@hardeman.nu>


> ---
> drivers/media/rc/winbond-cir.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index 95ae60e..59050f5 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -660,7 +660,7 @@ wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count)
> unsigned i;
> unsigned long flags;
> 
> - buf = kmalloc(count * sizeof(*b), GFP_KERNEL);
> + buf = kmalloc_array(count, sizeof(*b), GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
> 
> -- 
> 2.10.1

Re: [PATCH 2/5] [media] winbond-cir: Move a variable assignment in wbcir_tx()

[David Härdeman]

October 14, 2016 1:42 PM, "SF Markus Elfring" <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 14 Oct 2016 07:34:46 +0200
> 
> Move the assignment for the local variable "data" behind the source code
> for a memory allocation by this function.

Sorry, I can't see what the point is?


> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/media/rc/winbond-cir.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index 59050f5..fd997f0 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -655,7 +655,7 @@ wbcir_txmask(struct rc_dev *dev, u32 mask)
> static int
> wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count)
> {
> - struct wbcir_data *data = dev->priv;
> + struct wbcir_data *data;
> unsigned *buf;
> unsigned i;
> unsigned long flags;
> @@ -668,6 +668,7 @@ wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count)
> for (i = 0; i < count; i++)
> buf[i] = DIV_ROUND_CLOSEST(b[i], 10);
> 
> + data = dev->priv;
> /* Not sure if this is possible, but better safe than sorry */
> spin_lock_irqsave(&data->spinlock, flags);
> if (data->txstate != WBCIR_TXSTATE_INACTIVE) {
> -- 
> 2.10.1

Re: [PATCH 3/5] [media] winbond-cir: Move assignments for three variables in wbcir_shutdown()

[David Härdeman]

October 14, 2016 1:43 PM, "SF Markus Elfring" <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 14 Oct 2016 10:40:12 +0200
> 
> Move the setting for the local variables "mask", "match" and "rc6_csl"
> behind the source code for a condition check by this function
> at the beginning.
 
Again, I can't see what the point is?

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/media/rc/winbond-cir.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index fd997f0..9d05e17 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -702,17 +702,17 @@ wbcir_shutdown(struct pnp_dev *device)
> bool do_wake = true;
> u8 match[11];
> u8 mask[11];
> - u8 rc6_csl = 0;
> + u8 rc6_csl;
> int i;
> 
> - memset(match, 0, sizeof(match));
> - memset(mask, 0, sizeof(mask));
> -
> if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev)) {
> do_wake = false;
> goto finish;
> }
> 
> + rc6_csl = 0;
> + memset(match, 0, sizeof(match));
> + memset(mask, 0, sizeof(mask));
> switch (protocol) {
> case IR_PROTOCOL_RC5:
> if (wake_sc > 0xFFF) {
> -- 
> 2.10.1

Re: [PATCH 4/5] [media] winbond-cir: One variable and its check less in wbcir_shutdown() after error detection

[David Härdeman]

October 15, 2016 3:30 PM, "Sean Young" <sean@mess.org> wrote:
> On Fri, Oct 14, 2016 at 01:44:02PM +0200, SF Markus Elfring wrote:
> 
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Fri, 14 Oct 2016 12:48:41 +0200
>> 
>> The local variable "do_wake" was set to "false" after an invalid system
>> setting was detected so that a bit of error handling was triggered.
>> 
>> * Replace these assignments by direct jumps to the source code with the
>> desired exception handling.
>> 
>> * Delete this status variable and a corresponding check which became
>> unnecessary with this refactoring.
>> 
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>> drivers/media/rc/winbond-cir.c | 78 ++++++++++++++++++------------------------
>> 1 file changed, 34 insertions(+), 44 deletions(-)
>> 
>> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
>> index 9d05e17..3d286b9 100644
>> --- a/drivers/media/rc/winbond-cir.c
>> +++ b/drivers/media/rc/winbond-cir.c
>> @@ -699,16 +699,13 @@ wbcir_shutdown(struct pnp_dev *device)
>> {
>> struct device *dev = &device->dev;
>> struct wbcir_data *data = pnp_get_drvdata(device);
>> - bool do_wake = true;
>> u8 match[11];
>> u8 mask[11];
>> u8 rc6_csl;
>> int i;
>> 
>> - if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev)) {
>> - do_wake = false;
>> - goto finish;
>> - }
>> + if (wake_sc == INVALID_SCANCODE || !device_may_wakeup(dev))
>> + goto clear_bits;
>> 
>> rc6_csl = 0;
>> memset(match, 0, sizeof(match));
>> @@ -716,9 +713,8 @@ wbcir_shutdown(struct pnp_dev *device)
>> switch (protocol) {
>> case IR_PROTOCOL_RC5:
>> if (wake_sc > 0xFFF) {
>> - do_wake = false;
>> dev_err(dev, "RC5 - Invalid wake scancode\n");
>> - break;
>> + goto clear_bits;
>> }
>> 
>> /* Mask = 13 bits, ex toggle */
>> @@ -735,9 +731,8 @@ wbcir_shutdown(struct pnp_dev *device)
>> 
>> case IR_PROTOCOL_NEC:
>> if (wake_sc > 0xFFFFFF) {
>> - do_wake = false;
>> dev_err(dev, "NEC - Invalid wake scancode\n");
>> - break;
>> + goto clear_bits;
>> }
>> 
>> mask[0] = mask[1] = mask[2] = mask[3] = 0xFF;
>> @@ -757,9 +752,8 @@ wbcir_shutdown(struct pnp_dev *device)
>> 
>> if (wake_rc6mode == 0) {
>> if (wake_sc > 0xFFFF) {
>> - do_wake = false;
>> dev_err(dev, "RC6 - Invalid wake scancode\n");
>> - break;
>> + goto clear_bits;
>> }
>> 
>> /* Command */
>> @@ -813,9 +807,8 @@ wbcir_shutdown(struct pnp_dev *device)
>> } else if (wake_sc <= 0x007FFFFF) {
>> rc6_csl = 60;
>> } else {
>> - do_wake = false;
>> dev_err(dev, "RC6 - Invalid wake scancode\n");
>> - break;
>> + goto clear_bits;
>> }
>> 
>> /* Header */
>> @@ -825,49 +818,38 @@ wbcir_shutdown(struct pnp_dev *device)
>> mask[i++] = 0x0F;
>> 
>> } else {
>> - do_wake = false;
>> dev_err(dev, "RC6 - Invalid wake mode\n");
>> + goto clear_bits;
>> }
>> 
>> break;
>> 
>> default:
>> - do_wake = false;
>> - break;
>> + goto clear_bits;
>> }
>> 
>> -finish:
>> - if (do_wake) {
>> - /* Set compare and compare mask */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
>> - WBCIR_REGSEL_COMPARE | WBCIR_REG_ADDR0,
>> - 0x3F);
>> - outsb(data->wbase + WBCIR_REG_WCEIR_DATA, match, 11);
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
>> - WBCIR_REGSEL_MASK | WBCIR_REG_ADDR0,
>> - 0x3F);
>> - outsb(data->wbase + WBCIR_REG_WCEIR_DATA, mask, 11);
>> -
>> - /* RC6 Compare String Len */
>> - outb(rc6_csl, data->wbase + WBCIR_REG_WCEIR_CSL);
>> -
>> - /* Clear status bits NEC_REP, BUFF, MSG_END, MATCH */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_STS, 0x17, 0x17);
>> + /* Set compare and compare mask */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
>> + WBCIR_REGSEL_COMPARE | WBCIR_REG_ADDR0,
>> + 0x3F);
>> + outsb(data->wbase + WBCIR_REG_WCEIR_DATA, match, 11);
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_INDEX,
>> + WBCIR_REGSEL_MASK | WBCIR_REG_ADDR0,
>> + 0x3F);
>> + outsb(data->wbase + WBCIR_REG_WCEIR_DATA, mask, 11);
>> 
>> - /* Clear BUFF_EN, Clear END_EN, Set MATCH_EN */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x01, 0x07);
>> + /* RC6 Compare String Len */
>> + outb(rc6_csl, data->wbase + WBCIR_REG_WCEIR_CSL);
>> 
>> - /* Set CEIR_EN */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
>> -
>> - } else {
>> - /* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
>> + /* Clear status bits NEC_REP, BUFF, MSG_END, MATCH */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_STS, 0x17, 0x17);
>> 
>> - /* Clear CEIR_EN */
>> - wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
>> - }
>> + /* Clear BUFF_EN, Clear END_EN, Set MATCH_EN */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x01, 0x07);
>> 
>> + /* Set CEIR_EN */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
>> +set_irqmask:
>> /*
>> * ACPI will set the HW disable bit for SP3 which means that the
>> * output signals are left in an undefined state which may cause
>> @@ -876,6 +858,14 @@ wbcir_shutdown(struct pnp_dev *device)
>> */
>> wbcir_set_irqmask(data, WBCIR_IRQ_NONE);
>> disable_irq(data->irq);
>> + return;
>> +clear_bits:
>> + /* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
>> +
>> + /* Clear CEIR_EN */
>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
>> + goto set_irqmask;
> 
> I'm not convinced that adding a goto which goes backwards is making this
> code any more readible, just so that a local variable can be dropped.
> 

Agreed.

Re: [PATCH 4/5] [media] winbond-cir: One variable and its check less in wbcir_shutdown() after error detection

[David Härdeman]

October 15, 2016 6:42 PM, "SF Markus Elfring" <elfring@users.sourceforge.net> wrote:
>>> + /* Set CEIR_EN */
>>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
>>> +set_irqmask:
>>> /*
>>> * ACPI will set the HW disable bit for SP3 which means that the
>>> * output signals are left in an undefined state which may cause
>>> @@ -876,6 +858,14 @@ wbcir_shutdown(struct pnp_dev *device)
>>> */
>>> wbcir_set_irqmask(data, WBCIR_IRQ_NONE);
>>> disable_irq(data->irq);
>>> + return;
>>> +clear_bits:
>>> + /* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
>>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
>>> +
>>> + /* Clear CEIR_EN */
>>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
>>> + goto set_irqmask;
>> 
>> I'm not convinced that adding a goto which goes backwards is making this
>> code any more readible, just so that a local variable can be dropped.
> 
> Thanks for your feedback.
> 
> Is such a "backward jump" usual and finally required when you would like
> to move a bit of common error handling code to the end without using extra
> local variables and a few statements should still be performed after it?
> 

I'm sorry, I can't parse this.

Re: [PATCH 5/5] [media] winbond-cir: Move a variable assignment in two functions

[David Härdeman]

October 14, 2016 1:45 PM, "SF Markus Elfring" <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 14 Oct 2016 13:13:11 +0200
> 
> Move the assignment for the local variable "data" behind the source code
> for condition checks by these functions.

Why?

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/media/rc/winbond-cir.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index 3d286b9..716b1fe 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -566,7 +566,7 @@ wbcir_set_carrier_report(struct rc_dev *dev, int enable)
> static int
> wbcir_txcarrier(struct rc_dev *dev, u32 carrier)
> {
> - struct wbcir_data *data = dev->priv;
> + struct wbcir_data *data;
> unsigned long flags;
> u8 val;
> u32 freq;
> @@ -592,6 +592,7 @@ wbcir_txcarrier(struct rc_dev *dev, u32 carrier)
> break;
> }
> 
> + data = dev->priv;
> spin_lock_irqsave(&data->spinlock, flags);
> if (data->txstate != WBCIR_TXSTATE_INACTIVE) {
> spin_unlock_irqrestore(&data->spinlock, flags);
> @@ -611,7 +612,7 @@ wbcir_txcarrier(struct rc_dev *dev, u32 carrier)
> static int
> wbcir_txmask(struct rc_dev *dev, u32 mask)
> {
> - struct wbcir_data *data = dev->priv;
> + struct wbcir_data *data;
> unsigned long flags;
> u8 val;
> 
> @@ -637,6 +638,7 @@ wbcir_txmask(struct rc_dev *dev, u32 mask)
> return -EINVAL;
> }
> 
> + data = dev->priv;
> spin_lock_irqsave(&data->spinlock, flags);
> if (data->txstate != WBCIR_TXSTATE_INACTIVE) {
> spin_unlock_irqrestore(&data->spinlock, flags);
> -- 
> 2.10.1

Re: [media] winbond-cir: Move a variable assignment in wbcir_tx()

[SF Markus Elfring]

>> Move the assignment for the local variable "data" behind the source code
>> for a memory allocation by this function.
> 
> Sorry, I can't see what the point is?

* How do you think about to avoid a variable assignment in case
  that this memory allocation failed anyhow?

* Do you care for data access locality?

Regards,
Markus

Re: [media] winbond-cir: Move assignments for three variables in wbcir_shutdown()

[SF Markus Elfring]

>> Move the setting for the local variables "mask", "match" and "rc6_csl"
>> behind the source code for a condition check by this function
>> at the beginning.
>  
> Again, I can't see what the point is?

* How do you think about to set these variables only after the initial
  check succeded?

* Do you care for data access locality?

Regards,
Markus

Re: [media] winbond-cir: Move a variable assignment in wbcir_tx()

[David Härdeman]

October 19, 2016 3:32 PM, "SF Markus Elfring" <elfring@users.sourceforge.net> wrote:
>>> Move the assignment for the local variable "data" behind the source code
>>> for a memory allocation by this function.
>> 
>> Sorry, I can't see what the point is?
> 
> * How do you think about to avoid a variable assignment in case
> that this memory allocation failed anyhow?

There is no memory allocation that can fail at this point.

> * Do you care for data access locality?

Not unless you can show measurable performance improvements?

Re: [media] winbond-cir: Move assignments for three variables in wbcir_shutdown()

[David Härdeman]

October 19, 2016 3:38 PM, "SF Markus Elfring" <elfring@users.sourceforge.net> wrote:
>>> Move the setting for the local variables "mask", "match" and "rc6_csl"
>>> behind the source code for a condition check by this function
>>> at the beginning.
>> 
>> Again, I can't see what the point is?
> 
> * How do you think about to set these variables only after the initial
> check succeded?

I prefer setting variables early so that no thinking about whether they're initialized or not is necessary later. 

> * Do you care for data access locality?

Not unless you can show measurable performance improvements?

Re: [PATCH 4/5] [media] winbond-cir: One variable and its check less in wbcir_shutdown() after error detection

[SF Markus Elfring]

>>>> + /* Set CEIR_EN */
>>>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x01, 0x01);
>>>> +set_irqmask:
>>>> /*
>>>> * ACPI will set the HW disable bit for SP3 which means that the
>>>> * output signals are left in an undefined state which may cause
>>>> @@ -876,6 +858,14 @@ wbcir_shutdown(struct pnp_dev *device)
>>>> */
>>>> wbcir_set_irqmask(data, WBCIR_IRQ_NONE);
>>>> disable_irq(data->irq);
>>>> + return;
>>>> +clear_bits:
>>>> + /* Clear BUFF_EN, Clear END_EN, Clear MATCH_EN */
>>>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_EV_EN, 0x00, 0x07);
>>>> +
>>>> + /* Clear CEIR_EN */
>>>> + wbcir_set_bits(data->wbase + WBCIR_REG_WCEIR_CTL, 0x00, 0x01);
>>>> + goto set_irqmask;
>>>
>>> I'm not convinced that adding a goto which goes backwards is making this
>>> code any more readible, just so that a local variable can be dropped.
>>
>> Thanks for your feedback.
>>
>> Is such a "backward jump" usual and finally required when you would like
>> to move a bit of common error handling code to the end without using extra
>> local variables and a few statements should still be performed after it?
>>
> 
> I'm sorry, I can't parse this.

Can an other update suggestion like "[PATCH 6/6] crypto-caamhash:
Move common error handling code in two functions" explain this technique
a bit better in principle?
https://patchwork.kernel.org/patch/9333861/
https://lkml.kernel.org/r/<baa5db91-27e7-ecab-f2c9-29e549b6e5f0@users.sourceforge.net>

Regards,
Markus

Re: [media] winbond-cir: Move a variable assignment in two functions

[SF Markus Elfring]

>> Move the assignment for the local variable "data" behind the source code
>> for condition checks by these functions.
> 
> Why?

* Would you like to set these variables only after the initial
  check succeeded?

* Do you care for data access locality also in these cases?

Regards,
Markus

Re: [media] winbond-cir: Move a variable assignment in wbcir_tx()

[SF Markus Elfring]

>> * How do you think about to avoid a variable assignment in case
>> that this memory allocation failed anyhow?
> 
> There is no memory allocation that can fail at this point.

Do you really know the failure probability for a call of the
function "kmalloc" (within the function "wbcir_tx") under all
possible run time situations?


>> * Do you care for data access locality?
> 
> Not unless you can show measurable performance improvements?

Did any software developer (before me) dare anything in this direction?

Regards,
Markus

Re: [media] winbond-cir: Move assignments for three variables in wbcir_shutdown()

[SF Markus Elfring]

>>>> Move the setting for the local variables "mask", "match" and "rc6_csl"
>>>> behind the source code for a condition check by this function
>>>> at the beginning.
>>>
>>> Again, I can't see what the point is?
>>
>> * How do you think about to set these variables only after the initial
>> check succeded?
> 
> I prefer setting variables early so that no thinking about
> whether they're initialized or not is necessary later.

* How do you think about to reduce the scope for these variables then?

* Would you dare to move the corresponding source code into a separate function?

Regards,
Markus

Re: [PATCH 14/18] [media] RedRat3: Rename a jump label in redrat3_init_rc_dev()

[Mauro Carvalho Chehab]

Em Thu, 13 Oct 2016 18:42:16 +0200
SF Markus Elfring <elfring@users.sourceforge.net> escreveu:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 13 Oct 2016 15:00:12 +0200
> 
> Adjust a jump label according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/rc/redrat3.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
> index 74d93dd..055f214 100644
> --- a/drivers/media/rc/redrat3.c
> +++ b/drivers/media/rc/redrat3.c
> @@ -890,12 +890,11 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
>  	ret = rc_register_device(rc);
>  	if (ret < 0) {
>  		dev_err(rr3->dev, "remote dev registration failed\n");
> -		goto out;
> +		goto free_device;
>  	}
>  
>  	return rc;
> -
> -out:
> +free_device:
>  	rc_free_device(rc);
>  	return NULL;
>  }

I don't see *any* sense on patches like this. Please don't flood me with
useless patches like that.

I'll silently ignore any patches like this during my review.

Regards,
Mauro



Thanks,
Mauro

Re: [PATCH 14/18] [media] RedRat3: Rename a jump label in redrat3_init_rc_dev()

[Mauro Carvalho Chehab]

Em Fri, 18 Nov 2016 10:52:40 -0200
Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:

> Em Thu, 13 Oct 2016 18:42:16 +0200
> SF Markus Elfring <elfring@users.sourceforge.net> escreveu:
> 
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Thu, 13 Oct 2016 15:00:12 +0200
> > 
> > Adjust a jump label according to the Linux coding style convention.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/media/rc/redrat3.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
> > index 74d93dd..055f214 100644
> > --- a/drivers/media/rc/redrat3.c
> > +++ b/drivers/media/rc/redrat3.c
> > @@ -890,12 +890,11 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
> >  	ret = rc_register_device(rc);
> >  	if (ret < 0) {
> >  		dev_err(rr3->dev, "remote dev registration failed\n");
> > -		goto out;
> > +		goto free_device;
> >  	}
> >  
> >  	return rc;
> > -
> > -out:
> > +free_device:
> >  	rc_free_device(rc);
> >  	return NULL;
> >  }  
> 
> I don't see *any* sense on patches like this. Please don't flood me with
> useless patches like that.
> 
> I'll silently ignore any patches like this during my review.

Btw:
	[PATCH 14/18] [media] RedRat3: Rename a jump label in redrat3_init_rc_dev()

Don't use CamelCase for the name of the driver. We don't use CamelCase
inside the Kernel, as it violates its coding style. Also, it means
nothing, as the name of this driver, and its c file is "redhat3".

Thanks,
Mauro

Re: [PATCH 1/2] uvc_v4l2: Use memdup_user() rather than duplicating its implementation

[Laurent Pinchart]

Hi Markus,

Thank you for the patch.

On Friday 19 Aug 2016 11:23:18 SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 19 Aug 2016 10:50:05 +0200
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree.

> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index 05eed4b..a7e12fd 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -70,14 +70,9 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
> *chain, }
> 
>  		size = xmap->menu_count * sizeof(*map->menu_info);
> -		map->menu_info = kmalloc(size, GFP_KERNEL);
> -		if (map->menu_info == NULL) {
> -			ret = -ENOMEM;
> -			goto done;
> -		}
> -
> -		if (copy_from_user(map->menu_info, xmap->menu_info, size)) {
> -			ret = -EFAULT;
> +		map->menu_info = memdup_user(xmap->menu_info, size);
> +		if (IS_ERR(map->menu_info)) {
> +			ret = PTR_ERR(map->menu_info);
>  			goto done;
>  		}

-- 
Regards,

Laurent Pinchart