Skip to contentSkip to conversation

Go7007: Adjustments for some function implementations

PATCH In review SF Markus Elfring 2 participants 11 messages Sep 18, 2017 – Sep 20, 2017

Subsystems (3)
Media (V4L/DVB) Kernel janitors GO7007 mpeg codec
lore
Share & tools
lore

SF Markus Elfring

author
#0 lore raw

cover Go7007: Adjustments for some function implementations

From: Markus Elfring <[email protected]>
Date: Mon, 18 Sep 2017 14:54:32 +0200

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

Markus Elfring (6):
Delete an error message for a failed memory allocation in go7007_load_encoder() Adjust 35 checks for null pointers
Improve a size determination in four functions
Use common error handling code in s2250_probe()
Use common error handling code in go7007_snd_init()
Delete an unnecessary variable initialisation in go7007_snd_init()

 drivers/media/usb/go7007/go7007-driver.c | 13 ++++---
 drivers/media/usb/go7007/go7007-fw.c     |  8 ++---
 drivers/media/usb/go7007/go7007-loader.c |  4 +--
 drivers/media/usb/go7007/go7007-usb.c    | 20 +++++------
 drivers/media/usb/go7007/go7007-v4l2.c   |  6 ++--
 drivers/media/usb/go7007/s2250-board.c   | 59 +++++++++++++++-----------------
 drivers/media/usb/go7007/snd-go7007.c    | 45 ++++++++++++------------
 7 files changed, 75 insertions(+), 80 deletions(-)
Signature · 1 line
2.14.1
author
#1 lore raw

PATCH 1/6 go7007: Delete an error message for a failed memory allocation in go7007_load_encoder()

From: Markus Elfring <[email protected]>
Date: Mon, 18 Sep 2017 10:52:42 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/media/usb/go7007/go7007-driver.c | 1 -
 1 file changed, 1 deletion(-)
drivers/media/usb/go7007/go7007-driver.c +0 -1
index 05b1126f263e..222332189fa4 100644
@@ -107,4 +107,3 @@ static int go7007_load_encoder(struct go7007 *go)
-			v4l2_err(go, "unable to allocate %d bytes for firmware transfer\n", fw_len);
 			release_firmware(fw_entry);
 			return -1;
 		}
Signature · 1 line
2.14.1
author
#2 lore raw

PATCH 2/6 go7007: Adjust 35 checks for null pointers

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Mon, 18 Sep 2017 11:13:27 +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 <[email protected]>
---
 drivers/media/usb/go7007/go7007-driver.c | 10 +++++-----
 drivers/media/usb/go7007/go7007-fw.c     |  8 ++++----
 drivers/media/usb/go7007/go7007-loader.c |  4 ++--
 drivers/media/usb/go7007/go7007-usb.c    | 18 +++++++++---------
 drivers/media/usb/go7007/go7007-v4l2.c   |  6 +++---
 drivers/media/usb/go7007/s2250-board.c   | 20 +++++++++-----------
 drivers/media/usb/go7007/snd-go7007.c    |  6 +++---
 7 files changed, 35 insertions(+), 37 deletions(-)
drivers/media/usb/go7007/go7007-driver.c +3 -3
index 222332189fa4..390f66ec8fd2 100644
@@ -91,6 +91,6 @@ static int go7007_load_encoder(struct go7007 *go)
 	int fw_len, rv = 0;
 	u16 intr_val, intr_data;
 
