media: atmel-isc: avoid returning a random value at isc_parse_dt()

As warned by smatch:
drivers/media/platform/atmel/atmel-isc.c:2097 isc_parse_dt() error: uninitialized symbol 'ret'.

The problem here is that of_graph_get_next_endpoint() can potentially return NULL on its first pass, with would make it return a random value, as ret is not initialized.

While here, use while(1) instead of for(; ;), as while is the preferred syntax for such kind of loops.

Em Wed, 1 Nov 2017 16:56:33 -0400
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 9 lines

As warned by smatch:
drivers/media/platform/atmel/atmel-isc.c:2097 isc_parse_dt() error: uninitialized symbol 'ret'.

The problem here is that of_graph_get_next_endpoint() can potentially return NULL on its first pass, with would make it return a random value, as ret is not initialized.

While here, use while(1) instead of for(; ;), as while is the preferred syntax for such kind of loops.

Sorry, please discard this e-mail... there's something wrong on my environment.

git send-email is dying after the first e-mail:

Died at /usr/libexec/git-core/git-send-email line 1350.

Em Wed, 1 Nov 2017 16:56:33 -0400
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 9 lines

As warned by smatch:
drivers/media/platform/atmel/atmel-isc.c:2097 isc_parse_dt() error: uninitialized symbol 'ret'.

The problem here is that of_graph_get_next_endpoint() can potentially return NULL on its first pass, with would make it return a random value, as ret is not initialized.

While here, use while(1) instead of for(; ;), as while is the preferred syntax for such kind of loops.

Sorry, please discard this e-mail... there's something wrong on my environment.

git send-email is dying after the first e-mail:

Died at /usr/libexec/git-core/git-send-email line 1350.

I'll try to fix and re-send it.

As smatch warned:

	drivers/media/dvb-core/dvb_frontend.c:2468

dvb_frontend_handle_ioctl() error: uninitialized symbol 'err'.

The ioctl handler actually got a regression here: before changeset d73dcf0cdb95 ("media: dvb_frontend: cleanup ioctl handling logic"), the code used to return -EOPNOTSUPP if an ioctl handler was not implemented on a driver. After the change, it may return a random value.

Fixes d73dcf0cdb95 ("media: dvb_frontend: cleanup ioctl handling

logic") # For 4.14

Cc [email protected]

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/dvb-core/dvb_frontend.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/media/dvb-core/dvb_frontend.c index daaf969719e4..bcf0cbcbf7b2 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -2109,7 +2109,7 @@ static int dvb_frontend_handle_ioctl(struct file *file, struct dvb_frontend *fe = dvbdev->priv; struct dvb_frontend_private *fepriv = fe->frontend_priv; struct dtv_frontend_properties *c = &fe->dtv_property_cache;

drivers/media/dvb-core/dvb_frontend.c +2 -1

-	int i, err;
+	int i, err = -EOPNOTSUPP;
 
 	dev_dbg(fe->dvb->device, "%s:\n", __func__);

@@ -2144,6 +2144,7 @@ static int dvb_frontend_handle_ioctl(struct
 file *file, }
 		}
 		kfree(tvp);
+		err = 0;
 		break;
 	}
 	case FE_GET_PROPERTY: {

The logic at V4L2 led core assumes that the flash struct can be null. However, it doesn't check for null while trying to set, causing some smatch to warn:

drivers/media/v4l2-core/v4l2-flash-led-class.c:210 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 200)

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 include/linux/led-class-flash.h | 4 ++++
 1 file changed, 4 insertions(+)

include/linux/led-class-flash.h +4 -0

index e97966d1fb8d..700efaa9e115 100644
@@ -121,6 +121,8 @@ extern void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev);
 static inline int led_set_flash_strobe(struct led_classdev_flash *fled_cdev,
 					bool state)
 {
+	if (!fled_cdev)
+		return -EINVAL;
 	return fled_cdev->ops->strobe_set(fled_cdev, state);
 }
 

