[PATCH 00/14] Modify signed comparisons of unsigned variables

[PATCH 00/14] Modify signed comparisons of unsigned variables

[Tushar Behera]

The occurrences were identified through the coccinelle script at
following location.

http://www.emn.fr/z-info/coccinelle/rules/find_unsigned.cocci

Signed checks for unsigned variables are removed if it is also checked
for upper error limit. For error checks, IS_ERR_VALUE() macros is used.

Tushar Behera (14):
  [media] ivtv: Remove redundant check on unsigned variable
  [media] meye: Remove redundant check on unsigned variable
  [media] saa7134: Remove redundant check on unsigned variable
  [media] tlg2300: Remove redundant check on unsigned variable
  [media] atmel-isi: Update error check for unsigned variables
  pinctrl: samsung: Update error check for unsigned variables
  pinctrl: SPEAr: Update error check for unsigned variables
  xen: netback: Remove redundant check on unsigned variable
  xen: events: Remove redundant check on unsigned variable
  atm: Removed redundant check on unsigned variable
  HID: hiddev: Remove redundant check on unsigned variable
  gru: Remove redundant check on unsigned variable
  misc: tsl2550: Remove redundant check on unsigned variable
  wlcore: Remove redundant check on unsigned variable

 drivers/atm/fore200e.c                        |    2 +-
 drivers/hid/usbhid/hiddev.c                   |    2 +-
 drivers/media/pci/ivtv/ivtv-ioctl.c           |    2 +-
 drivers/media/pci/meye/meye.c                 |    2 +-
 drivers/media/pci/saa7134/saa7134-video.c     |    2 +-
 drivers/media/platform/soc_camera/atmel-isi.c |    2 +-
 drivers/media/usb/tlg2300/pd-video.c          |    2 +-
 drivers/misc/sgi-gru/grukdump.c               |    2 +-
 drivers/misc/tsl2550.c                        |    4 ++--
 drivers/net/wireless/ti/wlcore/debugfs.c      |    2 +-
 drivers/net/xen-netback/netback.c             |    4 ++--
 drivers/pinctrl/pinctrl-samsung.c             |    2 +-
 drivers/pinctrl/spear/pinctrl-plgpio.c        |    2 +-
 drivers/xen/events.c                          |    2 +-
 14 files changed, 16 insertions(+), 16 deletions(-)

-- 
1.7.4.1

CC: Mauro Carvalho Chehab <mchehab@infradead.org>
CC: Linus Walleij <linus.walleij@linaro.org>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Jeremy Fitzhardinge <jeremy@goop.org>
CC: Chas Williams <chas@cmf.nrl.navy.mil>
CC: Jack Steiner <steiner@sgi.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Luciano Coelho <coelho@ti.com>
CC: Jiri Kosina <jkosina@suse.cz>
CC: ivtv-devel@ivtvdriver.org
CC: linux-media@vger.kernel.org
CC: xen-devel@lists.xensource.com
CC: netdev@vger.kernel.org
CC: virtualization@lists.linux-foundation.org
CC: linux-atm-general@lists.sourceforge.net
CC: linux-usb@vger.kernel.org
CC: linux-input@vger.kernel.org
CC: linux-wireless@vger.kernel.org

[PATCH 08/14] xen: netback: Remove redundant check on unsigned variable

[Tushar Behera]

No need to check whether unsigned variable is less than 0.

