[PATCH 1/2] usb drivers: use BUG_ON() instead of if () BUG

[PATCH 1/2] usb drivers: use BUG_ON() instead of if () BUG

[Mauro Carvalho Chehab]

Some USB drivers have a logic at the VB buffer handling like:
	if (in_interrupt())
		BUG();
Use, instead:
	BUG_ON(in_interrupt());

Btw, this logic looks weird on my eyes. We should convert them
to use VB2, in order to avoid those crappy things.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c
index 855a708387c6..47a98a2014a5 100644
--- a/drivers/media/usb/cx231xx/cx231xx-417.c
+++ b/drivers/media/usb/cx231xx/cx231xx-417.c
@@ -1249,8 +1249,7 @@ static void free_buffer(struct videobuf_queue *vq, struct cx231xx_buffer *buf)
 	struct cx231xx *dev = fh->dev;
 	unsigned long flags = 0;
 
-	if (in_interrupt())
-		BUG();
+	BUG_ON(in_interrupt());
 
 	spin_lock_irqsave(&dev->video_mode.slock, flags);
 	if (dev->USE_ISO) {
diff --git a/drivers/media/usb/cx231xx/cx231xx-vbi.c b/drivers/media/usb/cx231xx/cx231xx-vbi.c
index 80261ac40208..a08014d20a5c 100644
--- a/drivers/media/usb/cx231xx/cx231xx-vbi.c
+++ b/drivers/media/usb/cx231xx/cx231xx-vbi.c
@@ -192,8 +192,7 @@ static void free_buffer(struct videobuf_queue *vq, struct cx231xx_buffer *buf)
 	struct cx231xx_fh *fh = vq->priv_data;
 	struct cx231xx *dev = fh->dev;
 	unsigned long flags = 0;
-	if (in_interrupt())
-		BUG();
+	BUG_ON(in_interrupt());
 
 	/* We used to wait for the buffer to finish here, but this didn't work
 	   because, as we were keeping the state as VIDEOBUF_QUEUED,
diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c
index af44f2d1c0a1..c6ff8968286a 100644
--- a/drivers/media/usb/cx231xx/cx231xx-video.c
+++ b/drivers/media/usb/cx231xx/cx231xx-video.c
@@ -749,8 +749,7 @@ static void free_buffer(struct videobuf_queue *vq, struct cx231xx_buffer *buf)
 	struct cx231xx *dev = fh->dev;
 	unsigned long flags = 0;
 
-	if (in_interrupt())
-		BUG();
+	BUG_ON(in_interrupt());
 
 	/* We used to wait for the buffer to finish here, but this didn't work
 	   because, as we were keeping the state as VIDEOBUF_QUEUED,
diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
index 77ce9efe1f24..26b6ae8d04da 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -714,8 +714,7 @@ static void free_buffer(struct videobuf_queue *vq, struct tm6000_buffer *buf)
 	struct tm6000_core   *dev = fh->dev;
 	unsigned long flags;
 
-	if (in_interrupt())
-		BUG();
+	BUG_ON(in_interrupt());
 
 	/* We used to wait for the buffer to finish here, but this didn't work
 	   because, as we were keeping the state as VIDEOBUF_QUEUED,
diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c
index ca850316d379..7433ba5c4bad 100644
--- a/drivers/media/usb/zr364xx/zr364xx.c
+++ b/drivers/media/usb/zr364xx/zr364xx.c
@@ -377,8 +377,7 @@ static void free_buffer(struct videobuf_queue *vq, struct zr364xx_buffer *buf)
 {
 	_DBG("%s\n", __func__);
 
-	if (in_interrupt())
-		BUG();
+	BUG_ON(in_interrupt());
 
 	videobuf_vmalloc_free(&buf->vb);
 	buf->vb.state = VIDEOBUF_NEEDS_INIT;
-- 
2.1.0

[PATCH 2/2] drivers: Simplify the return code

[Mauro Carvalho Chehab]

If the last thing we do in a function is to call another
function and then return its value, we don't need to store
the returned code into some ancillary var.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/dvb-frontends/lgs8gxx.c b/drivers/media/dvb-frontends/lgs8gxx.c
index 3c92f36ea5c7..9b0166cdc7c2 100644
--- a/drivers/media/dvb-frontends/lgs8gxx.c
+++ b/drivers/media/dvb-frontends/lgs8gxx.c
@@ -544,11 +544,7 @@ static int lgs8gxx_set_mpeg_mode(struct lgs8gxx_state *priv,
 	t |= clk_pol ? TS_CLK_INVERTED : TS_CLK_NORMAL;
 	t |= clk_gated ? TS_CLK_GATED : TS_CLK_FREERUN;
 
-	ret = lgs8gxx_write_reg(priv, reg_addr, t);
-	if (ret != 0)
-		return ret;
-
-	return 0;
+	return lgs8gxx_write_reg(priv, reg_addr, t);
 }
 
 /* A/D input peak-to-peak voltage range */
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index a493c0b0b5fe..1fb8ac93970f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -1310,11 +1310,7 @@ static int adv7180_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	ret = adv7180_set_power(state, state->powered);
-	if (ret)
-		return ret;
-
-	return 0;
+	return adv7180_set_power(state, state->powered);
 }
 
 static SIMPLE_DEV_PM_OPS(adv7180_pm_ops, adv7180_suspend, adv7180_resume);
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index 3632958f2158..7a0263db4d1a 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -2103,7 +2103,6 @@ verify_window_lock(struct bttv_fh *fh, struct v4l2_window *win,
 {
 	enum v4l2_field field;
 	unsigned int width_mask;
-	int rc;
 
 	if (win->w.width < 48)
 		win->w.width = 48;
@@ -2156,13 +2155,10 @@ verify_window_lock(struct bttv_fh *fh, struct v4l2_window *win,
 	win->w.width -= win->w.left & ~width_mask;
 	win->w.left = (win->w.left - width_mask - 1) & width_mask;
 
-	rc = limit_scaled_size_lock(fh, &win->w.width, &win->w.height,
+	return limit_scaled_size_lock(fh, &win->w.width, &win->w.height,
 			       field, width_mask,
 			       /* width_bias: round down */ 0,
 			       adjust_size, adjust_crop);
-	if (0 != rc)
-		return rc;
-	return 0;
 }
 
 static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
diff --git a/drivers/media/pci/ngene/ngene-cards.c b/drivers/media/pci/ngene/ngene-cards.c
index 039bed3cc919..59dade6c156b 100644
--- a/drivers/media/pci/ngene/ngene-cards.c
+++ b/drivers/media/pci/ngene/ngene-cards.c
@@ -522,15 +522,11 @@ static int eeprom_read_ushort(struct i2c_adapter *adapter, u16 tag, u16 *data)
 
 static int eeprom_write_ushort(struct i2c_adapter *adapter, u16 tag, u16 data)
 {
-	int stat;
 	u8 buf[2];
 
 	buf[0] = data >> 8;
 	buf[1] = data & 0xff;
-	stat = WriteEEProm(adapter, tag, 2, buf);
-	if (stat)
-		return stat;
-	return 0;
+	return WriteEEProm(adapter, tag, 2, buf);
 }
 
 static s16 osc_deviation(void *priv, s16 deviation, int flag)
diff --git a/drivers/media/pci/saa7134/saa7134-alsa.c b/drivers/media/pci/saa7134/saa7134-alsa.c
index 1d2c310ce838..f78938d13950 100644
--- a/drivers/media/pci/saa7134/saa7134-alsa.c
+++ b/drivers/media/pci/saa7134/saa7134-alsa.c
@@ -354,15 +354,10 @@ static int saa7134_alsa_dma_free(struct saa7134_dmasound *dma)
 
 static int dsp_buffer_init(struct saa7134_dev *dev)
 {
-	int err;
-
 	BUG_ON(!dev->dmasound.bufsize);
 
-	err = saa7134_alsa_dma_init(dev,
+	return saa7134_alsa_dma_init(dev,
 			       (dev->dmasound.bufsize + PAGE_SIZE) >> PAGE_SHIFT);
-	if (0 != err)
-		return err;
-	return 0;
 }
 
 /*
diff --git a/drivers/media/tuners/fc0011.c b/drivers/media/tuners/fc0011.c
index 3932aa81e18c..d70a18d8e6c1 100644
--- a/drivers/media/tuners/fc0011.c
+++ b/drivers/media/tuners/fc0011.c
@@ -152,11 +152,7 @@ static int fc0011_vcocal_trigger(struct fc0011_priv *priv)
 	err = fc0011_writereg(priv, FC11_REG_VCOCAL, FC11_VCOCAL_RESET);
 	if (err)
 		return err;
-	err = fc0011_writereg(priv, FC11_REG_VCOCAL, FC11_VCOCAL_RUN);
-	if (err)
-		return err;
-
-	return 0;
+	return fc0011_writereg(priv, FC11_REG_VCOCAL, FC11_VCOCAL_RUN);
 }
 
 /* Read VCO calibration value */
@@ -168,11 +164,7 @@ static int fc0011_vcocal_read(struct fc0011_priv *priv, u8 *value)
 	if (err)
 		return err;
 	usleep_range(10000, 20000);
-	err = fc0011_readreg(priv, FC11_REG_VCOCAL, value);
-	if (err)
-		return err;
-
-	return 0;
+	return fc0011_readreg(priv, FC11_REG_VCOCAL, value);
 }
 
 static int fc0011_set_params(struct dvb_frontend *fe)
diff --git a/drivers/media/usb/dvb-usb-v2/anysee.c b/drivers/media/usb/dvb-usb-v2/anysee.c
index ae917c042a52..2ab3342d6b6c 100644
--- a/drivers/media/usb/dvb-usb-v2/anysee.c
+++ b/drivers/media/usb/dvb-usb-v2/anysee.c
@@ -1184,14 +1184,9 @@ static int anysee_ci_write_attribute_mem(struct dvb_ca_en50221 *ci, int slot,
 	int addr, u8 val)
 {
 	struct dvb_usb_device *d = ci->data;
-	int ret;
 	u8 buf[] = {CMD_CI, 0x03, 0x40 | addr >> 8, addr & 0xff, 0x00, 1, val};
 
-	ret = anysee_ctrl_msg(d, buf, sizeof(buf), NULL, 0);
-	if (ret)
-		return ret;
-
-	return 0;
+	return anysee_ctrl_msg(d, buf, sizeof(buf), NULL, 0);
 }
 
 static int anysee_ci_read_cam_control(struct dvb_ca_en50221 *ci, int slot,
@@ -1213,14 +1208,9 @@ static int anysee_ci_write_cam_control(struct dvb_ca_en50221 *ci, int slot,
 	u8 addr, u8 val)
 {
 	struct dvb_usb_device *d = ci->data;
-	int ret;
 	u8 buf[] = {CMD_CI, 0x05, 0x40, addr, 0x00, 1, val};
 
-	ret = anysee_ctrl_msg(d, buf, sizeof(buf), NULL, 0);
-	if (ret)
-		return ret;
-
-	return 0;
+	return anysee_ctrl_msg(d, buf, sizeof(buf), NULL, 0);
 }
 
 static int anysee_ci_slot_reset(struct dvb_ca_en50221 *ci, int slot)
@@ -1237,11 +1227,7 @@ static int anysee_ci_slot_reset(struct dvb_ca_en50221 *ci, int slot)
 
 	msleep(300);
 
-	ret = anysee_wr_reg_mask(d, REG_IOA, (1 << 7), 0x80);
-	if (ret)
-		return ret;
-
-	return 0;
+	return anysee_wr_reg_mask(d, REG_IOA, (1 << 7), 0x80);
 }
 
 static int anysee_ci_slot_shutdown(struct dvb_ca_en50221 *ci, int slot)
@@ -1255,23 +1241,14 @@ static int anysee_ci_slot_shutdown(struct dvb_ca_en50221 *ci, int slot)
 
 	msleep(30);
 
-	ret = anysee_wr_reg_mask(d, REG_IOA, (1 << 7), 0x80);
-	if (ret)
-		return ret;
-
-	return 0;
+	return anysee_wr_reg_mask(d, REG_IOA, (1 << 7), 0x80);
 }
 
 static int anysee_ci_slot_ts_enable(struct dvb_ca_en50221 *ci, int slot)
 {
 	struct dvb_usb_device *d = ci->data;
-	int ret;
 
-	ret = anysee_wr_reg_mask(d, REG_IOD, (0 << 1), 0x02);
-	if (ret)
-		return ret;
-
-	return 0;
+	return anysee_wr_reg_mask(d, REG_IOD, (0 << 1), 0x02);
 }
 
 static int anysee_ci_poll_slot_status(struct dvb_ca_en50221 *ci, int slot,
diff --git a/drivers/media/usb/dvb-usb/dtv5100.c b/drivers/media/usb/dvb-usb/dtv5100.c
index 3d11df41cac0..1ddb26369a5d 100644
--- a/drivers/media/usb/dvb-usb/dtv5100.c
+++ b/drivers/media/usb/dvb-usb/dtv5100.c
@@ -158,12 +158,8 @@ static int dtv5100_probe(struct usb_interface *intf,
 			return ret;
 	}
 
-	ret = dvb_usb_device_init(intf, &dtv5100_properties,
+	return dvb_usb_device_init(intf, &dtv5100_properties,
 				  THIS_MODULE, NULL, adapter_nr);
-	if (ret)
-		return ret;
-
-	return 0;
 }
 
 static struct usb_device_id dtv5100_table[] = {
diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 08fb0f2da64d..420cd41ac509 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -241,11 +241,7 @@ static int usbtv_setup_capture(struct usbtv *usbtv)
 	if (ret)
 		return ret;
 
-	ret = usbtv_select_input(usbtv, usbtv->input);
-	if (ret)
-		return ret;
-
-	return 0;
+	return usbtv_select_input(usbtv, usbtv->input);
 }
 
 /* Copy data from chunk into a frame buffer, deinterlacing the data
diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
index f669cedca8bd..994df1938d64 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -562,11 +562,7 @@ static int __videobuf_iolock(struct videobuf_queue *q,
 	default:
 		BUG();
 	}
-	err = videobuf_dma_map(q->dev, &mem->dma);
-	if (0 != err)
-		return err;
-
-	return 0;
+	return videobuf_dma_map(q->dev, &mem->dma);
 }
 
 static int __videobuf_sync(struct videobuf_queue *q,
-- 
2.1.0

Re: [PATCH 2/2] drivers: Simplify the return code

[Federico Simoncelli]

----- Original Message -----
> From: "Mauro Carvalho Chehab" <mchehab@osg.samsung.com>
> To: "Linux Media Mailing List" <linux-media@vger.kernel.org>
> Cc: "Mauro Carvalho Chehab" <mchehab@osg.samsung.com>, "Mauro Carvalho Chehab" <mchehab@infradead.org>, "Lars-Peter
> Clausen" <lars@metafoo.de>, "Michael Buesch" <m@bues.ch>, "Antti Palosaari" <crope@iki.fi>, "Hans Verkuil"
> <hverkuil@xs4all.nl>, "Sakari Ailus" <sakari.ailus@linux.intel.com>, "Ondrej Zary" <linux@rainbow-software.org>,
> "Ramakrishnan Muthukrishnan" <ramakrmu@cisco.com>, "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>, "Takashi
> Iwai" <tiwai@suse.de>, "Amber Thrall" <amber.rose.thrall@gmail.com>, "Federico Simoncelli" <fsimonce@redhat.com>,
> "James Harper" <james.harper@ejbdigital.com.au>, "Dan Carpenter" <dan.carpenter@oracle.com>, "Konrad Rzeszutek Wilk"
> <konrad.wilk@oracle.com>
> Sent: Tuesday, May 19, 2015 1:00:57 PM
> Subject: [PATCH 2/2] drivers: Simplify the return code
> 
> If the last thing we do in a function is to call another
> function and then return its value, we don't need to store
> the returned code into some ancillary var.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> diff --git a/drivers/media/dvb-frontends/lgs8gxx.c
> b/drivers/media/dvb-frontends/lgs8gxx.c
> index 3c92f36ea5c7..9b0166cdc7c2 100644
> --- a/drivers/media/dvb-frontends/lgs8gxx.c
> +++ b/drivers/media/dvb-frontends/lgs8gxx.c
> @@ -544,11 +544,7 @@ static int lgs8gxx_set_mpeg_mode(struct lgs8gxx_state
> *priv,
>  	t |= clk_pol ? TS_CLK_INVERTED : TS_CLK_NORMAL;
>  	t |= clk_gated ? TS_CLK_GATED : TS_CLK_FREERUN;
>  
> -	ret = lgs8gxx_write_reg(priv, reg_addr, t);
> -	if (ret != 0)
> -		return ret;
> -
> -	return 0;
> +	return lgs8gxx_write_reg(priv, reg_addr, t);
>  }

Personally I prefer the current style because it's more consistent with all
the other calls in the same function (return ret when ret != 0).

It also allows you to easily add/remove calls without having to deal with
the last special case return my_last_fun_call(...).

Anyway it's not a big deal, I think it's your call.
-- 
Federico

Re: [PATCH 2/2] drivers: Simplify the return code

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

On Tue, 19 May 2015 08:05:56 -0400 (EDT)
Federico Simoncelli <fsimonce@redhat.com> wrote:
> > diff --git a/drivers/media/dvb-frontends/lgs8gxx.c
> > b/drivers/media/dvb-frontends/lgs8gxx.c
> > index 3c92f36ea5c7..9b0166cdc7c2 100644
> > --- a/drivers/media/dvb-frontends/lgs8gxx.c
> > +++ b/drivers/media/dvb-frontends/lgs8gxx.c
> > @@ -544,11 +544,7 @@ static int lgs8gxx_set_mpeg_mode(struct lgs8gxx_state
> > *priv,
> >  	t |= clk_pol ? TS_CLK_INVERTED : TS_CLK_NORMAL;
> >  	t |= clk_gated ? TS_CLK_GATED : TS_CLK_FREERUN;
> >  
> > -	ret = lgs8gxx_write_reg(priv, reg_addr, t);
> > -	if (ret != 0)
> > -		return ret;
> > -
> > -	return 0;
> > +	return lgs8gxx_write_reg(priv, reg_addr, t);
> >  }
> 
> Personally I prefer the current style because it's more consistent with all
> the other calls in the same function (return ret when ret != 0).
> 
> It also allows you to easily add/remove calls without having to deal with
> the last special case return my_last_fun_call(...).
> 
> Anyway it's not a big deal, I think it's your call.


I agree. I also prefer the current style for these reasons. The compiler will also generate the same code in both cases.
I don't think it really simplifies the code.
But if you really insist on doing this change, go for it. You get my ack for fc0011

-- 
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] drivers: Simplify the return code

[Antti Palosaari]

On 05/19/2015 03:17 PM, Michael Büsch wrote:
> On Tue, 19 May 2015 08:05:56 -0400 (EDT)
> Federico Simoncelli <fsimonce@redhat.com> wrote:
>>> diff --git a/drivers/media/dvb-frontends/lgs8gxx.c
>>> b/drivers/media/dvb-frontends/lgs8gxx.c
>>> index 3c92f36ea5c7..9b0166cdc7c2 100644
>>> --- a/drivers/media/dvb-frontends/lgs8gxx.c
>>> +++ b/drivers/media/dvb-frontends/lgs8gxx.c
>>> @@ -544,11 +544,7 @@ static int lgs8gxx_set_mpeg_mode(struct lgs8gxx_state
>>> *priv,
>>>   	t |= clk_pol ? TS_CLK_INVERTED : TS_CLK_NORMAL;
>>>   	t |= clk_gated ? TS_CLK_GATED : TS_CLK_FREERUN;
>>>
>>> -	ret = lgs8gxx_write_reg(priv, reg_addr, t);
>>> -	if (ret != 0)
>>> -		return ret;
>>> -
>>> -	return 0;
>>> +	return lgs8gxx_write_reg(priv, reg_addr, t);
>>>   }
>>
>> Personally I prefer the current style because it's more consistent with all
>> the other calls in the same function (return ret when ret != 0).
>>
>> It also allows you to easily add/remove calls without having to deal with
>> the last special case return my_last_fun_call(...).
>>
>> Anyway it's not a big deal, I think it's your call.
>
>
> I agree. I also prefer the current style for these reasons. The compiler will also generate the same code in both cases.
> I don't think it really simplifies the code.
> But if you really insist on doing this change, go for it. You get my ack for fc0011


I am also against that kind of simplifications. Even it reduces line or 
two, it makes code more inconsistent, which means you have to make extra 
thinking when reading that code. I prefer similar repeating patterns as 
much as possible.

This is how I do it usually, even there is that extra last goto.

	ret = write_reg();
	if (ret)
		goto err;

	ret = write_reg();
	if (ret)
		goto err;
err:
	return ret;
};

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 2/2] drivers: Simplify the return code

[Dan Carpenter]

On Tue, May 19, 2015 at 07:00:50PM +0300, Antti Palosaari wrote:
> I am also against that kind of simplifications. Even it reduces line
> or two, it makes code more inconsistent, which means you have to
> make extra thinking when reading that code. I prefer similar
> repeating patterns as much as possible.
> 
> This is how I do it usually, even there is that extra last goto.
> 
> 	ret = write_reg();
> 	if (ret)
> 		goto err;
> 
> 	ret = write_reg();
> 	if (ret)
> 		goto err;
> err:
> 	return ret;
> };
> 

I don't care too much about the original patch one way or the other.
The new code is more fashionable and fewer lines.

But these sorts of do-nothing returns are a blight.

They are misleading.  You expect goto err to do something.  You wander
what it is.  The name tells you nothing.  So you have to scroll down.
Oh crap, it's just a @#$@$@#$ waste of time do-nothing goto.  It's the
travel through a door problem, you have completely forgotten what you
are doing.

http://www.scientificamerican.com/article/why-walking-through-doorway-makes-you-forget/


And also they are a total waste of time if you care about preventing
bugs.

Some people complain about "hidden return statements" but that is only
an issue if you don't have syntax highlighting.  If you look through the
git logs it is full of places like 95f38411df055a0e ('netns: use a
spin_lock to protect nsid management') where the other coder had gotos
highlighted in the same color as regular code.  If you actually measure
how common return with lock held bugs are the goto err and the direct
return style code have equal amount of bugs.  (I have looked at this but
only briefly, so it would be interesting to see a thourough scientific
paper on it).

Also the goto err style code introduces a new class of "forgot to set
the error code" bugs which are not there in direct return code.

regards,
dan carpenter

Re: [PATCH 2/2] drivers: Simplify the return code

[Mauro Carvalho Chehab]

Em Tue, 19 May 2015 19:00:50 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 05/19/2015 03:17 PM, Michael Büsch wrote:
> > On Tue, 19 May 2015 08:05:56 -0400 (EDT)
> > Federico Simoncelli <fsimonce@redhat.com> wrote:
> >>> diff --git a/drivers/media/dvb-frontends/lgs8gxx.c
> >>> b/drivers/media/dvb-frontends/lgs8gxx.c
> >>> index 3c92f36ea5c7..9b0166cdc7c2 100644
> >>> --- a/drivers/media/dvb-frontends/lgs8gxx.c
> >>> +++ b/drivers/media/dvb-frontends/lgs8gxx.c
> >>> @@ -544,11 +544,7 @@ static int lgs8gxx_set_mpeg_mode(struct lgs8gxx_state
> >>> *priv,
> >>>   	t |= clk_pol ? TS_CLK_INVERTED : TS_CLK_NORMAL;
> >>>   	t |= clk_gated ? TS_CLK_GATED : TS_CLK_FREERUN;
> >>>
> >>> -	ret = lgs8gxx_write_reg(priv, reg_addr, t);
> >>> -	if (ret != 0)
> >>> -		return ret;
> >>> -
> >>> -	return 0;
> >>> +	return lgs8gxx_write_reg(priv, reg_addr, t);
> >>>   }
> >>
> >> Personally I prefer the current style because it's more consistent with all
> >> the other calls in the same function (return ret when ret != 0).
> >>
> >> It also allows you to easily add/remove calls without having to deal with
> >> the last special case return my_last_fun_call(...).
> >>
> >> Anyway it's not a big deal, I think it's your call.
> >
> >
> > I agree. I also prefer the current style for these reasons. The compiler will also generate the same code in both cases.
> > I don't think it really simplifies the code.
> > But if you really insist on doing this change, go for it. You get my ack for fc0011
> 
> 
> I am also against that kind of simplifications. Even it reduces line or 
> two, it makes code more inconsistent, which means you have to make extra 
> thinking when reading that code.

Actually, it simplifies the thinking: less lines to read and the function
return code is clearly defined.

> I prefer similar repeating patterns as 
> much as possible.
> 
> This is how I do it usually, even there is that extra last goto.
> 
> 	ret = write_reg();
> 	if (ret)
> 		goto err;
> 
> 	ret = write_reg();
> 	if (ret)
> 		goto err;
> err:
> 	return ret;
> };

Nah, the above sucks: it is just hiding the return if error. Having to
go until the end of a function to see what "err" would do is not good.
Ok, if you have to deallocate things, do mutex unlock, etc, it is
justifiable.

However, in this case, it is quite a deception to discover that,
after going all the way down the code, "err" is just 
do-nothing-but-return.

The above code is exactly why several academic professors forbid the
usage of goto: the code can easily become hard to read if you use lots
of goto, instead of using structured loops and returns.

Regards,
Mauro

Re: [PATCH 2/2] drivers: Simplify the return code

[Mauro Carvalho Chehab]

Em Tue, 19 May 2015 08:05:56 -0400 (EDT)
Federico Simoncelli <fsimonce@redhat.com> escreveu:

> ----- Original Message -----
> > From: "Mauro Carvalho Chehab" <mchehab@osg.samsung.com>
> > To: "Linux Media Mailing List" <linux-media@vger.kernel.org>
> > Cc: "Mauro Carvalho Chehab" <mchehab@osg.samsung.com>, "Mauro Carvalho Chehab" <mchehab@infradead.org>, "Lars-Peter
> > Clausen" <lars@metafoo.de>, "Michael Buesch" <m@bues.ch>, "Antti Palosaari" <crope@iki.fi>, "Hans Verkuil"
> > <hverkuil@xs4all.nl>, "Sakari Ailus" <sakari.ailus@linux.intel.com>, "Ondrej Zary" <linux@rainbow-software.org>,
> > "Ramakrishnan Muthukrishnan" <ramakrmu@cisco.com>, "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>, "Takashi
> > Iwai" <tiwai@suse.de>, "Amber Thrall" <amber.rose.thrall@gmail.com>, "Federico Simoncelli" <fsimonce@redhat.com>,
> > "James Harper" <james.harper@ejbdigital.com.au>, "Dan Carpenter" <dan.carpenter@oracle.com>, "Konrad Rzeszutek Wilk"
> > <konrad.wilk@oracle.com>
> > Sent: Tuesday, May 19, 2015 1:00:57 PM
> > Subject: [PATCH 2/2] drivers: Simplify the return code
> > 
> > If the last thing we do in a function is to call another
> > function and then return its value, we don't need to store
> > the returned code into some ancillary var.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > 
> > diff --git a/drivers/media/dvb-frontends/lgs8gxx.c
> > b/drivers/media/dvb-frontends/lgs8gxx.c
> > index 3c92f36ea5c7..9b0166cdc7c2 100644
> > --- a/drivers/media/dvb-frontends/lgs8gxx.c
> > +++ b/drivers/media/dvb-frontends/lgs8gxx.c
> > @@ -544,11 +544,7 @@ static int lgs8gxx_set_mpeg_mode(struct lgs8gxx_state
> > *priv,
> >  	t |= clk_pol ? TS_CLK_INVERTED : TS_CLK_NORMAL;
> >  	t |= clk_gated ? TS_CLK_GATED : TS_CLK_FREERUN;
> >  
> > -	ret = lgs8gxx_write_reg(priv, reg_addr, t);
> > -	if (ret != 0)
> > -		return ret;
> > -
> > -	return 0;
> > +	return lgs8gxx_write_reg(priv, reg_addr, t);
> >  }
> 
> Personally I prefer the current style because it's more consistent with all
> the other calls in the same function (return ret when ret != 0).
> 
> It also allows you to easily add/remove calls without having to deal with
> the last special case return my_last_fun_call(...).
> 
> Anyway it's not a big deal, I think it's your call.

First of all, I wrote this patch to test coccinelle (with was broken
in Fedora 21 due to a dependency issue[1]). Once done, why not submit?
So, it is not that I'm really wanting to do that.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1218987

Yet, as someone that review a lot of code, the above is a way better,
as I can instantly check that the data returned by lgs8gxx_write_reg()
will also be returned to the caller function. Ok, this optimizes a few
milliseconds of my brain's processing, but it still means a faster
review to me, with is a gain, as I spend big periods of time reviewing
code. So, the simpler, the merrier.

Also, I don't trust too much on compiler optimization. I mean: I don't
expect that gcc will always do the right thing and remove the useless
if. Writing software for so long time, I know that software has bugs.
So, if I see something that can be easily optimized like the above
code, I do. It is one less thing to trust that the compiler will do
right.

Regards,
Mauro

Re: [PATCH 1/2] usb drivers: use BUG_ON() instead of if () BUG

[Lad, Prabhakar]

On Tue, May 19, 2015 at 12:00 PM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> Some USB drivers have a logic at the VB buffer handling like:
>         if (in_interrupt())
>                 BUG();
> Use, instead:
>         BUG_ON(in_interrupt());
>
> Btw, this logic looks weird on my eyes. We should convert them
> to use VB2, in order to avoid those crappy things.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
--Prabhakar Lad