The function can return negative values, so its result should
be assigned to signed variable.
The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
Signed-off-by: Andrzej Hajda <[email protected]>
---
drivers/cpufreq/scpi-cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
index 2c3b16f..de5e89b 100644
--- a/drivers/cpufreq/scpi-cpufreq.c
+++ b/drivers/cpufreq/scpi-cpufreq.c
@@ -31,7 +31,7 @@ static struct scpi_ops *scpi_ops;
static struct scpi_dvfs_info *scpi_get_dvfs_info(struct device *cpu_dev)
{
- u8 domain = topology_physical_package_id(cpu_dev->id);
+ int domain = topology_physical_package_id(cpu_dev->id);
if (domain < 0)
return ERR_PTR(-EINVAL);
--
1.9.1
Iterator i declared as unsigned is always non-negative so the
loop will never end.
The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
Signed-off-by: Andrzej Hajda <[email protected]>
---
drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
index 34648f6..ad5b04c 100644
--- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
@@ -439,7 +439,8 @@ static int nsp_gpio_set_strength(struct nsp_gpio *chip, unsigned gpio,
static int nsp_gpio_get_strength(struct nsp_gpio *chip, unsigned gpio,
u16 *strength)
{
- unsigned int i, offset, shift;
+ unsigned int offset, shift;
+ int i;
u32 val;
unsigned long flags;
--
1.9.1
The function return NCI_HCI_INVALID_PIPE in case of error.
The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
Signed-off-by: Andrzej Hajda <[email protected]>
---
net/nfc/nci/hci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/nfc/nci/hci.c b/net/nfc/nci/hci.c
index 2aedac1..a0ab26d 100644
--- a/net/nfc/nci/hci.c
+++ b/net/nfc/nci/hci.c
@@ -676,7 +676,7 @@ int nci_hci_connect_gate(struct nci_dev *ndev,
break;
default:
pipe = nci_hci_create_pipe(ndev, dest_host, dest_gate, &r);
- if (pipe < 0)
+ if (pipe == NCI_HCI_INVALID_PIPE)
return r;
pipe_created = true;
break;
--
1.9.1
Field usrcnt is unsigned so it cannot be lesser than zero.
The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
Signed-off-by: Andrzej Hajda <[email protected]>
---
sound/soc/sh/rcar/ssi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
index 0b91692..8ca30fd 100644
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -381,9 +381,9 @@ rsnd_ssi_quit_end:
rsnd_mod_power_off(mod);
- ssi->usrcnt--;
-
- if (ssi->usrcnt < 0)
+ if (ssi->usrcnt > 0)
+ ssi->usrcnt--;
+ else
dev_err(dev, "%s[%d] usrcnt error\n",
rsnd_mod_name(mod), rsnd_mod_id(mod));
--
1.9.1
On 23-12-15, 11:37, Andrzej Hajda wrote:
> The function can return negative values, so its result should
> be assigned to signed variable.
>
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
>
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
>
> Signed-off-by: Andrzej Hajda <[email protected]>
> ---
> drivers/cpufreq/scpi-cpufreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
> index 2c3b16f..de5e89b 100644
> --- a/drivers/cpufreq/scpi-cpufreq.c
> +++ b/drivers/cpufreq/scpi-cpufreq.c
> @@ -31,7 +31,7 @@ static struct scpi_ops *scpi_ops;
>
> static struct scpi_dvfs_info *scpi_get_dvfs_info(struct device *cpu_dev)
> {
> - u8 domain = topology_physical_package_id(cpu_dev->id);
> + int domain = topology_physical_package_id(cpu_dev->id);
>
> if (domain < 0)
> return ERR_PTR(-EINVAL);
Acked-by: Viresh Kumar <[email protected]>
--
viresh
On 23/12/15 10:37, Andrzej Hajda wrote:
> The function can return negative values, so its result should
> be assigned to signed variable.
>
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
>
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
>
There has a patch posted by Dan Carpenter [1] which I reposted[2],
but it again slipped through the cracks. I will poke Rafael again on that.
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/380292.html
[2] https://lkml.org/lkml/2015/12/15/219
--
Regards,
Sudeep
+ Reddy
On 12/23/2015 2:37 AM, Andrzej Hajda wrote:
> Iterator i declared as unsigned is always non-negative so the
> loop will never end.
>
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
>
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
>
> Signed-off-by: Andrzej Hajda <[email protected]>
> ---
> drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
> index 34648f6..ad5b04c 100644
> --- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
> +++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
> @@ -439,7 +439,8 @@ static int nsp_gpio_set_strength(struct nsp_gpio *chip, unsigned gpio,
> static int nsp_gpio_get_strength(struct nsp_gpio *chip, unsigned gpio,
> u16 *strength)
> {
> - unsigned int i, offset, shift;
> + unsigned int offset, shift;
> + int i;
> u32 val;
> unsigned long flags;
>
>
The fix is a valid fix. And at the same time it exposes other potential
issues in the driver. I just found the loop used in _set_strength and
_get_strength is inconsistent:
In _set_strength:
427 for (i = GPIO_DRV_STRENGTH_BITS; i > 0; i--) {
For i to start at GPIO_DRV_STRENGTH_BITS, it seems to be wrong.
428 val = readl(chip->io_ctrl + offset);
429 val &= ~BIT(shift);
430 val |= ((strength >> (i-1)) & 0x1) << shift;
431 writel(val, chip->io_ctrl + offset);
432 offset += 4;
433 }
In _get_strength:
451 for (i = (GPIO_DRV_STRENGTH_BITS - 1); i >= 0; i--) {
452 val = readl(chip->io_ctrl + offset) & BIT(shift);
453 val >>= shift;
454 *strength += (val << i);
455 offset += 4;
456 }
Reddy, could you please review and comment?
Thanks,
Ray
Hi Andrzej
> Field usrcnt is unsigned so it cannot be lesser than zero.
>
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
>
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
>
> Signed-off-by: Andrzej Hajda <[email protected]>
> ---
Thank you for your patch. good catch !
I noticed current error case is not good for ssi.c
Can you agree below ?
---------
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
index 7db05fd..e519e30 100644
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -403,6 +403,12 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod,
struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
struct device *dev = rsnd_priv_to_dev(priv);
+ if (!ssi->usrcnt) {
+ dev_err(dev, "%s[%d] usrcnt error\n",
+ rsnd_mod_name(mod), rsnd_mod_id(mod));
+ return -EIO;
+ }
+
if (rsnd_ssi_is_parent(mod, io))
goto rsnd_ssi_quit_end;
@@ -422,10 +428,6 @@ rsnd_ssi_quit_end:
ssi->usrcnt--;
- if (ssi->usrcnt < 0)
- dev_err(dev, "%s[%d] usrcnt error\n",
- rsnd_mod_name(mod), rsnd_mod_id(mod));
-
return 0;
}
---------
Best regards
---
Kuninori Morimoto
On 12/24/2015 4:05 AM, Ray Jui wrote:
> + Reddy
>
> On 12/23/2015 2:37 AM, Andrzej Hajda wrote:
>> Iterator i declared as unsigned is always non-negative so the
>> loop will never end.
>>
>> The problem has been detected using proposed semantic patch
>> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
>>
>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
>>
>> Signed-off-by: Andrzej Hajda <[email protected]>
>> ---
>> drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
>> index 34648f6..ad5b04c 100644
>> --- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
>> +++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
>> @@ -439,7 +439,8 @@ static int nsp_gpio_set_strength(struct nsp_gpio *chip, unsigned gpio,
>> static int nsp_gpio_get_strength(struct nsp_gpio *chip, unsigned gpio,
>> u16 *strength)
>> {
>> - unsigned int i, offset, shift;
>> + unsigned int offset, shift;
>> + int i;
>> u32 val;
>> unsigned long flags;
>>
>>
> The fix is a valid fix. And at the same time it exposes other potential
> issues in the driver. I just found the loop used in _set_strength and
> _get_strength is inconsistent:
>
> In _set_strength:
>
> 427 for (i = GPIO_DRV_STRENGTH_BITS; i > 0; i--) {
>
> For i to start at GPIO_DRV_STRENGTH_BITS, it seems to be wrong.
>
> 428 val = readl(chip->io_ctrl + offset);
> 429 val &= ~BIT(shift);
> 430 val |= ((strength >> (i-1)) & 0x1) << shift;
> 431 writel(val, chip->io_ctrl + offset);
> 432 offset += 4;
> 433 }
>
> In _get_strength:
>
> 451 for (i = (GPIO_DRV_STRENGTH_BITS - 1); i >= 0; i--) {
> 452 val = readl(chip->io_ctrl + offset) & BIT(shift);
> 453 val >>= shift;
> 454 *strength += (val << i);
> 455 offset += 4;
> 456 }
>
> Reddy, could you please review and comment?
Hi Ray,
The logic is correct. The drive strength has three bits distributed in three registers. In one case the "-1" is in for loop initialization and the other case it is in the for loop body. The fix looks good.
Thanks
Dhananjay
> Thanks,
>
> Ray
>
>
On 12/24/2015 01:04 AM, Kuninori Morimoto wrote:
> Hi Andrzej
>
>> Field usrcnt is unsigned so it cannot be lesser than zero.
>>
>> The problem has been detected using proposed semantic patch
>> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
>>
>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
>>
>> Signed-off-by: Andrzej Hajda <[email protected]>
>> ---
> Thank you for your patch. good catch !
> I noticed current error case is not good for ssi.c
> Can you agree below ?
Yes, of course.
Regards
Andrzej
>
> ---------
> diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
> index 7db05fd..e519e30 100644
> --- a/sound/soc/sh/rcar/ssi.c
> +++ b/sound/soc/sh/rcar/ssi.c
> @@ -403,6 +403,12 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod,
> struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
> struct device *dev = rsnd_priv_to_dev(priv);
>
> + if (!ssi->usrcnt) {
> + dev_err(dev, "%s[%d] usrcnt error\n",
> + rsnd_mod_name(mod), rsnd_mod_id(mod));
> + return -EIO;
> + }
> +
> if (rsnd_ssi_is_parent(mod, io))
> goto rsnd_ssi_quit_end;
>
> @@ -422,10 +428,6 @@ rsnd_ssi_quit_end:
>
> ssi->usrcnt--;
>
> - if (ssi->usrcnt < 0)
> - dev_err(dev, "%s[%d] usrcnt error\n",
> - rsnd_mod_name(mod), rsnd_mod_id(mod));
> -
> return 0;
> }
>
> ---------
>
>
> Best regards
> ---
> Kuninori Morimoto
>
>
Hi Andrzej
> >> Field usrcnt is unsigned so it cannot be lesser than zero.
> >>
> >> The problem has been detected using proposed semantic patch
> >> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
> >>
> >> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
> >>
> >> Signed-off-by: Andrzej Hajda <[email protected]>
> >> ---
> > Thank you for your patch. good catch !
> > I noticed current error case is not good for ssi.c
> > Can you agree below ?
> Yes, of course.
Thanks.
Who send this patch ? you or me ?
I think you can do it ?
Field usrcnt is unsigned so it cannot be lesser than zero.
The patch fixes the check, moves it to the beginning of the function
and changes return value to -EIO in case of usercnt error.
The problem has been detected using proposed semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
[1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
Signed-off-by: Andrzej Hajda <[email protected]>
---
v2: changed according to Kuninori Morimoto advice
---
sound/soc/sh/rcar/ssi.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
index 0b91692..f23c921 100644
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -364,29 +364,30 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod,
struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
struct device *dev = rsnd_priv_to_dev(priv);
- if (rsnd_ssi_is_parent(mod, io))
- goto rsnd_ssi_quit_end;
+ if (!ssi->usrcnt) {
+ dev_err(dev, "%s[%d] usrcnt error\n",
+ rsnd_mod_name(mod), rsnd_mod_id(mod));
+ return -EIO;
+ }
- if (ssi->err > 0)
- dev_warn(dev, "%s[%d] under/over flow err = %d\n",
- rsnd_mod_name(mod), rsnd_mod_id(mod), ssi->err);
+ if (!rsnd_ssi_is_parent(mod, io)) {
+ if (ssi->err > 0)
+ dev_warn(dev, "%s[%d] under/over flow err = %d\n",
+ rsnd_mod_name(mod), rsnd_mod_id(mod),
+ ssi->err);
- ssi->cr_own = 0;
- ssi->err = 0;
+ ssi->cr_own = 0;
+ ssi->err = 0;
- rsnd_ssi_irq_disable(mod);
+ rsnd_ssi_irq_disable(mod);
+ }
-rsnd_ssi_quit_end:
rsnd_ssi_master_clk_stop(ssi, io);
rsnd_mod_power_off(mod);
ssi->usrcnt--;
- if (ssi->usrcnt < 0)
- dev_err(dev, "%s[%d] usrcnt error\n",
- rsnd_mod_name(mod), rsnd_mod_id(mod));
-
return 0;
}
--
1.9.1
Hi
>
> Field usrcnt is unsigned so it cannot be lesser than zero.
> The patch fixes the check, moves it to the beginning of the function
> and changes return value to -EIO in case of usercnt error.
>
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
>
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
>
> Signed-off-by: Andrzej Hajda <[email protected]>
> ---
Acked-by: Kuninori Morimoto <[email protected]>
On Wed, Dec 23, 2015 at 11:37 AM, Andrzej Hajda <[email protected]> wrote:
> Iterator i declared as unsigned is always non-negative so the
> loop will never end.
>
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1].
>
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576
>
> Signed-off-by: Andrzej Hajda <[email protected]>
I already applied Dan Carpenters fix, somehow it was earlier
in my mail flow. Thanks anyways!
Yours,
Linus Walleij
On Thu, Dec 24, 2015 at 08:02:39AM +0100, Andrzej Hajda wrote:
> Field usrcnt is unsigned so it cannot be lesser than zero.
> The patch fixes the check, moves it to the beginning of the function
> and changes return value to -EIO in case of usercnt error.
Please don't send new patches in reply to old ones, it makes it hard to
figure out what the current version of things is.