[PATCH v2 01/26] media: atmel-isc: avoid returning a random value at isc_parse_dt()

[PATCH v2 01/26] media: atmel-isc: avoid returning a random value at isc_parse_dt()

[Mauro Carvalho Chehab]

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.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/platform/atmel/atmel-isc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 13f1c1c797b0..0c2635647f69 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -2039,10 +2039,10 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 
 	INIT_LIST_HEAD(&isc->subdev_entities);
 
-	for (; ;) {
+	while (1) {
 		epn = of_graph_get_next_endpoint(np, epn);
 		if (!epn)
-			break;
+			return 0;
 
 		rem = of_graph_get_remote_port_parent(epn);
 		if (!rem) {
-- 
2.13.6

Re: [PATCH v2 01/26] media: atmel-isc: avoid returning a random value at isc_parse_dt()

[Mauro Carvalho Chehab]

Em Wed,  1 Nov 2017 16:56:33 -0400
Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:

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

Thanks,
Mauro

Re: [PATCH v2 01/26] media: atmel-isc: avoid returning a random value at isc_parse_dt()

[Mauro Carvalho Chehab]

Em Wed, 1 Nov 2017 18:59:48 -0200
Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:

> Em Wed,  1 Nov 2017 16:56:33 -0400
> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:
> 
> > 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.

Btw, this never happened before today... probably it is because I'm trying
to send a pull request between Halloween and the Day of the Dead :-p

Thanks,
Mauro

Re: [PATCH v2 01/26] media: atmel-isc: avoid returning a random value at isc_parse_dt()

[Mauro Carvalho Chehab]

Em Wed, 1 Nov 2017 18:59:48 -0200
Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:

> Em Wed,  1 Nov 2017 16:56:33 -0400
> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:
> 
> > 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.

Found the issue... on patch 2 of this series, it was marked with:

Cc: stable@vger.kernel.org # for 4.14

It seems that my environment didn't like this...

Just resent the hole stuff


Thanks,
Mauro

[PATCH v2 02/26] media: dvb_frontend: be sure to init dvb_frontend_handle_ioctl() return code

[Mauro Carvalho Chehab]

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: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvb_frontend.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c 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;
-	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: {
@@ -2195,6 +2196,7 @@ static int dvb_frontend_handle_ioctl(struct file *file,
 			return -EFAULT;
 		}
 		kfree(tvp);
+		err = 0;
 		break;
 	}
 
-- 
2.13.6

Re: [PATCH v2 02/26] media: dvb_frontend: be sure to init dvb_frontend_handle_ioctl() return code

[Daniel Scheller]

Am Wed,  1 Nov 2017 17:05:39 -0400
schrieb Mauro Carvalho Chehab <mchehab@s-opensource.com>:

> 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: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c
> 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;
> -	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: {
> @@ -2195,6 +2196,7 @@ static int dvb_frontend_handle_ioctl(struct
> file *file, return -EFAULT;
>  		}
>  		kfree(tvp);
> +		err = 0;
>  		break;
>  	}
>  

Tested-by: Daniel Scheller <d.scheller@gmx.net>

Fixes false reporting of successful DVBv3 signal stats inquiry when
they're not available, causing wrong assumptions and values in userspace
apps.

Best regards,
Daniel Scheller
-- 
https://github.com/herrnst

[PATCH v2 03/26] media: led-class-flash: better handle NULL flash struct

[Mauro Carvalho Chehab]

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 <mchehab@s-opensource.com>
---
 include/linux/led-class-flash.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
index e97966d1fb8d..700efaa9e115 100644
--- a/include/linux/led-class-flash.h
+++ b/include/linux/led-class-flash.h
@@ -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);
 
-- 
2.13.6

Re: [PATCH v2 03/26] media: led-class-flash: better handle NULL flash struct

[Sakari Ailus]

Hi Mauro,

Somehow the To header in your message ends up being:

To: unlisted-recipients: no To-header on input <;

This doesn't end well when replying to the messages.

On Wed, Nov 01, 2017 at 05:05:40PM -0400, Mauro Carvalho Chehab wrote:
> 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)

I guess this is fine for suppressing the warning but there's not a
technical problem it fixes: if there's no flash, then the flash controls
aren't registered with the control handler.

Anyway,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  include/linux/led-class-flash.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
> index e97966d1fb8d..700efaa9e115 100644
> --- a/include/linux/led-class-flash.h
> +++ b/include/linux/led-class-flash.h
> @@ -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);
>  

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

[PATCH v2 04/26] media: tda8290: initialize agc gain

[Mauro Carvalho Chehab]

The tuning logic at tda8290 relies on agc_stat and
adc_sat to be initialized. However, as warned by smatch:

	drivers/media/tuners/tda8290.c:261 tda8290_set_params() error: uninitialized symbol 'agc_stat'.
	drivers/media/tuners/tda8290.c:261 tda8290_set_params() error: uninitialized symbol 'adc_sat'.
	drivers/media/tuners/tda8290.c:262 tda8290_set_params() error: uninitialized symbol 'adc_sat'.

That could cause an erratic behavior if PLL is not locked,
as the code will only work if
	!(pll_stat & 0x80) && (adc_sat < 20)