@@ -136,6 +138,8 @@ static inline int led_set_flash_strobe(struct led_classdev_flash *fled_cdev,
 static inline int led_get_flash_strobe(struct led_classdev_flash *fled_cdev,
 					bool *state)
 {
+	if (!fled_cdev)
+		return -EINVAL;
 	if (fled_cdev->ops->strobe_get)
 		return fled_cdev->ops->strobe_get(fled_cdev, state);
 

As warned by smatch:

	drivers/media/i2c/s5c73m3/s5c73m3-core.c:268 s5c73m3_check_status() error: uninitialized symbol 'status'.

if s5c73m3_check_status() is called too late, time_is_after_jiffies(end) will return 0, causing the while to abort before reading status.

The current code will do the wrong thing here, as it will still check if status != value. The right fix here is to just change the initial state of ret to -ETIMEDOUT. This way, if the routine is called too late, it will skip the flawed check and return that a timeout happened.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/media/i2c/s5c73m3/s5c73m3-core.c +1 -1

index cdc4f2392ef9..45345f8b27a5 100644
@@ -248,7 +248,7 @@ static int s5c73m3_check_status(struct s5c73m3 *state, unsigned int value)
 {
 	unsigned long start = jiffies;
 	unsigned long end = start + msecs_to_jiffies(2000);
-	int ret = 0;
+	int ret = -ETIMEDOUT;
 	u16 status;
 	int count = 0;
 

Smatch reports this warning:

	drivers/media/v4l2-core/v4l2-async.c:597 v4l2_async_register_subdev()

error: uninitialized symbol 'ret'.

However, there's nothing wrong there. So, just shut up the warning.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/v4l2-core/v4l2-async.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/media/v4l2-core/v4l2-async.c index 49f7eccc76db..5bc861c4ae5c 100644

drivers/media/v4l2-core/v4l2-async.c +1 -1

@@ -532,7 +532,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 {
 	struct v4l2_async_notifier *subdev_notifier;
 	struct v4l2_async_notifier *notifier;
-	int ret;
+	int uninitialized_var(ret);
 
 	/*
 	 * No reference taken. The reference is held by the device

Hi Mauro,

Thank you for the patch.

Quoted 4 lines · On Wednesday, 1 November 2017 23:05:45 EET Mauro Carvalho Chehab wrote:

Smatch reports this warning:

	drivers/media/v4l2-core/v4l2-async.c:597 v4l2_async_register_subdev()

error: uninitialized symbol 'ret'.

Quoted 2 lines

However, there's nothing wrong there. So, just shut up the warning.

Nothing wrong, really ? ret does seem to be used uninitialized when the function returns at the very last line.

Hi Mauro,

Thank you for the patch.

Quoted 4 lines · On Wednesday, 1 November 2017 23:05:45 EET Mauro Carvalho Chehab wrote:

Smatch reports this warning:

	drivers/media/v4l2-core/v4l2-async.c:597 v4l2_async_register_subdev()

error: uninitialized symbol 'ret'.

Quoted 2 lines

However, there's nothing wrong there. So, just shut up the warning.

Nothing wrong, really ? ret does seem to be used uninitialized when the function returns at the very last line.

Em Thu, 02 Nov 2017 04:51:40 +0200 Laurent Pinchart escreveu:

Quoted 11 lines · On Wednesday, 1 November 2017 23:05:45 EET Mauro Carvalho Chehab wrote:

Quoted 7 lines

Smatch reports this warning:

	drivers/media/v4l2-core/v4l2-async.c:597 v4l2_async_register_subdev()

error: uninitialized symbol 'ret'.

However, there's nothing wrong there. So, just shut up the warning.

Nothing wrong, really ? ret does seem to be used uninitialized when the function returns at the very last line.

There's nothing wrong. If you follow the logic, you'll see that the line:

	return ret;

is called only at "err_unbind" label, with is called only on two places:

                ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd);
                if (ret)
                        goto err_unbind;

                ret = v4l2_async_notifier_try_complete(notifier);
                if (ret)
                        goto err_unbind;

There, ret is defined.

Yeah, the logic there is confusing.

Hi Mauro,

Quoted 29 lines · On Monday, 11 December 2017 20:10:58 EET Mauro Carvalho Chehab wrote:

Em Thu, 02 Nov 2017 04:51:40 +0200 Laurent Pinchart escreveu:

Quoted 11 lines · On Wednesday, 1 November 2017 23:05:45 EET Mauro Carvalho Chehab wrote:

Quoted 7 lines

Smatch reports this warning:

	drivers/media/v4l2-core/v4l2-async.c:597 v4l2_async_register_subdev()

error: uninitialized symbol 'ret'.

However, there's nothing wrong there. So, just shut up the warning.

Nothing wrong, really ? ret does seem to be used uninitialized when the function returns at the very last line.

There's nothing wrong. If you follow the logic, you'll see that the line:

	return ret;

is called only at "err_unbind" label, with is called only on two places:

                ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd);
                if (ret)
                        goto err_unbind;

                ret = v4l2_async_notifier_try_complete(notifier);
                if (ret)
                        goto err_unbind;

Quoted 3 lines

There, ret is defined.

Yeah, the logic there is confusing.

I had missed the return 0 just before the error label. Sorry for the noise.

Two orthogonal changesets caused a breakage at a printk inside davinci. Changeset a2d17962c9ca ("[media] davinci: Switch from V4L2 OF to V4L2 fwnode") made davinci to use struct fwnode_handle instead of struct device_node. Changeset 68d9c47b1679 ("media: Convert to using %pOF instead of full_name") changed the printk to not use ->full_name, but, instead, to rely on %pOF.

With both patches applied, the Kernel will do the wrong
thing, as warned by smatch:
drivers/media/platform/davinci/vpif_capture.c:1399 vpif_async_bound() error: '%pOF' expects argument of type 'struct device_node*', argument 5 has type 'void*'

So, change the logic to actually print the device name that was obtained before the print logic.

Fixes 68d9c47b1679 ("media: Convert to using %pOF instead of full_name")

Fixes a2d17962c9ca ("[media] davinci: Switch from V4L2 OF to V4L2 fwnode")

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/platform/davinci/vpif_capture.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Two orthogonal changesets caused a breakage at a printk inside rcar. Changeset 859969b38e2e
("[media] v4l: Switch from V4L2 OF not V4L2 fwnode API") made davinci to use struct fwnode_handle instead of struct device_node. Changeset 68d9c47b1679 ("media: Convert to using %pOF instead of full_name") changed the printk to not use ->full_name, but, instead, to rely on %pOF.

With both patches applied, the Kernel will do the wrong
thing, as warned by smatch:
drivers/media/platform/rcar-vin/rcar-core.c:189 rvin_digital_graph_init() error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'

So, change the logic to actually print the device name that was obtained before the print logic.

Fixes 68d9c47b1679 ("media: Convert to using %pOF instead of full_name")

Fixes 859969b38e2e ("[media] v4l: Switch from V4L2 OF not V4L2 fwnode API")

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/platform/rcar-vin/rcar-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/media/platform/rcar-vin/rcar-core.c +0 -0

index 108d776f3265..ce5914f7a056 100644

drivers/media/platform/rcar-vin/rcar-core.c +0 -0

 	vin->notifier.ops = &rvin_digital_notify_ops;
 	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);

Signature · 1 line

2.13.6

Two orthogonal changesets caused a breakage at several printk messages inside xilinx. Changeset 859969b38e2e ("[media] v4l: Switch from V4L2 OF not V4L2 fwnode API") made davinci to use struct fwnode_handle instead of struct device_node. Changeset 68d9c47b1679
("media: Convert to using %pOF instead of full_name") changed the printk to not use ->full_name, but, instead, to rely on %pOF.

With both patches applied, the Kernel will do the wrong
thing, as warned by smatch:
drivers/media/platform/xilinx/xilinx-vipp.c:108 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:117 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:126 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:138 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 3 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:148 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:245 xvip_graph_build_dma()
error: '%pOF' expects argument of type 'struct device_node*', argument 3 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:254 xvip_graph_build_dma()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'

So, change the logic to actually print the device name that was obtained before the print logic.

Fixes 68d9c47b1679 ("media: Convert to using %pOF instead of full_name")

Fixes 859969b38e2e ("[media] v4l: Switch from V4L2 OF not V4L2 fwnode API")

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/platform/xilinx/xilinx-vipp.c | 31 +++++++++++++-------------
 1 file changed, 17 insertions(+), 14 deletions(-)

(quoted diff) +0 -0

b/drivers/media/platform/xilinx/xilinx-vipp.c index d881cf09876d..dd777c834c43 100644

drivers/media/platform/xilinx/xilinx-vipp.c +5 -2

@@ -65,6 +65,9 @@ xvip_graph_find_entity(struct xvip_composite_device *xdev,
 return NULL;
 }
 
+#define LOCAL_NAME(link)	to_of_node(link.local_node)->full_name
+#define REMOTE_NAME(link)	to_of_node(link.remote_node)->full_name
+
 static int xvip_graph_build_one(struct xvip_composite_device *xdev,
 				struct xvip_graph_entity *entity)
 {

@@ -103,9 +106,9 @@ static int xvip_graph_build_one(struct
 xvip_composite_device *xdev,
 		 * the link.
 		 */
 		if (link.local_port >= local->num_pads) {
-			dev_err(xdev->dev, "invalid port number %u for %pOF\n",
+			dev_err(xdev->dev, "invalid port number %u for %s\n",
 				link.local_port,
-				to_of_node(link.local_node));
+				LOCAL_NAME(link));
 			v4l2_fwnode_put_link(&link);
 			ret = -EINVAL;

drivers/media/platform/xilinx/xilinx-vipp.c +4 -4

 			break;

@@ -114,8 +117,8 @@ static int xvip_graph_build_one(struct
 xvip_composite_device *xdev, local_pad = &local->pads[link.local_port];
 
 		if (local_pad->flags & MEDIA_PAD_FL_SINK) {
-			dev_dbg(xdev->dev, "skipping sink port %pOF:%u\n",
-				to_of_node(link.local_node),
+			dev_dbg(xdev->dev, "skipping sink port %s:%u\n",
+				LOCAL_NAME(link),
 				link.local_port);
 			v4l2_fwnode_put_link(&link);
 			continue;

@@ -123,8 +126,8 @@ static int xvip_graph_build_one(struct
 xvip_composite_device *xdev,
 
 		/* Skip DMA engines, they will be processed separately. */
 		if (link.remote_node == of_fwnode_handle(xdev->dev->of_node)) {
-			dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
-				to_of_node(link.local_node),
+			dev_dbg(xdev->dev, "skipping DMA port %s:%u\n",
+				REMOTE_NAME(link),
 				link.local_port);
 			v4l2_fwnode_put_link(&link);

drivers/media/platform/xilinx/xilinx-vipp.c +8 -8

 			continue;

@@ -134,8 +137,8 @@ static int xvip_graph_build_one(struct
 xvip_composite_device *xdev, ent = xvip_graph_find_entity(xdev,
 					     to_of_node(link.remote_node));
 		if (ent == NULL) {
-			dev_err(xdev->dev, "no entity found for %pOF\n",
-				to_of_node(link.remote_node));
+			dev_err(xdev->dev, "no entity found for %s\n",
+				REMOTE_NAME(link));
 			v4l2_fwnode_put_link(&link);
 			ret = -ENODEV;
 			break;

@@ -144,8 +147,8 @@ static int xvip_graph_build_one(struct
 xvip_composite_device *xdev, remote = ent->entity;
 
 		if (link.remote_port >= remote->num_pads) {
-			dev_err(xdev->dev, "invalid port number %u on %pOF\n",
-				link.remote_port, to_of_node(link.remote_node));
+			dev_err(xdev->dev, "invalid port number %u on %s\n",
+				link.remote_port, REMOTE_NAME(link));
 			v4l2_fwnode_put_link(&link);
 			ret = -EINVAL;
 			break;

@@ -241,17 +244,17 @@ static int xvip_graph_build_dma(struct
 xvip_composite_device *xdev) ent = xvip_graph_find_entity(xdev,
 					     to_of_node(link.remote_node));
 		if (ent == NULL) {
-			dev_err(xdev->dev, "no entity found for %pOF\n",
-				to_of_node(link.remote_node));
+			dev_err(xdev->dev, "no entity found for %s\n",
+				REMOTE_NAME(link));
 			v4l2_fwnode_put_link(&link);
 			ret = -ENODEV;
 			break;
 		}
 
 		if (link.remote_port >= ent->entity->num_pads) {
-			dev_err(xdev->dev, "invalid port number %u on %pOF\n",
+			dev_err(xdev->dev, "invalid port number %u on %s\n",
 				link.remote_port,
-				to_of_node(link.remote_node));
+				REMOTE_NAME(link));
 			v4l2_fwnode_put_link(&link);
 			ret = -EINVAL;
 			break;

Hi Mauro,

(CC'ing Rob and Sakari)

Thank you for the patch.

Quoted 33 lines · On Wednesday, 1 November 2017 23:05:51 EET Mauro Carvalho Chehab wrote:

Two orthogonal changesets caused a breakage at several printk messages inside xilinx. Changeset 859969b38e2e ("[media] v4l: Switch from V4L2 OF not V4L2 fwnode API") made davinci to use struct fwnode_handle instead of struct device_node. Changeset 68d9c47b1679
("media: Convert to using %pOF instead of full_name") changed the printk to not use ->full_name, but, instead, to rely on %pOF.

With both patches applied, the Kernel will do the wrong
thing, as warned by smatch:
drivers/media/platform/xilinx/xilinx-vipp.c:108 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:117 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:126 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:138 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 3 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:148 xvip_graph_build_one()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:245 xvip_graph_build_dma()
error: '%pOF' expects argument of type 'struct device_node*', argument 3 has type 'void*'
drivers/media/platform/xilinx/xilinx-vipp.c:254 xvip_graph_build_dma()
error: '%pOF' expects argument of type 'struct device_node*', argument 4 has type 'void*'

Quoted 2 lines

So, change the logic to actually print the device name that was obtained before the print logic.

This doesn't seem like a good idea to me. The main point of commit 68d9c47b1679 is to avoid accessing full_name directly to prepare for removal of that field.

to_of_node() is defined as

#define to_of_node(__fwnode)                                            \
        ({                                                              \
                typeof(__fwnode) __to_of_node_fwnode = (__fwnode);      \
                                                                        \
                is_of_node(__to_of_node_fwnode) ?                       \
                        container_of(__to_of_node_fwnode,               \
                                     struct device_node, fwnode) :      \
                        NULL;                                           \
        })

when CONFIG_OF is set, and as

static inline struct device_node *to_of_node(const struct fwnode_handle *fwnode)

{
        return NULL;
}

otherwise. I wonder why smatch believes the value is a void *, but in any case that should be fixed either in smatch (if it's a smatch bug) or in the definition of the to_of_node() macro.

(Fixed Mauro's e-mail.)

to_of_node() macro checks whether the fwnode_handle passed to it is not an OF node, and if so, returns NULL in order to be NULL-safe. Otherwise it returns the pointer to the OF node which the fwnode_handle contains.

The problem with returning NULL is that its type was void *, which sometimes matters. Explicitly return struct device_node * instead.

Make a similar change to of_fwnode_handle() as well.

Fixes d20dc1493db4 ("of: Support const and non-const use for to_of_node()")

Fixes debd3a3b27c7 ("of: Make of_fwnode_handle() safer")

Signed-off-by Sakari Ailus <[email protected]>

---
Hi Mauro,

Could you check whether this addresses the smatch issue with the xilinx driver?

Thanks.

 include/linux/of.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

include/linux/of.h +3 -2

index b240ed69dc96..0651231c115e 100644
@@ -161,7 +161,7 @@ static inline bool is_of_node(const struct fwnode_handle
 *fwnode) is_of_node(__to_of_node_fwnode) ?			\
 			container_of(__to_of_node_fwnode,		\
 				     struct device_node, fwnode) :	\
-			NULL;						\
+			(struct device_node *)NULL;			\
 	})
 
 #define of_fwnode_handle(node)						\

@@ -169,7 +169,8 @@ static inline bool is_of_node(const struct fwnode_handle
 *fwnode) typeof(node) __of_fwnode_handle_node = (node);		\
 									\
 		__of_fwnode_handle_node ?				\
-			&__of_fwnode_handle_node->fwnode : NULL;	\
+			&__of_fwnode_handle_node->fwnode :		\
+			(struct fwnode_handle *)NULL;			\
 	})
 
 static inline bool of_have_populated_dt(void)

(Fixed Mauro's e-mail.)

to_of_node() macro checks whether the fwnode_handle passed to it is not an OF node, and if so, returns NULL in order to be NULL-safe. Otherwise it returns the pointer to the OF node which the fwnode_handle contains.

The problem with returning NULL is that its type was void *, which sometimes matters. Explicitly return struct device_node * instead.

Make a similar change to of_fwnode_handle() as well.

Fixes d20dc1493db4 ("of: Support const and non-const use for to_of_node()")

Fixes debd3a3b27c7 ("of: Make of_fwnode_handle() safer")

Signed-off-by Sakari Ailus <[email protected]>

---
Hi Mauro,

Could you check whether this addresses the smatch issue with the xilinx driver?

Thanks.

 include/linux/of.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

The smatch logic gets confused with the syntax used to check if the ov9650x_read() reads succedded:

	drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized symbol 'reg2'.
	drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized symbol 'reg1'.

There's nothing wrong with the original logic, except that it is a little more harder to review.

So, let's stick with the syntax that won't cause read issues.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/ov9650.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/media/i2c/ov9650.c +6 -4

index 69433e1e2533..e519f278d5f9 100644
@@ -886,10 +886,12 @@ static int __g_volatile_ctrl(struct ov965x *ov965x, struct v4l2_ctrl *ctrl)
 		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
 			return 0;
 		ret = ov965x_read(client, REG_COM1, &reg0);
-		if (!ret)
-			ret = ov965x_read(client, REG_AECH, &reg1);
-		if (!ret)
-			ret = ov965x_read(client, REG_AECHM, &reg2);
+		if (ret < 0)
+			return ret;
+		ret = ov965x_read(client, REG_AECH, &reg1);
+		if (ret < 0)
+			return ret;
+		ret = ov965x_read(client, REG_AECHM, &reg2);
 		if (ret < 0)
 			return ret;
 		exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |

Signature · 1 line

2.13.6

Quoted 7 lines · Mauro Carvalho Chehab

The smatch logic gets confused with the syntax used to check if the ov9650x_read() reads succedded:

	drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized symbol 'reg2'.
	drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized symbol 'reg1'.

There's nothing wrong with the original logic, except that it is a little more harder to review.

Maybe I do not understand the original logic correctly but ov965x_read(...) is passing on the return value from i2c_transfer() -> __i2c_transfer() and thus if one of those calls would have been a negativ error status it would have simply executed the next call to ov965x_read() and if that call would have suceeded it would eventually reach the line " exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |..." with the potential to operate on uninitialized registers reg0/1/2 the current code sems only to handle error conditions in the last call to ov965x_read() correctly.

I think this is actually not an equivalent transform but a bug-fix of case V4L2_CID_EXPOSURE_AUTO: (aside from being a code inconsistency) So should this not carry a

Fixes 84a15ded76ec ("[media] V4L: Add driver for OV9650/52 image sensors")

tag ?

thx!
hofrat

Quoted 5 lines

So, let's stick with the syntax that won't cause read issues.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Nicholas Mc Guire <[email protected]>

Quoted 28 lines

---
 drivers/media/i2c/ov9650.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/media/i2c/ov9650.c +6 -4

index 69433e1e2533..e519f278d5f9 100644
@@ -886,10 +886,12 @@ static int __g_volatile_ctrl(struct ov965x *ov965x, struct v4l2_ctrl *ctrl)
 		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
 			return 0;
 		ret = ov965x_read(client, REG_COM1, &reg0);
-		if (!ret)
-			ret = ov965x_read(client, REG_AECH, &reg1);
-		if (!ret)
-			ret = ov965x_read(client, REG_AECHM, &reg2);
+		if (ret < 0)
+			return ret;
+		ret = ov965x_read(client, REG_AECH, &reg1);
+		if (ret < 0)
+			return ret;
+		ret = ov965x_read(client, REG_AECHM, &reg2);
 		if (ret < 0)
 			return ret;
 		exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |

Signature · 1 line

2.13.6