CC: Ian Campbell <ian.campbell@citrix.com>
CC: xen-devel@lists.xensource.com
CC: netdev@vger.kernel.org
Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
---
 drivers/net/xen-netback/netback.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index aab8677..515e10c 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -190,14 +190,14 @@ static int get_page_ext(struct page *pg,
 
 	group = ext.e.group - 1;
 
-	if (group < 0 || group >= xen_netbk_group_nr)
+	if (group >= xen_netbk_group_nr)
 		return 0;
 
 	netbk = &xen_netbk[group];
 
 	idx = ext.e.idx;
 
-	if ((idx < 0) || (idx >= MAX_PENDING_REQS))
+	if (idx >= MAX_PENDING_REQS)
 		return 0;
 
 	if (netbk->mmap_pages[idx] != pg)
-- 
1.7.4.1

[PATCH 09/14] xen: events: Remove redundant check on unsigned variable

[Tushar Behera]

No need to check whether unsigned variable is less than 0.

CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Jeremy Fitzhardinge <jeremy@goop.org>
CC: xen-devel@lists.xensource.com
CC: virtualization@lists.linux-foundation.org
Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
---
 drivers/xen/events.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 4293c57..cadd7d1 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -216,7 +216,7 @@ static void xen_irq_info_pirq_init(unsigned irq,
  */
 static unsigned int evtchn_from_irq(unsigned irq)
 {
-	if (unlikely(WARN(irq < 0 || irq >= nr_irqs, "Invalid irq %d!\n", irq)))
+	if (unlikely(WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)))
 		return 0;
 
 	return info_for_irq(irq)->evtchn;
-- 
1.7.4.1

Re: [PATCH 08/14] xen: netback: Remove redundant check on unsigned variable

[Ian Campbell]

On Fri, 2012-11-16 at 06:50 +0000, Tushar Behera wrote:
> No need to check whether unsigned variable is less than 0.
> 
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: xen-devel@lists.xensource.com
> CC: netdev@vger.kernel.org
> Signed-off-by: Tushar Behera <tushar.behera@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks.

> ---
>  drivers/net/xen-netback/netback.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index aab8677..515e10c 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -190,14 +190,14 @@ static int get_page_ext(struct page *pg,
>  
>  	group = ext.e.group - 1;
>  
> -	if (group < 0 || group >= xen_netbk_group_nr)
> +	if (group >= xen_netbk_group_nr)
>  		return 0;
>  
>  	netbk = &xen_netbk[group];
>  
>  	idx = ext.e.idx;
>  
> -	if ((idx < 0) || (idx >= MAX_PENDING_REQS))
> +	if (idx >= MAX_PENDING_REQS)
>  		return 0;
>  
>  	if (netbk->mmap_pages[idx] != pg)

Re: [PATCH 09/14] xen: events: Remove redundant check on unsigned variable

[Konrad Rzeszutek Wilk]

On Fri, Nov 16, 2012 at 12:20:41PM +0530, Tushar Behera wrote:
> No need to check whether unsigned variable is less than 0.
> 
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> CC: Jeremy Fitzhardinge <jeremy@goop.org>
> CC: xen-devel@lists.xensource.com
> CC: virtualization@lists.linux-foundation.org
> Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
> ---
>  drivers/xen/events.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 4293c57..cadd7d1 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -216,7 +216,7 @@ static void xen_irq_info_pirq_init(unsigned irq,
>   */
>  static unsigned int evtchn_from_irq(unsigned irq)
>  {
> -	if (unlikely(WARN(irq < 0 || irq >= nr_irqs, "Invalid irq %d!\n", irq)))
> +	if (unlikely(WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)))
>  		return 0;
>  
>  	return info_for_irq(irq)->evtchn;
> -- 
> 1.7.4.1

Re: [PATCH 09/14] xen: events: Remove redundant check on unsigned variable

[Jeremy Fitzhardinge]

[-- Attachment #1.1: Type: text/plain, Size: 1462 bytes --]

To be honest I'd nack this kind of patch. The test is only redundant in the most trivial sense that the compiler can easily optimise away. The point of the test is to make sure that the range is OK even if the type subsequently becomes signed (to hold a -ve error, for example).

J

Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

>On Fri, Nov 16, 2012 at 12:20:41PM +0530, Tushar Behera wrote:
>> No need to check whether unsigned variable is less than 0.
>> 
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
>Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
>> CC: Jeremy Fitzhardinge <jeremy@goop.org>
>> CC: xen-devel@lists.xensource.com
>> CC: virtualization@lists.linux-foundation.org
>> Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
>> ---
>>  drivers/xen/events.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
>> index 4293c57..cadd7d1 100644
>> --- a/drivers/xen/events.c
>> +++ b/drivers/xen/events.c
>> @@ -216,7 +216,7 @@ static void xen_irq_info_pirq_init(unsigned irq,
>>   */
>>  static unsigned int evtchn_from_irq(unsigned irq)
>>  {
>> -	if (unlikely(WARN(irq < 0 || irq >= nr_irqs, "Invalid irq %d!\n",
>irq)))
>> +	if (unlikely(WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)))
>>  		return 0;
>>  
>>  	return info_for_irq(irq)->evtchn;
>> -- 
>> 1.7.4.1

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[-- Attachment #1.2: Type: text/html, Size: 2224 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 09/14] xen: events: Remove redundant check on unsigned variable

[Tushar Behera]

On 11/16/2012 10:23 PM, Jeremy Fitzhardinge wrote:
> To be honest I'd nack this kind of patch. The test is only redundant in the most trivial sense that the compiler can easily optimise away. The point of the test is to make sure that the range is OK even if the type subsequently becomes signed (to hold a -ve error, for example).
> 
> J
> 

The check is on the function argument which is unsigned, so checking '<
0' doesn't make sense. We should force signed check only if the argument
is of signed type. In any case, even if irq has been assigned some error
value, that would be caught by the check irq >= nr_irqs.

> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
>> On Fri, Nov 16, 2012 at 12:20:41PM +0530, Tushar Behera wrote:
>>> No need to check whether unsigned variable is less than 0.
>>>
>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>>> CC: Jeremy Fitzhardinge <jeremy@goop.org>
>>> CC: xen-devel@lists.xensource.com
>>> CC: virtualization@lists.linux-foundation.org
>>> Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
>>> ---
>>>  drivers/xen/events.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
>>> index 4293c57..cadd7d1 100644
>>> --- a/drivers/xen/events.c
>>> +++ b/drivers/xen/events.c
>>> @@ -216,7 +216,7 @@ static void xen_irq_info_pirq_init(unsigned irq,
>>>   */
>>>  static unsigned int evtchn_from_irq(unsigned irq)
>>>  {
>>> -	if (unlikely(WARN(irq < 0 || irq >= nr_irqs, "Invalid irq %d!\n",
>> irq)))
>>> +	if (unlikely(WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)))
>>>  		return 0;
>>>  
>>>  	return info_for_irq(irq)->evtchn;
>>> -- 
>>> 1.7.4.1
> 


-- 
Tushar Behera

Re: [PATCH 09/14] xen: events: Remove redundant check on unsigned variable

[Ian Campbell]

On Mon, 2012-11-19 at 03:52 +0000, Tushar Behera wrote:
> On 11/16/2012 10:23 PM, Jeremy Fitzhardinge wrote:
> > To be honest I'd nack this kind of patch. The test is only redundant in the most trivial sense that the compiler can easily optimise away. The point of the test is to make sure that the range is OK even if the type subsequently becomes signed (to hold a -ve error, for example).
> > 
> > J
> > 
> 
> The check is on the function argument which is unsigned, so checking '<
> 0' doesn't make sense. We should force signed check only if the argument
> is of signed type. In any case, even if irq has been assigned some error
> value, that would be caught by the check irq >= nr_irqs.

Jeremy is (I think) arguing that this check is not redundant because
someone might change the type of the argument to be signed and until
then the compiler can trivially optimise the check away, so what's the
harm in it?

I'm somewhat inclined to agree with him.

Ian.

Re: [Xen-devel] [PATCH 09/14] xen: events: Remove redundant check on unsigned variable

[Tushar Behera]

On 11/19/2012 04:01 PM, Ian Campbell wrote:
> On Mon, 2012-11-19 at 03:52 +0000, Tushar Behera wrote:
>> On 11/16/2012 10:23 PM, Jeremy Fitzhardinge wrote:
>>> To be honest I'd nack this kind of patch. The test is only redundant in the most trivial sense that the compiler can easily optimise away. The point of the test is to make sure that the range is OK even if the type subsequently becomes signed (to hold a -ve error, for example).
>>>
>>> J
>>>
>>
>> The check is on the function argument which is unsigned, so checking '<
>> 0' doesn't make sense. We should force signed check only if the argument
>> is of signed type. In any case, even if irq has been assigned some error
>> value, that would be caught by the check irq >= nr_irqs.
> 
> Jeremy is (I think) arguing that this check is not redundant because
> someone might change the type of the argument to be signed and until
> then the compiler can trivially optimise the check away, so what's the
> harm in it?
> 
> I'm somewhat inclined to agree with him.
> 
> Ian.
> 
Ok, I don't have much argument against this.

-- 
Tushar Behera

Re: [PATCH 08/14] xen: netback: Remove redundant check on unsigned variable

[Tushar Behera]

On 11/16/2012 02:46 PM, Ian Campbell wrote:
> On Fri, 2012-11-16 at 06:50 +0000, Tushar Behera wrote:
>> No need to check whether unsigned variable is less than 0.
>>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: xen-devel@lists.xensource.com
>> CC: netdev@vger.kernel.org
>> Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Thanks.
> 

This patch was not picked up for 3.8-rc1. Any idea, who should pick this up?

>> ---
>>  drivers/net/xen-netback/netback.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>> index aab8677..515e10c 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -190,14 +190,14 @@ static int get_page_ext(struct page *pg,
>>  
>>  	group = ext.e.group - 1;
>>  
>> -	if (group < 0 || group >= xen_netbk_group_nr)
>> +	if (group >= xen_netbk_group_nr)
>>  		return 0;
>>  
>>  	netbk = &xen_netbk[group];
>>  
>>  	idx = ext.e.idx;
>>  
>> -	if ((idx < 0) || (idx >= MAX_PENDING_REQS))
>> +	if (idx >= MAX_PENDING_REQS)
>>  		return 0;
>>  
>>  	if (netbk->mmap_pages[idx] != pg)
> 
> 


-- 
Tushar Behera

Re: [Xen-devel] [PATCH 08/14] xen: netback: Remove redundant check on unsigned variable

[Wei Liu]

On Fri, 2012-12-28 at 05:15 +0000, Tushar Behera wrote:
> On 11/16/2012 02:46 PM, Ian Campbell wrote:
> > On Fri, 2012-11-16 at 06:50 +0000, Tushar Behera wrote:
> >> No need to check whether unsigned variable is less than 0.
> >>
> >> CC: Ian Campbell <ian.campbell@citrix.com>
> >> CC: xen-devel@lists.xensource.com
> >> CC: netdev@vger.kernel.org
> >> Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Thanks.
> > 
> 
> This patch was not picked up for 3.8-rc1. Any idea, who should pick this up?

CC'ing Konrad.

Re: [Xen-devel] [PATCH 08/14] xen: netback: Remove redundant check on unsigned variable

[Konrad Rzeszutek Wilk]

On Fri, Dec 28, 2012 at 10:41:32AM +0000, Wei Liu wrote:
> On Fri, 2012-12-28 at 05:15 +0000, Tushar Behera wrote:
> > On 11/16/2012 02:46 PM, Ian Campbell wrote:
> > > On Fri, 2012-11-16 at 06:50 +0000, Tushar Behera wrote:
> > >> No need to check whether unsigned variable is less than 0.
> > >>
> > >> CC: Ian Campbell <ian.campbell@citrix.com>
> > >> CC: xen-devel@lists.xensource.com
> > >> CC: netdev@vger.kernel.org
> > >> Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
> > > 
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > Thanks.
> > > 
> > 
> > This patch was not picked up for 3.8-rc1. Any idea, who should pick this up?
> 
> CC'ing Konrad.
> 

And CC-ing the network maintainer. David, Ian (who is the sub-maintainer
of xen-netback) has Acked the patch.

I can put this in my queue if you would like.