So, initialize it to zero, in order to let the code below
to be called, with should give more chances to adjust the
tuner gain, in order to get a PLL lock.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/tuners/tda8290.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/tuners/tda8290.c b/drivers/media/tuners/tda8290.c
index a59c567c55d6..f226e6ecc175 100644
--- a/drivers/media/tuners/tda8290.c
+++ b/drivers/media/tuners/tda8290.c
@@ -196,7 +196,7 @@ static void tda8290_set_params(struct dvb_frontend *fe,
 	unsigned char addr_adc_sat  = 0x1a;
 	unsigned char addr_agc_stat = 0x1d;
 	unsigned char addr_pll_stat = 0x1b;
-	unsigned char adc_sat, agc_stat,
+	unsigned char adc_sat = 0, agc_stat = 0,
 		      pll_stat;
 	int i;
 
-- 
2.13.6

[PATCH v2 05/26] media: s5c73m3-core: fix logic on a timeout condition

[Mauro Carvalho Chehab]

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 <mchehab@s-opensource.com>
---
 drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
index cdc4f2392ef9..45345f8b27a5 100644
--- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
+++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
@@ -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;
 
-- 
2.13.6

Re: [PATCH v2 05/26] media: s5c73m3-core: fix logic on a timeout condition

[Andrzej Hajda]

On 01.11.2017 22:05, Mauro Carvalho Chehab wrote:
> 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.

First of all it is rather uncommon situation, scenario in which two
consecutive lines of simple code takes more than 2 seconds is rather rare.
Of course it is possible but in such case proposed solution does not
look like as a proper fix, it reports timeout on the sensor without even
touching it.
I think the right fix would be to re-factor the loop to read the status
first and check timeout later.

Regards
Andrzej

>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> index cdc4f2392ef9..45345f8b27a5 100644
> --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> @@ -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;
>  

[PATCH v2 06/26] media: xc5000: better handle I2C error messages

[Mauro Carvalho Chehab]

As warned by smatch, there are several places where the I2C
transfer may fail, leading into inconsistent behavior:

	drivers/media/tuners/xc5000.c:689 xc_debug_dump() error: uninitialized symbol 'regval'.
	drivers/media/tuners/xc5000.c:841 xc5000_is_firmware_loaded() error: uninitialized symbol 'id'.
	drivers/media/tuners/xc5000.c:939 xc5000_set_tv_freq() error: uninitialized symbol 'pll_lock_status'.
	drivers/media/tuners/xc5000.c:1195 xc_load_fw_and_init_tuner() error: uninitialized symbol 'pll_lock_status'.

Handle the return codes from the I2C transfer, in order to
address those issues.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/tuners/xc5000.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index 0e7e4fdf9e50..98ba177dbc29 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -685,8 +685,8 @@ static void xc_debug_dump(struct xc5000_priv *priv)
 		(totalgain % 256) * 100 / 256);
 
 	if (priv->pll_register_no) {
-		xc5000_readreg(priv, priv->pll_register_no, &regval);
-		dprintk(1, "*** PLL lock status = 0x%04x\n", regval);
+		if (!xc5000_readreg(priv, priv->pll_register_no, &regval))
+			dprintk(1, "*** PLL lock status = 0x%04x\n", regval);
 	}
 }
 
@@ -831,15 +831,16 @@ static int xc5000_is_firmware_loaded(struct dvb_frontend *fe)
 	u16 id;
 
 	ret = xc5000_readreg(priv, XREG_PRODUCT_ID, &id);
-	if (ret == 0) {
+	if (!ret) {
 		if (id == XC_PRODUCT_ID_FW_NOT_LOADED)
 			ret = -ENOENT;
 		else
 			ret = 0;
+		dprintk(1, "%s() returns id = 0x%x\n", __func__, id);
+	} else {
+		dprintk(1, "%s() returns error %d\n", __func__, ret);
 	}
 
-	dprintk(1, "%s() returns %s id = 0x%x\n", __func__,
-		ret == 0 ? "True" : "False", id);
 	return ret;
 }
 