-	if (go->boot_fw = NULL) {
+	if (!go->boot_fw) {
 		if (request_firmware(&fw_entry, fw_name, go->dev)) {
 			v4l2_err(go, "unable to load firmware from file \"%s\"\n", fw_name);

@@ -103,5 +103,5 @@ static int go7007_load_encoder(struct go7007 *go)
 		}
 		fw_len = fw_entry->size - 16;
 		bounce = kmemdup(fw_entry->data + 16, fw_len, GFP_KERNEL);
-		if (bounce = NULL) {
+		if (!bounce) {
 			release_firmware(fw_entry);

@@ -448,7 +448,7 @@ static struct go7007_buffer *frame_boundary(struct go7007 *go, struct go7007_buf
 	u32 *bytesused;
 	struct go7007_buffer *vb_tmp = NULL;
 
-	if (vb = NULL) {
+	if (!vb) {
 		spin_lock(&go->spinlock);
 		if (!list_empty(&go->vidq_active))
 			vb = go->active_buf @@ -598,7 +598,7 @@ void go7007_parse_video_stream(struct go7007 *go, u8 *buf, int length)

 			    (buf[i] = seq_start_code ||
 			     buf[i] = gop_start_code ||

buf[i] = frame_start_code)) {
- if (vb = NULL || go->seen_frame)
+ if (!vb || go->seen_frame)
vb = frame_boundary(go, vb);
go->seen_frame = buf[i] = frame_start_code; if (vb && go->seen_frame)
@@ -703,4 +703,4 @@ struct go7007 *go7007_alloc(const struct go7007_board_info *board,
- if (go = NULL)
+ if (!go)
return NULL;
go->dev = dev;
go->board_info = board;

drivers/media/usb/go7007/go7007-fw.c +4 -4
index 60bf5f0644d1..a70a3fba79fb 100644
@@ -378,7 +378,7 @@ static int gen_mjpeghdr_to_package(struct go7007 *go, __le16 *code, int space)
 	int size = 0, i, off = 0, chunk;
 
 	buf = kzalloc(4096, GFP_KERNEL);
-	if (buf = NULL)
+	if (!buf)
 		return -ENOMEM;
 
 	for (i = 1; i < 32; ++i) {

@@ -645,7 +645,7 @@ static int gen_mpeg1hdr_to_package(struct go7007 *go,
 	int i, off = 0, chunk;
 
 	buf = kzalloc(5120, GFP_KERNEL);
-	if (buf = NULL)
+	if (!buf)
 		return -ENOMEM;
 
 	framelen[0] = mpeg1_frame_header(go, buf, 0, 1, PFRAME);

@@ -831,7 +831,7 @@ static int gen_mpeg4hdr_to_package(struct go7007 *go,
 	int i, off = 0, chunk;
 
 	buf = kzalloc(5120, GFP_KERNEL);
-	if (buf = NULL)
+	if (!buf)
 		return -ENOMEM;
 
 	framelen[0] = mpeg4_frame_header(go, buf, 0, PFRAME);

@@ -1577,7 +1577,7 @@ int go7007_construct_fw_image(struct go7007 *go, u8 **fw, int *fwlen)
 		return -1;
 	}
 	code = kzalloc(codespace * 2, GFP_KERNEL);
-	if (code = NULL)
+	if (!code)
 		goto fw_failed;
 
 	src = (__le16 *)fw_entry->data;
drivers/media/usb/go7007/go7007-loader.c +2 -2
index 042f78a31283..5e94f03c044d 100644
@@ -67,7 +67,7 @@ static int go7007_loader_probe(struct usb_interface *interface,
 			break;
 
 	/* Should never happen */
-	if (fw_configs[i].fw_name1 = NULL)
+	if (!fw_configs[i].fw_name1)
 		goto failed2;
 
 	fw1 = fw_configs[i].fw_name1;

@@ -87,7 +87,7 @@ static int go7007_loader_probe(struct usb_interface *interface,
 		goto failed2;
 	}
 
-	if (fw2 = NULL)
+	if (!fw2)
 		return 0;
 
 	if (request_firmware(&fw, fw2, &usbdev->dev)) {
drivers/media/usb/go7007/go7007-usb.c +9 -9
index ed9bcaf08d5e..5ad40b77763d 100644
@@ -829,7 +829,7 @@ static void go7007_usb_read_audio_pipe_complete(struct urb *urb)
 		dev_err(go->dev, "short read in audio pipe!\n");
 		return;
 	}
-	if (go->audio_deliver != NULL)
+	if (go->audio_deliver)
 		go->audio_deliver(go, urb->transfer_buffer, urb->actual_length);
 	r = usb_submit_urb(urb, GFP_ATOMIC);
 	if (r < 0)

@@ -1121,7 +1121,7 @@ static int go7007_usb_probe(struct usb_interface *intf,
 	go = go7007_alloc(&board->main_info, &intf->dev);
-	if (go = NULL)
+	if (!go)
 		return -ENOMEM;
 
 	usb = kzalloc(sizeof(struct go7007_usb), GFP_KERNEL);
-	if (usb = NULL) {
+	if (!usb) {
 		kfree(go);

@@ -1143,8 +1143,8 @@ static int go7007_usb_probe(struct usb_interface *intf,
 	usb->intr_urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (usb->intr_urb = NULL)
+	if (!usb->intr_urb)
 		goto allocfail;
 	usb->intr_urb->transfer_buffer = kmalloc(2*sizeof(u16), GFP_KERNEL);
-	if (usb->intr_urb->transfer_buffer = NULL)
+	if (!usb->intr_urb->transfer_buffer)
 		goto allocfail;
 
 	if (go->board_id = GO7007_BOARDID_SENSORAY_2250)

@@ -1278,9 +1278,9 @@ static int go7007_usb_probe(struct usb_interface *intf,
 		usb->video_urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
-		if (usb->video_urbs[i] = NULL)
+		if (!usb->video_urbs[i])
 			goto allocfail;
 		usb->video_urbs[i]->transfer_buffer  						kmalloc(v_urb_len, GFP_KERNEL);
-		if (usb->video_urbs[i]->transfer_buffer = NULL)
+		if (!usb->video_urbs[i]->transfer_buffer)
 			goto allocfail;
 		usb_fill_bulk_urb(usb->video_urbs[i], usb->usbdev, video_pipe,
 				usb->video_urbs[i]->transfer_buffer, v_urb_len,

@@ -1294,9 +1294,9 @@ static int go7007_usb_probe(struct usb_interface *intf,
 			usb->audio_urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
-			if (usb->audio_urbs[i] = NULL)
+			if (!usb->audio_urbs[i])
 				goto allocfail;
 			usb->audio_urbs[i]->transfer_buffer = kmalloc(4096,
 								GFP_KERNEL);
-			if (usb->audio_urbs[i]->transfer_buffer = NULL)
+			if (!usb->audio_urbs[i]->transfer_buffer)
 				goto allocfail;
 			usb_fill_bulk_urb(usb->audio_urbs[i], usb->usbdev,
 				usb_rcvbulkpipe(usb->usbdev, 8),
drivers/media/usb/go7007/go7007-v4l2.c +3 -3
index 98cd57eaf36a..f915fb5a316a 100644
@@ -187,9 +187,9 @@ static int set_capture_size(struct go7007 *go, struct v4l2_format *fmt, int try)
 	int width, height;
 
-	if (fmt != NULL && !valid_pixelformat(fmt->fmt.pix.pixelformat))
+	if (fmt && !valid_pixelformat(fmt->fmt.pix.pixelformat))
 		return -EINVAL;
 
 	get_resolution(go, &sensor_width, &sensor_height);
 
-	if (fmt = NULL) {
+	if (!fmt) {
 		width = sensor_width;

@@ -225,7 +225,7 @@ static int set_capture_size(struct go7007 *go, struct v4l2_format *fmt, int try)
 		height &= ~0xf;
 	}
 
-	if (fmt != NULL) {
+	if (fmt) {
 		u32 pixelformat = fmt->fmt.pix.pixelformat;
 
 		memset(fmt, 0, sizeof(*fmt));
drivers/media/usb/go7007/s2250-board.c +9 -11
index 1466db150d82..d987c5f2b45a 100644
@@ -165,13 +165,13 @@ static int write_reg(struct i2c_client *client, u8 reg, u8 value)
 	u8 *buf;
 
-	if (go = NULL)
+	if (!go)
 		return -ENODEV;
 
 	if (go->status = STATUS_SHUTDOWN)
 		return -EBUSY;
 
 	buf = kzalloc(16, GFP_KERNEL);
-	if (buf = NULL)
+	if (!buf)
 		return -ENOMEM;
 
 	usb = go->hpi_context;

@@ -199,12 +199,11 @@ static int write_reg_fp(struct i2c_client *client, u16 addr, u16 val)
 	struct s2250 *dec = i2c_get_clientdata(client);
 
-	if (go = NULL)
+	if (!go)
 		return -ENODEV;
 
 	if (go->status = STATUS_SHUTDOWN)
 		return -EBUSY;
 
 	buf = kzalloc(16, GFP_KERNEL);
-
-	if (buf = NULL)
+	if (!buf)
 		return -ENOMEM;

@@ -261,13 +260,12 @@ static int read_reg_fp(struct i2c_client *client, u16 addr, u16 *val)
 	int rc;
 	u8 *buf;
 
-	if (go = NULL)
+	if (!go)
 		return -ENODEV;
 
 	if (go->status = STATUS_SHUTDOWN)
 		return -EBUSY;
 
 	buf = kzalloc(16, GFP_KERNEL);
-
-	if (buf = NULL)
+	if (!buf)
 		return -ENOMEM;

@@ -514,9 +512,9 @@ static int s2250_probe(struct i2c_client *client,
 	struct go7007_usb *usb = go->hpi_context;
 
 	audio = i2c_new_dummy(adapter, TLV320_ADDRESS >> 1);
-	if (audio = NULL)
+	if (!audio)
 		return -ENOMEM;
 
 	state = kzalloc(sizeof(struct s2250), GFP_KERNEL);
-	if (state = NULL) {
+	if (!state) {
 		i2c_unregister_device(audio);

@@ -581,4 +579,4 @@ static int s2250_probe(struct i2c_client *client,
 	if (mutex_lock_interruptible(&usb->i2c_lock) = 0) {
 		data = kzalloc(16, GFP_KERNEL);
-		if (data != NULL) {
+		if (data) {
 			int rc = go7007_usb_vendor_request(go, 0x41, 0, 0,
drivers/media/usb/go7007/snd-go7007.c +3 -3
index c618764480c6..4e612cf1afd9 100644
@@ -114,6 +114,6 @@ static int go7007_snd_hw_params(struct snd_pcm_substream *substream,
 		vfree(substream->runtime->dma_area);
 	substream->runtime->dma_bytes = 0;
 	substream->runtime->dma_area = vmalloc(bytes);
-	if (substream->runtime->dma_area = NULL)
+	if (!substream->runtime->dma_area)
 		return -ENOMEM;
 	substream->runtime->dma_bytes = bytes;

@@ -140,6 +140,6 @@ static int go7007_snd_capture_open(struct snd_pcm_substream *substream)
 	int r;
 
 	spin_lock_irqsave(&gosnd->lock, flags);
-	if (gosnd->substream = NULL) {
+	if (!gosnd->substream) {
 		gosnd->substream = substream;
 		substream->runtime->hw = go7007_snd_capture_hw;

@@ -239,4 +239,4 @@ int go7007_snd_init(struct go7007 *go)
-	if (gosnd = NULL)
+	if (!gosnd)
 		return -ENOMEM;
 	spin_lock_init(&gosnd->lock);
 	gosnd->hw_ptr = gosnd->w_idx = gosnd->avail = 0;
Signature · 1 line
2.14.1
author
#3 lore raw

PATCH 3/6 go7007: Improve a size determination in four functions

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Mon, 18 Sep 2017 11:27:30 +0200

Replace the specification of data structures by pointer dereferences as the parameter for the operator "sizeof" to make the corresponding size determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/media/usb/go7007/go7007-driver.c | 2 +-
 drivers/media/usb/go7007/go7007-usb.c    | 2 +-
 drivers/media/usb/go7007/s2250-board.c   | 2 +-
 drivers/media/usb/go7007/snd-go7007.c    | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
drivers/media/usb/go7007/go7007-driver.c +1 -1
index 390f66ec8fd2..f8c06b2f9d2f 100644
@@ -699,5 +699,5 @@ struct go7007 *go7007_alloc(const struct go7007_board_info *board,
 	struct go7007 *go;
 	int i;
 
-	go = kzalloc(sizeof(struct go7007), GFP_KERNEL);
+	go = kzalloc(sizeof(*go), GFP_KERNEL);
 	if (!go)
drivers/media/usb/go7007/go7007-usb.c +1 -1
index 5ad40b77763d..f0f70a92541a 100644
@@ -1122,5 +1122,5 @@ static int go7007_usb_probe(struct usb_interface *intf,
 	if (!go)
 		return -ENOMEM;
 
-	usb = kzalloc(sizeof(struct go7007_usb), GFP_KERNEL);
+	usb = kzalloc(sizeof(*usb), GFP_KERNEL);
 	if (!usb) {
drivers/media/usb/go7007/s2250-board.c +1 -1
index d987c5f2b45a..1fd4c09dd516 100644
@@ -515,5 +515,5 @@ static int s2250_probe(struct i2c_client *client,
 	if (!audio)
 		return -ENOMEM;
 
-	state = kzalloc(sizeof(struct s2250), GFP_KERNEL);
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state) {
drivers/media/usb/go7007/snd-go7007.c +1 -1
index 4e612cf1afd9..68e421bf38e1 100644
@@ -235,5 +235,5 @@ int go7007_snd_init(struct go7007 *go)
 		dev++;
 		return -ENOENT;
 	}
-	gosnd = kmalloc(sizeof(struct go7007_snd), GFP_KERNEL);
+	gosnd = kmalloc(sizeof(*gosnd), GFP_KERNEL);
 	if (!gosnd)
Signature · 1 line
2.14.1
author
#4 lore raw

PATCH 4/6 go7007: Use common error handling code in s2250_probe()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Mon, 18 Sep 2017 13:50:45 +0200

Adjust jump targets so that a bit of exception handling can be better reused at the end of this function.

This refactoring might fix also an error situation where the function "i2c_unregister_device" was not called after a software failure was noticed by the data member "hdl.error".

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/media/usb/go7007/s2250-board.c | 37 +++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 19 deletions(-)
drivers/media/usb/go7007/s2250-board.c +18 -19
index 1fd4c09dd516..1bd9a7f2e7a3 100644
@@ -510,6 +510,7 @@ static int s2250_probe(struct i2c_client *client,
 	u8 *data;
 	struct go7007 *go = i2c_get_adapdata(adapter);
 	struct go7007_usb *usb = go->hpi_context;
+	int code;
 
 	audio = i2c_new_dummy(adapter, TLV320_ADDRESS >> 1);
 	if (!audio)

@@ -517,8 +518,8 @@ static int s2250_probe(struct i2c_client *client,
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state) {
-		i2c_unregister_device(audio);
-		return -ENOMEM;
+		code = -ENOMEM;
+		goto unregister_device;
 	}
 
 	sd = &state->sd;

@@ -538,11 +539,8 @@ static int s2250_probe(struct i2c_client *client,
 		V4L2_CID_HUE, -512, 511, 1, 0);
 	sd->ctrl_handler = &state->hdl;
 	if (state->hdl.error) {
-		int err = state->hdl.error;
-
-		v4l2_ctrl_handler_free(&state->hdl);
-		kfree(state);
-		return err;
+		code = state->hdl.error;
+		goto free_handler;
 	}
 
 	state->std = V4L2_STD_NTSC;

@@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client,
 	/* initialize the audio */
 	if (write_regs(audio, aud_regs) < 0) {
 		dev_err(&client->dev, "error initializing audio\n");
-		goto fail;
+		goto e_io;
 	}
 
-	if (write_regs(client, vid_regs) < 0) {
-		dev_err(&client->dev, "error initializing decoder\n");
-		goto fail;
-	}
-	if (write_regs_fp(client, vid_regs_fp) < 0) {
-		dev_err(&client->dev, "error initializing decoder\n");
-		goto fail;
-	}
+	if (write_regs(client, vid_regs) < 0 ||
+	    write_regs_fp(client, vid_regs_fp) < 0)
+		goto report_write_failure;
+
 	/* set default channel */
 	/* composite */
 	write_reg_fp(client, 0x20, 0x020 | 1);

@@ -602,11 +596,16 @@ static int s2250_probe(struct i2c_client *client,
 	v4l2_info(sd, "initialized successfully\n");
 	return 0;
 
-fail:
-	i2c_unregister_device(audio);
+report_write_failure:
+	dev_err(&client->dev, "error initializing decoder\n");
+e_io:
+	code = -EIO;
+free_handler:
 	v4l2_ctrl_handler_free(&state->hdl);
 	kfree(state);
-	return -EIO;
+unregister_device:
+	i2c_unregister_device(audio);
+	return code;
 }
 
 static int s2250_remove(struct i2c_client *client)
Signature · 1 line
2.14.1
#5 lore raw

Re: [PATCH 4/6] [media] go7007: Use common error handling code in s2250_probe()

Quoted 20 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Mon, 18 Sep 2017 13:50:45 +0200

Adjust jump targets so that a bit of exception handling can be better reused at the end of this function.

This refactoring might fix also an error situation where the function "i2c_unregister_device" was not called after a software failure was noticed by the data member "hdl.error".

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/media/usb/go7007/s2250-board.c | 37 +++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 19 deletions(-)
drivers/media/usb/go7007/s2250-board.c +0 -0
index 1fd4c09dd516..1bd9a7f2e7a3 100644

drivers/media/usb/go7007/s2250-board.c static int s2250_probe(struct i2c_client *client, · line 513

Quoted 4 lines
drivers/media/usb/go7007/s2250-board.c +1 -0
@@ -510,6 +510,7 @@ static int s2250_probe(struct i2c_client *client,
 	u8 *data;
 	struct go7007 *go = i2c_get_adapdata(adapter);
 	struct go7007_usb *usb = go->hpi_context;
+	int code;

It should be called "int rc" to match the rest of the driver. "ret" or "err" would also have been acceptable.

Quoted 29 lines
drivers/media/usb/go7007/s2250-board.c +4 -7
 	audio = i2c_new_dummy(adapter, TLV320_ADDRESS >> 1);
 	if (!audio)

@@ -517,8 +518,8 @@ static int s2250_probe(struct i2c_client *client,
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state) {
-		i2c_unregister_device(audio);
-		return -ENOMEM;
+		code = -ENOMEM;
+		goto unregister_device;
 	}
 
 	sd = &state->sd;

@@ -538,11 +539,8 @@ static int s2250_probe(struct i2c_client *client,
 		V4L2_CID_HUE, -512, 511, 1, 0);
 	sd->ctrl_handler = &state->hdl;
 	if (state->hdl.error) {
-		int err = state->hdl.error;
-
-		v4l2_ctrl_handler_free(&state->hdl);
-		kfree(state);
-		return err;
+		code = state->hdl.error;
+		goto free_handler;
 	}
 
 	state->std = V4L2_STD_NTSC;

drivers/media/usb/go7007/s2250-board.c static int s2250_probe(struct i2c_client *client, · line 556

Quoted 5 lines
drivers/media/usb/go7007/s2250-board.c +1 -1
@@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client,
 	/* initialize the audio */
 	if (write_regs(audio, aud_regs) < 0) {
 		dev_err(&client->dev, "error initializing audio\n");
-		goto fail;
+		goto e_io;

Preserve the error code.

regards,
dan carpenter

author
#6 lore raw

Re: [PATCH 4/6] [media] go7007: Use common error handling code in s2250_probe()

Quoted 8 lines

drivers/media/usb/go7007/s2250-board.c static int s2250_probe(struct i2c_client *client, · line 556

Quoted 6 lines
drivers/media/usb/go7007/s2250-board.c +1 -1
@@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client,
 	/* initialize the audio */
 	if (write_regs(audio, aud_regs) < 0) {
 		dev_err(&client->dev, "error initializing audio\n");
-		goto fail;
+		goto e_io;

Preserve the error code.

Do you suggest then to adjust the implementation of the function "write_regs" so that a more meaningful value would be used instead of the failure indication "-1"?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/go7007/s2250-board.c?h=v4.14-rc1#n302
http://elixir.free-electrons.com/linux/v4.14-rc1/source/drivers/media/usb/go7007/s2250-board.c#L298

Regards,
Markus

#7 lore raw

Re: [PATCH 4/6] [media] go7007: Use common error handling code in s2250_probe()

Quoted 9 lines · SF Markus Elfring
Quoted 8 lines

drivers/media/usb/go7007/s2250-board.c static int s2250_probe(struct i2c_client *client, · line 556

Quoted 6 lines
drivers/media/usb/go7007/s2250-board.c +1 -1
@@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client,
 	/* initialize the audio */
 	if (write_regs(audio, aud_regs) < 0) {
 		dev_err(&client->dev, "error initializing audio\n");
-		goto fail;
+		goto e_io;

Preserve the error code.

Quoted 3 lines

Do you suggest then to adjust the implementation of the function "write_regs" so that a more meaningful value would be used instead of the failure indication "-1"?

If you want to, yeah, that would be good.

regards,
dan carpenter

author
#8 lore raw

PATCH 5/6 go7007: Use common error handling code in go7007_snd_init()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Mon, 18 Sep 2017 14:28:59 +0200

Add jump targets so that a bit of exception handling can be better reused at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/media/usb/go7007/snd-go7007.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)
drivers/media/usb/go7007/snd-go7007.c +17 -18
index 68e421bf38e1..7ae4d03ed3f7 100644
@@ -243,22 +243,18 @@ int go7007_snd_init(struct go7007 *go)
 	gosnd->capturing = 0;
 	ret = snd_card_new(go->dev, index[dev], id[dev], THIS_MODULE, 0,
 			   &gosnd->card);
-	if (ret < 0) {
-		kfree(gosnd);
-		return ret;
-	}
+	if (ret < 0)
+		goto free_snd;
+
 	ret = snd_device_new(gosnd->card, SNDRV_DEV_LOWLEVEL, go,
 			&go7007_snd_device_ops);
-	if (ret < 0) {
-		kfree(gosnd);
-		return ret;
-	}
+	if (ret < 0)
+		goto free_snd;
+
 	ret = snd_pcm_new(gosnd->card, "go7007", 0, 0, 1, &gosnd->pcm);
-	if (ret < 0) {
-		snd_card_free(gosnd->card);
-		kfree(gosnd);
-		return ret;
-	}
+	if (ret < 0)
+		goto free_card;
+
 	strlcpy(gosnd->card->driver, "go7007", sizeof(gosnd->card->driver));
 	strlcpy(gosnd->card->shortname, go->name, sizeof(gosnd->card->driver));
 	strlcpy(gosnd->card->longname, gosnd->card->shortname,

@@ -269,11 +265,8 @@ int go7007_snd_init(struct go7007 *go)
 			&go7007_snd_capture_ops);
 
 	ret = snd_card_register(gosnd->card);
-	if (ret < 0) {
-		snd_card_free(gosnd->card);
-		kfree(gosnd);
-		return ret;
-	}
+	if (ret < 0)
+		goto free_card;
 
 	gosnd->substream = NULL;
 	go->snd_context = gosnd;

@@ -281,6 +274,12 @@ int go7007_snd_init(struct go7007 *go)
 	++dev;
 
 	return 0;
+
+free_card:
+	snd_card_free(gosnd->card);
+free_snd:
+	kfree(gosnd);
+	return ret;
 }
 EXPORT_SYMBOL(go7007_snd_init);
Signature · 1 line
2.14.1
#9 lore raw

Re: [PATCH 5/6] [media] go7007: Use common error handling code in go7007_snd_init()

Quoted 14 lines · SF Markus Elfring
drivers/media/usb/go7007/snd-go7007.c +2 -4
index 68e421bf38e1..7ae4d03ed3f7 100644
@@ -243,22 +243,18 @@ int go7007_snd_init(struct go7007 *go)
 	gosnd->capturing = 0;
 	ret = snd_card_new(go->dev, index[dev], id[dev], THIS_MODULE, 0,
 			   &gosnd->card);
-	if (ret < 0) {
-		kfree(gosnd);
-		return ret;
-	}
+	if (ret < 0)
+		goto free_snd;

drivers/media/usb/go7007/snd-go7007.c int go7007_snd_init(struct go7007 *go) · line 253

Quoted 10 lines
drivers/media/usb/go7007/snd-go7007.c +4 -4
+
 	ret = snd_device_new(gosnd->card, SNDRV_DEV_LOWLEVEL, go,
 			&go7007_snd_device_ops);
-	if (ret < 0) {
-		kfree(gosnd);
-		return ret;
-	}
+	if (ret < 0)
+		goto free_snd;
+

I think the original code is buggy. It should probably call snd_card_free() if snd_device_new() fails.

regards,
dan carpenter

author
#10 lore raw

PATCH 6/6 go7007: Delete an unnecessary variable initialisation in go7007_snd_init()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Mon, 18 Sep 2017 14:35:43 +0200

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

Signed-off-by Markus Elfring <[email protected]>
---
 drivers/media/usb/go7007/snd-go7007.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/usb/go7007/snd-go7007.c +1 -1
index 7ae4d03ed3f7..27c09fd44657 100644
@@ -227,7 +227,7 @@ int go7007_snd_init(struct go7007 *go)
 {
 	static int dev;
 	struct go7007_snd *gosnd;
-	int ret = 0;
+	int ret;
 
 	if (dev >= SNDRV_CARDS)
 		return -ENODEV;
Signature · 1 line
2.14.1