@@ -935,7 +936,10 @@ static int xc5000_set_tv_freq(struct dvb_frontend *fe)
 
 	if (priv->pll_register_no != 0) {
 		msleep(20);
-		xc5000_readreg(priv, priv->pll_register_no, &pll_lock_status);
+		ret = xc5000_readreg(priv, priv->pll_register_no,
+				     &pll_lock_status);
+		if (ret)
+			return ret;
 		if (pll_lock_status > 63) {
 			/* PLL is unlocked, force reload of the firmware */
 			dprintk(1, "xc5000: PLL not locked (0x%x).  Reloading...\n",
@@ -1190,8 +1194,10 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
 		}
 
 		if (priv->pll_register_no) {
-			xc5000_readreg(priv, priv->pll_register_no,
-				       &pll_lock_status);
+			ret = xc5000_readreg(priv, priv->pll_register_no,
+					     &pll_lock_status);
+			if (ret)
+				continue;
 			if (pll_lock_status > 63) {
 				/* PLL is unlocked, force reload of the firmware */
 				printk(KERN_ERR
-- 
2.13.6

[PATCH v2 07/26] media: radio-si476x: fix behavior when seek->range* are defined

[Mauro Carvalho Chehab]

The logic at si476x_radio_s_hw_freq_seek() checks if the
frequency range that will be used to handle hardware seek
has the minimal frequency under rangelow. That works fine
if userspace zeros both fields. However, if userspace
fills either seek->rangelow or seek-rangehigh, it won't
read the corresponding range from the device, causing the
values to be unitialized, as warned by smatch:

	drivers/media/radio/radio-si476x.c:789 si476x_radio_s_hw_freq_seek() error: uninitialized symbol 'rangelow'.
	drivers/media/radio/radio-si476x.c:789 si476x_radio_s_hw_freq_seek() error: uninitialized symbol 'rangehigh'.

Fix it by initializing those vars from the values present at
the struct v4l2_hw_freq_seek.

While here, simplify the logic which reads such values from
the hardware limits.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/radio/radio-si476x.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/media/radio/radio-si476x.c b/drivers/media/radio/radio-si476x.c
index 271f725b17e8..740714d9dd85 100644
--- a/drivers/media/radio/radio-si476x.c
+++ b/drivers/media/radio/radio-si476x.c
@@ -755,7 +755,7 @@ static int si476x_radio_s_hw_freq_seek(struct file *file, void *priv,
 {
 	int err;
 	enum si476x_func func;
-	u32 rangelow, rangehigh;
+	u32 rangelow = seek->rangelow, rangehigh = seek->rangehigh;
 	struct si476x_radio *radio = video_drvdata(file);
 
 	if (file->f_flags & O_NONBLOCK)
@@ -767,23 +767,21 @@ static int si476x_radio_s_hw_freq_seek(struct file *file, void *priv,
 
 	si476x_core_lock(radio->core);
 
-	if (!seek->rangelow) {
+	if (!rangelow) {
 		err = regmap_read(radio->core->regmap,
 				  SI476X_PROP_SEEK_BAND_BOTTOM,
 				  &rangelow);
-		if (!err)
-			rangelow = si476x_to_v4l2(radio->core, rangelow);
-		else
+		if (err)
 			goto unlock;
+		rangelow = si476x_to_v4l2(radio->core, rangelow);
 	}
-	if (!seek->rangehigh) {
+	if (!rangehigh) {
 		err = regmap_read(radio->core->regmap,
 				  SI476X_PROP_SEEK_BAND_TOP,
 				  &rangehigh);
-		if (!err)
-			rangehigh = si476x_to_v4l2(radio->core, rangehigh);
-		else
+		if (err)
 			goto unlock;
+		rangehigh = si476x_to_v4l2(radio->core, rangehigh);
 	}
 
 	if (rangelow > rangehigh) {
-- 
2.13.6

[PATCH v2 08/26] media: v4l2-async: shut up an unitialized symbol warning

[Mauro Carvalho Chehab]

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 <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 49f7eccc76db..5bc861c4ae5c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -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
-- 
2.13.6

Re: [PATCH v2 08/26] media: v4l2-async: shut up an unitialized symbol warning

[Laurent Pinchart]

Hi Mauro,

Thank you for the patch.

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

> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 49f7eccc76db..5bc861c4ae5c
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -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


-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/26] media: v4l2-async: shut up an unitialized symbol warning

[Sakari Ailus]

On Thu, Nov 02, 2017 at 04:51:40AM +0200, Laurent Pinchart wrote:
> Hi Mauro,
> 
> Thank you for the patch.
> 
> 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'.
> > 
> > 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 another ret defined in a block under this one; removing that is the
correct fix. I wonder why GCC didn't complain about that to begin with...
usually it does.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v2 08/26] media: v4l2-async: shut up an unitialized symbol warning

[Mauro Carvalho Chehab]

Em Thu, 02 Nov 2017 04:51:40 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> 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'.
> > 
> > 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.

Thanks,
Mauro

media: v4l2-async: shut up an unitialized symbol warning

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. Yet, the logic is more
complex than it should. So, rework it to make clearer about
what happens when ret != 0.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-async.c |   38 +++++++++++++++--------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

--- patchwork.orig/drivers/media/v4l2-core/v4l2-async.c
+++ patchwork/drivers/media/v4l2-core/v4l2-async.c
@@ -532,7 +532,7 @@ int v4l2_async_register_subdev(struct v4
 {
 	struct v4l2_async_notifier *subdev_notifier;
 	struct v4l2_async_notifier *notifier;
-	int ret;
+	int ret = 0;
 
 	/*
 	 * No reference taken. The reference is held by the device
@@ -560,11 +560,11 @@ int v4l2_async_register_subdev(struct v4
 
 		ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd);
 		if (ret)
-			goto err_unbind;
+			break;
 
 		ret = v4l2_async_notifier_try_complete(notifier);
 		if (ret)
-			goto err_unbind;
+			break;
 
 		goto out_unlock;
 	}
@@ -572,26 +572,22 @@ int v4l2_async_register_subdev(struct v4
 	/* None matched, wait for hot-plugging */
 	list_add(&sd->async_list, &subdev_list);
 
-out_unlock:
-	mutex_unlock(&list_lock);
-
-	return 0;
-
-err_unbind:
-	/*
-	 * Complete failed. Unbind the sub-devices bound through registering
-	 * this async sub-device.
-	 */
-	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
-	if (subdev_notifier)
-		v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
-
-	if (sd->asd)
-		v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
-	v4l2_async_cleanup(sd);
+	if (ret) {
+		/*
+		 * Complete failed. Unbind the sub-devices bound through registering
+		 * this async sub-device.
+		 */
+		subdev_notifier = v4l2_async_find_subdev_notifier(sd);
+		if (subdev_notifier)
+			v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
+
+		if (sd->asd)
+			v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
+		v4l2_async_cleanup(sd);
+	}
 
+out_unlock:
 	mutex_unlock(&list_lock);
-
 	return ret;
 }
 EXPORT_SYMBOL(v4l2_async_register_subdev);

Re: [PATCH v2 08/26] media: v4l2-async: shut up an unitialized symbol warning

[Laurent Pinchart]

Hi Mauro,

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:
> > 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'.
> >> 
> >> 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.

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/26] media: v4l2-async: shut up an unitialized symbol warning

[Sakari Ailus]

Hi folks,

On Tue, Dec 12, 2017 at 12:13:59AM +0200, Laurent Pinchart wrote:
> Hi Mauro,
> 
> 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:
> > > 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'.
> > >> 
> > >> 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.
> 
> I had missed the return 0 just before the error label. Sorry for the noise.

I believe the matter has been addressed by this patch:

commit 580db6ca62c168733c6fd338cd2f0ebf39135283
Author: Colin Ian King <colin.king@canonical.com>
Date:   Fri Nov 3 02:58:27 2017 -0400

    media: v4l: async: fix return of unitialized variable ret
    
    A shadow declaration of variable ret is being assigned a return error
    status and this value is being lost when the error exit goto's jump
    out of the local scope. This leads to an uninitalized error return value
    in the outer scope being returned. Fix this by removing the inner scoped
    declaration of variable ret.
    
    Detected by CoverityScan, CID#1460380 ("Uninitialized scalar variable")
    
    Fixes: fb45f436b818 ("media: v4l: async: Fix notifier complete callback error handling")
    
    Signed-off-by: Colin Ian King <colin.king@canonical.com>
    Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
    Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

[PATCH v2 09/26] media: cx25821-alsa: fix usage of a pointer printk

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/pci/cx25821/cx25821-alsa.c:155 cx25821_alsa_dma_init() warn: argument 3 to %08lx specifier is cast from pointer

Use the standard %p to print a pointer.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/cx25821/cx25821-alsa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/cx25821/cx25821-alsa.c b/drivers/media/pci/cx25821/cx25821-alsa.c
index 2b34990e86f2..a45bf0331eeb 100644
--- a/drivers/media/pci/cx25821/cx25821-alsa.c
+++ b/drivers/media/pci/cx25821/cx25821-alsa.c
@@ -152,8 +152,8 @@ static int cx25821_alsa_dma_init(struct cx25821_audio_dev *chip, int nr_pages)
 		return -ENOMEM;
 	}
 
-	dprintk(1, "vmalloc is at addr 0x%08lx, size=%d\n",
-				(unsigned long)buf->vaddr,
+	dprintk(1, "vmalloc is at addr 0x%p, size=%d\n",
+				buf->vaddr,
 				nr_pages << PAGE_SHIFT);
 
 	memset(buf->vaddr, 0, nr_pages << PAGE_SHIFT);
-- 
2.13.6

[PATCH v2 10/26] media: xc4000: don't ignore error if hwmodel fails

[Mauro Carvalho Chehab]

If, for some reason, reading the hwmodel register on
xc4000 fails, it will cause the following logig to
use a random value, as reported by smatch:

	drivers/media/tuners/xc4000.c:1047 check_firmware() error: uninitialized symbol 'hwmodel'.
	drivers/media/tuners/xc4000.c:1060 check_firmware() error: uninitialized symbol 'hwmodel'.
	drivers/media/tuners/xc4000.c:1064 check_firmware() error: uninitialized symbol 'hwmodel'.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/tuners/xc4000.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/tuners/xc4000.c b/drivers/media/tuners/xc4000.c
index e30948e4ff87..2113ce594f75 100644
--- a/drivers/media/tuners/xc4000.c
+++ b/drivers/media/tuners/xc4000.c
@@ -1036,7 +1036,10 @@ static int check_firmware(struct dvb_frontend *fe, unsigned int type,
 		dprintk(1, "load scode failed %d\n", rc);
 
 check_device:
-	rc = xc4000_readreg(priv, XREG_PRODUCT_ID, &hwmodel);
+	if (xc4000_readreg(priv, XREG_PRODUCT_ID, &hwmodel) < 0) {
+		printk(KERN_ERR "Unable to read tuner registers.\n");
+		goto fail;
+	}
 
 	if (xc_get_version(priv, &hw_major, &hw_minor, &fw_major,
 			   &fw_minor) != 0) {
-- 
2.13.6

[PATCH v2 11/26] media: qt1010: fix bogus warnings

[Mauro Carvalho Chehab]

The logic at qt1010_init_meas1() and qt1010_init_meas2()
are too complex for static analizers to identify that
some vars are always be initialized.

That causes smatch to produce the following warnings:
	drivers/media/tuners/qt1010.c:248 qt1010_init_meas1() error: uninitialized symbol 'val2'.
	drivers/media/tuners/qt1010.c:282 qt1010_init_meas2() error: uninitialized symbol 'val'.

So, add annotations to prevent those bogus warnings.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/tuners/qt1010.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/tuners/qt1010.c b/drivers/media/tuners/qt1010.c
index ee33b7cc7682..b92be882ab3c 100644
--- a/drivers/media/tuners/qt1010.c
+++ b/drivers/media/tuners/qt1010.c
@@ -224,7 +224,7 @@ static int qt1010_set_params(struct dvb_frontend *fe)
 static int qt1010_init_meas1(struct qt1010_priv *priv,
 			     u8 oper, u8 reg, u8 reg_init_val, u8 *retval)
 {
-	u8 i, val1, val2;
+	u8 i, val1, uninitialized_var(val2);
 	int err;
 
 	qt1010_i2c_oper_t i2c_data[] = {
@@ -259,7 +259,7 @@ static int qt1010_init_meas1(struct qt1010_priv *priv,
 static int qt1010_init_meas2(struct qt1010_priv *priv,
 			    u8 reg_init_val, u8 *retval)
 {
-	u8 i, val;
+	u8 i, uninitialized_var(val);
 	int err;
 	qt1010_i2c_oper_t i2c_data[] = {
 		{ QT1010_WR, 0x07, reg_init_val },
-- 
2.13.6

[PATCH v2 12/26] media: davinci: fix a debug printk

[Mauro Carvalho Chehab]

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 <mchehab@s-opensource.com>
---
 drivers/media/platform/davinci/vpif_capture.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index a89367ab1e06..1a443181a320 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1397,9 +1397,9 @@ static int vpif_async_bound(struct v4l2_async_notifier *notifier,
 			vpif_obj.config->chan_config->inputs[i].subdev_name =
 				(char *)to_of_node(subdev->fwnode)->full_name;
 			vpif_dbg(2, debug,
-				 "%s: setting input %d subdev_name = %pOF\n",
+				 "%s: setting input %d subdev_name = %s\n",
 				 __func__, i,
-				 to_of_node(subdev->fwnode));
+				vpif_obj.config->chan_config->inputs[i].subdev_name);
 			return 0;
 		}
 	}
-- 
2.13.6

Re: [PATCH v2 12/26] media: davinci: fix a debug printk

[Lad, Prabhakar]

On Wed, Nov 1, 2017 at 9:05 PM, Mauro Carvalho Chehab
<mchehab@s-opensource.com> wrote:
> 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 <mchehab@s-opensource.com>
> ---
>  drivers/media/platform/davinci/vpif_capture.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Cheers,
--Prabhakar Lad

[PATCH v2 13/26] media: rcar: fix a debug printk

[Mauro Carvalho Chehab]

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 <mchehab@s-opensource.com>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 108d776f3265..ce5914f7a056 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -186,8 +186,8 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
 	if (!vin->digital)
 		return -ENODEV;
 
-	vin_dbg(vin, "Found digital subdevice %pOF\n",
-		to_of_node(vin->digital->asd.match.fwnode.fwnode));
+	vin_dbg(vin, "Found digital subdevice %s\n",
+		to_of_node(vin->digital->asd.match.fwnode.fwnode)->full_name);
 
 	vin->notifier.ops = &rvin_digital_notify_ops;
 	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
-- 
2.13.6

Re: [PATCH v2 13/26] media: rcar: fix a debug printk

[Niklas Söderlund]

Hi Mauro,

Thanks for your patch.

On 2017-11-01 17:05:50 -0400, Mauro Carvalho Chehab wrote:
> 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 <mchehab@s-opensource.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 108d776f3265..ce5914f7a056 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -186,8 +186,8 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
>  	if (!vin->digital)
>  		return -ENODEV;
>  
> -	vin_dbg(vin, "Found digital subdevice %pOF\n",
> -		to_of_node(vin->digital->asd.match.fwnode.fwnode));
> +	vin_dbg(vin, "Found digital subdevice %s\n",
> +		to_of_node(vin->digital->asd.match.fwnode.fwnode)->full_name);

For the same reasons as Laurent brings up in patch 14/26 I'm a bit 
sceptical to this change.

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

-- 
Regards,
Niklas Söderlund

[PATCH v2 14/26] media: xilinx: fix a debug printk

[Mauro Carvalho Chehab]

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 <mchehab@s-opensource.com>
---
 drivers/media/platform/xilinx/xilinx-vipp.c | 31 ++++++++++++++++-------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
index d881cf09876d..dd777c834c43 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -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;
 			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);
 			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;
-- 
2.13.6

Re: [PATCH v2 14/26] media: xilinx: fix a debug printk

[Laurent Pinchart]

Hi Mauro,

(CC'ing Rob and Sakari)

Thank you for the patch.

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*'
> 
> 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.

> Fixes: 68d9c47b1679 ("media: Convert to using %pOF instead of full_name")
> Fixes: 859969b38e2e ("[media] v4l: Switch from V4L2 OF not V4L2 fwnode API")

When submitting fixes you should CC the author of the original commits. They 
should have more information about the context, and in this case I believe Rob 
would have pointed out that adding back references to full_name would break 
the refactoring he's working on.

> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/platform/xilinx/xilinx-vipp.c | 31 +++++++++++++-------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
> b/drivers/media/platform/xilinx/xilinx-vipp.c index
> d881cf09876d..dd777c834c43 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -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;
>  			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);
>  			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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 14/26] media: xilinx: fix a debug printk

[Sakari Ailus]

Hi Laurent and Mauro,

On Thu, Nov 02, 2017 at 04:43:51AM +0200, Laurent Pinchart wrote:
> Hi Mauro,
> 
> (CC'ing Rob and Sakari)
> 
> Thank you for the patch.
> 
> 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*'
> > 
> > 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.

Probably because NULL maybe returned by the function. I can write a patch
for that. I presume the same issue would be present in a lot of places.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH 1/1] of: Make return types of to_of_node and of_fwnode_handle macros consistent

[Sakari Ailus]

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 <sakari.ailus@linux.intel.com>
---
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(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index b240ed69dc96..0651231c115e 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -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)
-- 
2.11.0

[RESEND PATCH 1/1] of: Make return types of to_of_node and of_fwnode_handle macros consistent

[Sakari Ailus]

(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 <sakari.ailus@linux.intel.com>
---
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(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index b240ed69dc96..0651231c115e 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -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)
-- 
2.11.0

Re: [RESEND PATCH 1/1] of: Make return types of to_of_node and of_fwnode_handle macros consistent

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thursday, 2 November 2017 11:59:18 EET Sakari Ailus wrote:
> (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 <sakari.ailus@linux.intel.com>

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

> ---
> 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(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index b240ed69dc96..0651231c115e 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -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)


-- 
Regards,

Laurent Pinchart

Re: [RESEND PATCH 1/1] of: Make return types of to_of_node and of_fwnode_handle macros consistent

[Rob Herring]

On Thu, Nov 02, 2017 at 11:59:18AM +0200, Sakari Ailus wrote:
> (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 <sakari.ailus@linux.intel.com>
> ---
> 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(-)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v2 15/26] media: pt1: fix logic when pt1_nr_tables is zero or negative

[Mauro Carvalho Chehab]

pt1_nr_tables is a modprobe parameter. The way the logic
handles it, it can't be negative. However, user can
set it to zero.

If set to zero, however, it will cause troubles at
pt1_init_tables(), as reported by smatch:
	drivers/media/pci/pt1/pt1.c:468 pt1_init_tables() error: uninitialized symbol 'first_pfn'.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/pt1/pt1.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/pci/pt1/pt1.c b/drivers/media/pci/pt1/pt1.c
index b6b1a8d20d86..acc3afeb6224 100644
--- a/drivers/media/pci/pt1/pt1.c
+++ b/drivers/media/pci/pt1/pt1.c
@@ -116,8 +116,8 @@ static u32 pt1_read_reg(struct pt1 *pt1, int reg)
 	return readl(pt1->regs + reg * 4);
 }
 
-static int pt1_nr_tables = 8;
-module_param_named(nr_tables, pt1_nr_tables, int, 0);
+static unsigned int pt1_nr_tables = 8;
+module_param_named(nr_tables, pt1_nr_tables, uint, 0);
 
 static void pt1_increment_table_count(struct pt1 *pt1)
 {
@@ -443,6 +443,9 @@ static int pt1_init_tables(struct pt1 *pt1)
 	int i, ret;
 	u32 first_pfn, pfn;
 
+	if (!pt1_nr_tables)
+		return 0;
+
 	tables = vmalloc(sizeof(struct pt1_table) * pt1_nr_tables);
 	if (tables == NULL)
 		return -ENOMEM;
@@ -450,12 +453,10 @@ static int pt1_init_tables(struct pt1 *pt1)
 	pt1_init_table_count(pt1);
 
 	i = 0;
-	if (pt1_nr_tables) {
-		ret = pt1_init_table(pt1, &tables[0], &first_pfn);
-		if (ret)
-			goto err;
-		i++;
-	}
+	ret = pt1_init_table(pt1, &tables[0], &first_pfn);
+	if (ret)
+		goto err;
+	i++;
 
 	while (i < pt1_nr_tables) {
 		ret = pt1_init_table(pt1, &tables[i], &pfn);
-- 
2.13.6

[PATCH v2 16/26] media: drxd_hard: better handle I2C errors

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/dvb-frontends/drxd_hard.c:989 HI_Command() error: uninitialized symbol 'waitCmd'.
	drivers/media/dvb-frontends/drxd_hard.c:1306 SC_WaitForReady() error: uninitialized symbol 'curCmd'.
	drivers/media/dvb-frontends/drxd_hard.c:1322 SC_SendCommand() error: uninitialized symbol 'errCode'.
	drivers/media/dvb-frontends/drxd_hard.c:1339 SC_ProcStartCommand() error: uninitialized symbol 'scExec'.

The error handling on several places are somewhat flawed, as
they don't check if Read16() returns an error.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-frontends/drxd_hard.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/media/dvb-frontends/drxd_hard.c b/drivers/media/dvb-frontends/drxd_hard.c
index 3bdf9b1f4e7c..79210db117a7 100644
--- a/drivers/media/dvb-frontends/drxd_hard.c
+++ b/drivers/media/dvb-frontends/drxd_hard.c
@@ -972,7 +972,6 @@ static int DownloadMicrocode(struct drxd_state *state,
 static int HI_Command(struct drxd_state *state, u16 cmd, u16 * pResult)
 {
 	u32 nrRetries = 0;
-	u16 waitCmd;
 	int status;
 
 	status = Write16(state, HI_RA_RAM_SRV_CMD__A, cmd, 0);
@@ -985,8 +984,8 @@ static int HI_Command(struct drxd_state *state, u16 cmd, u16 * pResult)
 			status = -1;
 			break;
 		}
-		status = Read16(state, HI_RA_RAM_SRV_CMD__A, &waitCmd, 0);
-	} while (waitCmd != 0);
+		status = Read16(state, HI_RA_RAM_SRV_CMD__A, NULL, 0);
+	} while (status != 0);
 
 	if (status >= 0)
 		status = Read16(state, HI_RA_RAM_SRV_RES__A, pResult, 0);
@@ -1298,12 +1297,11 @@ static int InitFT(struct drxd_state *state)
 
 static int SC_WaitForReady(struct drxd_state *state)
 {
-	u16 curCmd;
 	int i;
 
 	for (i = 0; i < DRXD_MAX_RETRIES; i += 1) {
-		int status = Read16(state, SC_RA_RAM_CMD__A, &curCmd, 0);
-		if (status == 0 || curCmd == 0)
+		int status = Read16(state, SC_RA_RAM_CMD__A, NULL, 0);
+		if (status == 0)
 			return status;
 	}
 	return -1;
@@ -1311,15 +1309,15 @@ static int SC_WaitForReady(struct drxd_state *state)
 
 static int SC_SendCommand(struct drxd_state *state, u16 cmd)
 {
-	int status = 0;
+	int status = 0, ret;
 	u16 errCode;
 
 	Write16(state, SC_RA_RAM_CMD__A, cmd, 0);
 	SC_WaitForReady(state);
 
-	Read16(state, SC_RA_RAM_CMD_ADDR__A, &errCode, 0);
+	ret = Read16(state, SC_RA_RAM_CMD_ADDR__A, &errCode, 0);
 
-	if (errCode == 0xFFFF) {
+	if (ret < 0 || errCode == 0xFFFF) {
 		printk(KERN_ERR "Command Error\n");
 		status = -1;
 	}
@@ -1330,13 +1328,13 @@ static int SC_SendCommand(struct drxd_state *state, u16 cmd)
 static int SC_ProcStartCommand(struct drxd_state *state,
 			       u16 subCmd, u16 param0, u16 param1)
 {
-	int status = 0;
+	int ret, status = 0;
 	u16 scExec;
 
 	mutex_lock(&state->mutex);
 	do {
-		Read16(state, SC_COMM_EXEC__A, &scExec, 0);
-		if (scExec != 1) {
+		ret = Read16(state, SC_COMM_EXEC__A, &scExec, 0);
+		if (ret < 0 || scExec != 1) {
 			status = -1;
 			break;
 		}
-- 
2.13.6

[PATCH v2 17/26] media: mxl111sf: improve error handling logic

[Mauro Carvalho Chehab]

As reported by smatch:
	drivers/media/usb/dvb-usb-v2/mxl111sf-demod.c:485 mxl111sf_demod_read_signal_strength() error: uninitialized symbol 'modulation'.

The mxl111sf_demod_read_signal_strength() just ignores if something
gets wrong while reading snr or modulation.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/usb/dvb-usb-v2/mxl111sf-demod.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/mxl111sf-demod.c b/drivers/media/usb/dvb-usb-v2/mxl111sf-demod.c
index f0ed37da73d4..221cf46b4140 100644
--- a/drivers/media/usb/dvb-usb-v2/mxl111sf-demod.c
+++ b/drivers/media/usb/dvb-usb-v2/mxl111sf-demod.c
@@ -477,10 +477,15 @@ static int mxl111sf_demod_read_signal_strength(struct dvb_frontend *fe,
 {
 	struct mxl111sf_demod_state *state = fe->demodulator_priv;
 	enum fe_modulation modulation;
+	int ret;
 	u16 snr;
 
-	mxl111sf_demod_calc_snr(state, &snr);
-	mxl1x1sf_demod_get_tps_modulation(state, &modulation);
+	ret = mxl111sf_demod_calc_snr(state, &snr);
+	if (ret < 0)
+		return ret;
+	ret = mxl1x1sf_demod_get_tps_modulation(state, &modulation);
+	if (ret < 0)
+		return ret;
 
 	switch (modulation) {
 	case QPSK:
-- 
2.13.6

[PATCH v2 18/26] media: dvbsky: shut up a bogus warning

[Mauro Carvalho Chehab]

Smatch gives two bogus warnings on this driver:
	drivers/media/usb/dvb-usb-v2/dvbsky.c:336 dvbsky_s960_attach() error: uninitialized symbol 'i2c_adapter'.
	drivers/media/usb/dvb-usb-v2/dvbsky.c:459 dvbsky_s960c_attach() error: uninitialized symbol 'i2c_adapter'.

Shut them up.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/usb/dvb-usb-v2/dvbsky.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c b/drivers/media/usb/dvb-usb-v2/dvbsky.c
index 131b6c08e199..f90540f947f8 100644
--- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
+++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
@@ -309,7 +309,7 @@ static int dvbsky_s960_attach(struct dvb_usb_adapter *adap)
 	struct dvb_usb_device *d = adap_to_d(adap);
 	int ret = 0;
 	/* demod I2C adapter */
-	struct i2c_adapter *i2c_adapter;
+	struct i2c_adapter *i2c_adapter = NULL;
 	struct i2c_client *client;
 	struct i2c_board_info info;
 	struct ts2020_config ts2020_config = {};
@@ -431,7 +431,7 @@ static int dvbsky_s960c_attach(struct dvb_usb_adapter *adap)
 	struct dvb_usb_device *d = adap_to_d(adap);
 	int ret = 0;
 	/* demod I2C adapter */
-	struct i2c_adapter *i2c_adapter;
+	struct i2c_adapter *i2c_adapter = NULL;
 	struct i2c_client *client_tuner, *client_ci;
 	struct i2c_board_info info;
 	struct sp2_config sp2_config;
-- 
2.13.6

[PATCH v2 19/26] media: ov9650: fix bogus warnings

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

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

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/i2c/ov9650.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 69433e1e2533..e519f278d5f9 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -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) |
-- 
2.13.6

Re: [PATCH v2 19/26] media: ov9650: fix bogus warnings

[Nicholas Mc Guire]

On Wed, Nov 01, 2017 at 05:05:56PM -0400, Mauro Carvalho Chehab wrote:
> 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

> 
> So, let's stick with the syntax that won't cause read
> issues.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Reviewed-by: Nicholas Mc Guire <hofrat@osadl.org>

> ---
>  drivers/media/i2c/ov9650.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 69433e1e2533..e519f278d5f9 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -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) |
> -- 
> 2.13.6
> 

Re: [PATCH v2 19/26] media: ov9650: fix bogus warnings

[Nicholas Mc Guire]

On Thu, Nov 02, 2017 at 10:06:06AM +0000, Nicholas Mc Guire wrote:
> On Wed, Nov 01, 2017 at 05:05:56PM -0400, Mauro Carvalho Chehab wrote:
> > 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.

sorry - sent that out too fast - the logic is equivalent the negative
returns are treated correctly because only the success case is 
being returned explicidly as 0 in ov965x_read() return statement
by checking for the expecte return of 1 from i2c_transfer() wrapping
it to 0.

sorry for the noise.

thx!
hofrat

> 
> 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
> 
> > 
> > So, let's stick with the syntax that won't cause read
> > issues.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> 
> Reviewed-by: Nicholas Mc Guire <hofrat@osadl.org>
> 
> > ---
> >  drivers/media/i2c/ov9650.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> > index 69433e1e2533..e519f278d5f9 100644
> > --- a/drivers/media/i2c/ov9650.c
> > +++ b/drivers/media/i2c/ov9650.c
> > @@ -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) |
> > -- 
> > 2.13.6
> > 

[PATCH v2 20/26] media: imx274: don't randomly return if range_count is zero

[Mauro Carvalho Chehab]

As smatch reported:
	drivers/media/i2c/imx274.c:659 imx274_regmap_util_write_table_8() error: uninitialized symbol 'err'.

There is a bug at imx274_regmap_util_write_table_8() with causes
it to randomly return a random error if range_count is zero.

Worse than that, the logic there starts with range_count
equal to zero, and periodically resets it to zero again.

As it is a way more likely that err assumes a non-zero value,
I suspect that the chance of this code to run is very small,
so, it would be worth to review the entire function.

Anyway, clearly it shouldn't be returning error if range_count
is zero. So, let's fix it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/i2c/imx274.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index ab6a5f31da74..0d8314bfd3cb 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -655,6 +655,8 @@ static int imx274_regmap_util_write_table_8(struct regmap *regmap,
 				err = regmap_bulk_write(regmap, range_start,
 							&range_vals[0],
 							range_count);
+			else
+				err = 0;
 
 			if (err)
 				return err;
-- 
2.13.6

[PATCH v2 21/26] media: m88rs2000: handle the case where tuner doesn't have get_frequency

[Mauro Carvalho Chehab]

If the tuner doesn't have get_frequency() callback, the current
code will place a random value as the frequency offset. That
doesn't seem right! The better is to just assume that, on such
case, the tuner was able to set the exact frequency that was
requested.

Fixes a smatch warning:
	drivers/media/dvb-frontends/m88rs2000.c:639 m88rs2000_set_frontend() error: uninitialized symbol 'tuner_freq'.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-frontends/m88rs2000.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-frontends/m88rs2000.c b/drivers/media/dvb-frontends/m88rs2000.c
index ce6c21d405ee..e34dab41d104 100644
--- a/drivers/media/dvb-frontends/m88rs2000.c
+++ b/drivers/media/dvb-frontends/m88rs2000.c
@@ -630,13 +630,16 @@ static int m88rs2000_set_frontend(struct dvb_frontend *fe)
 	if (ret < 0)
 		return -ENODEV;
 
-	if (fe->ops.tuner_ops.get_frequency)
+	if (fe->ops.tuner_ops.get_frequency) {
 		ret = fe->ops.tuner_ops.get_frequency(fe, &tuner_freq);
 
-	if (ret < 0)
-		return -ENODEV;
+		if (ret < 0)
+			return -ENODEV;
 
-	offset = (s16)((s32)tuner_freq - c->frequency);
+		offset = (s16)((s32)tuner_freq - c->frequency);
+	} else {
+		offset = 0;
+	}
 
 	/* default mclk value 96.4285 * 2 * 1000 = 192857 */
 	if (((c->frequency % 192857) >= (192857 - 3000)) ||
-- 
2.13.6

[PATCH v2 22/26] [RFC] media: cxd2841er: ensure that status will always be available

[Mauro Carvalho Chehab]

The loop with read status use a dynamic timeout value, calculated
from symbol rate. It should run the loop at least one time for
the status to be handled after the loop.

While this should, in practice, happen every time, it doesn't
hurt to change the logic to make it explicit.

This solves a smatch warning:
	drivers/media/dvb-frontends/cxd2841er.c:3350 cxd2841er_set_frontend_s() error: uninitialized symbol 'status'.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

---

On a separate note, it looks weird to have something there waiting
for lock. This should happen on userspace, except if there are
some bugs at the hardware that prevent it to work otherwise.
---
 drivers/media/dvb-frontends/cxd2841er.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/cxd2841er.c b/drivers/media/dvb-frontends/cxd2841er.c
index 48ee9bc00c06..98e40b7adad5 100644
--- a/drivers/media/dvb-frontends/cxd2841er.c
+++ b/drivers/media/dvb-frontends/cxd2841er.c
@@ -3340,13 +3340,17 @@ static int cxd2841er_set_frontend_s(struct dvb_frontend *fe)
 
 	cxd2841er_tune_done(priv);
 	timeout = ((3000000 + (symbol_rate - 1)) / symbol_rate) + 150;
-	for (i = 0; i < timeout / CXD2841ER_DVBS_POLLING_INVL; i++) {
+
+	i = 0;
+	do {
 		usleep_range(CXD2841ER_DVBS_POLLING_INVL*1000,
 			(CXD2841ER_DVBS_POLLING_INVL + 2) * 1000);
 		cxd2841er_read_status_s(fe, &status);
 		if (status & FE_HAS_LOCK)
 			break;
-	}
+		i++;
+	} while (i < timeout / CXD2841ER_DVBS_POLLING_INVL);
+
 	if (status & FE_HAS_LOCK) {
 		if (cxd2841er_get_carrier_offset_s_s2(
 				priv, &carr_offset)) {
-- 
2.13.6

[PATCH v2 23/26] media: drxj: better handle errors

[Mauro Carvalho Chehab]

as reported by smatch:
	drivers/media/dvb-frontends/drx39xyj/drxj.c:2157 drxj_dap_atomic_read_write_block() error: uninitialized symbol 'word'.

The driver doesn't check if a read error occurred. Add such
check.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-frontends/drx39xyj/drxj.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/drx39xyj/drxj.c b/drivers/media/dvb-frontends/drx39xyj/drxj.c
index 499ccff557bf..28e24d5b7fb3 100644
--- a/drivers/media/dvb-frontends/drx39xyj/drxj.c
+++ b/drivers/media/dvb-frontends/drx39xyj/drxj.c
@@ -2151,9 +2151,13 @@ int drxj_dap_atomic_read_write_block(struct i2c_device_addr *dev_addr,
 	if (read_flag) {
 		/* read data from buffer */
 		for (i = 0; i < (datasize / 2); i++) {
-			drxj_dap_read_reg16(dev_addr,
-					    (DRXJ_HI_ATOMIC_BUF_START + i),
-					   &word, 0);
+			rc = drxj_dap_read_reg16(dev_addr,
+						 (DRXJ_HI_ATOMIC_BUF_START + i),
+						 &word, 0);
+			if (rc) {
+				pr_err("error %d\n", rc);
+				goto rw_error;
+			}
 			data[2 * i] = (u8) (word & 0xFF);
 			data[(2 * i) + 1] = (u8) (word >> 8);
 		}
-- 
2.13.6

[PATCH v2 24/26] media: stv090x: Only print tuner lock if get_status is available

[Mauro Carvalho Chehab]

The current code doesn't report tuner lock properly if the
tuner get_status callback is not available, as reported by
smatch:
	drivers/media/dvb-frontends/stv090x.c:2220 stv090x_get_coldlock() error: uninitialized symbol 'reg'.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-frontends/stv090x.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
index 7ef469c0c866..0f375df13fbe 100644
--- a/drivers/media/dvb-frontends/stv090x.c
+++ b/drivers/media/dvb-frontends/stv090x.c
@@ -2215,13 +2215,12 @@ static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd)
 		if (state->config->tuner_get_status) {
 			if (state->config->tuner_get_status(fe, &reg) < 0)
 				goto err_gateoff;
+			if (reg)
+				dprintk(FE_DEBUG, 1, "Tuner phase locked");
+			else
+				dprintk(FE_DEBUG, 1, "Tuner unlocked");
 		}
 
-		if (reg)
-			dprintk(FE_DEBUG, 1, "Tuner phase locked");
-		else
-			dprintk(FE_DEBUG, 1, "Tuner unlocked");
-
 		if (stv090x_i2c_gate_ctrl(state, 0) < 0)
 			goto err;
 
-- 
2.13.6

[PATCH v2 25/26] media: mb86a16: be more resilient if I2C fails on sync

[Mauro Carvalho Chehab]

If the I2C read fails while check for sync, there's no point
on doing adjusting the tuner due to a random value that might
be at VIRM var. So, set VIRM to zero, as that makes the caller
for check_sync() to return an error.

Fix those smatch warnings:
	drivers/media/dvb-frontends/mb86a16.c:1460 mb86a16_set_fe() error: uninitialized symbol 'VIRM'.
	drivers/media/dvb-frontends/mb86a16.c:1461 mb86a16_set_fe() error: uninitialized symbol 'VIRM'.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-frontends/mb86a16.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/dvb-frontends/mb86a16.c b/drivers/media/dvb-frontends/mb86a16.c
index dfe322eccaa1..f1aad52094c3 100644
--- a/drivers/media/dvb-frontends/mb86a16.c
+++ b/drivers/media/dvb-frontends/mb86a16.c
@@ -635,6 +635,7 @@ static int sync_chk(struct mb86a16_state *state,
 	return sync;
 err:
 	dprintk(verbose, MB86A16_ERROR, 1, "I2C transfer error");
+	*VIRM = 0;
 	return -EREMOTEIO;
 
 }
-- 
2.13.6

[PATCH v2 26/26] media: mb86a16: avoid division by zero

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/dvb-frontends/mb86a16.c:1690 mb86a16_read_ber() error: uninitialized symbol 'timer'.
	drivers/media/dvb-frontends/mb86a16.c:1706 mb86a16_read_ber() error: uninitialized symbol 'timer'.

There is a potential risk of doing a division by zero if
timer is not handled well. Enforce it by setting a bit mask
for the values used to select the timer.

While here, optimize the logic to prevent uneeded tests.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-frontends/mb86a16.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/dvb-frontends/mb86a16.c b/drivers/media/dvb-frontends/mb86a16.c
index f1aad52094c3..5d2bb76bc07a 100644
--- a/drivers/media/dvb-frontends/mb86a16.c
+++ b/drivers/media/dvb-frontends/mb86a16.c
@@ -1677,15 +1677,15 @@ static int mb86a16_read_ber(struct dvb_frontend *fe, u32 *ber)
 			 * the deinterleaver output.
 			 * monitored BER is expressed as a 20 bit output in total
 			 */
-			ber_rst = ber_mon >> 3;
+			ber_rst = (ber_mon >> 3) & 0x03;
 			*ber = (((ber_msb << 8) | ber_mid) << 8) | ber_lsb;
 			if (ber_rst == 0)
 				timer =  12500000;
-			if (ber_rst == 1)
+			else if (ber_rst == 1)
 				timer =  25000000;
-			if (ber_rst == 2)
+			else if (ber_rst == 2)
 				timer =  50000000;
-			if (ber_rst == 3)
+			else /* ber_rst == 3 */
 				timer = 100000000;
 
 			*ber /= timer;
@@ -1697,11 +1697,11 @@ static int mb86a16_read_ber(struct dvb_frontend *fe, u32 *ber)
 			 * QPSK demodulator output.
 			 * monitored BER is expressed as a 24 bit output in total
 			 */
-			ber_tim = ber_mon >> 1;
+			ber_tim = (ber_mon >> 1) & 0x01;
 			*ber = (((ber_msb << 8) | ber_mid) << 8) | ber_lsb;
 			if (ber_tim == 0)
 				timer = 16;
-			if (ber_tim == 1)
+			else /* ber_tim == 1 */
 				timer = 24;
 
 			*ber /= 2 ^ timer;
-- 
2.13.6