Re: [PATCH 1/2] Staging: vme: remove unreachable code

Re: [PATCH 1/2] Staging: vme: remove unreachable code

[Martyn Welch]

On 22/02/11 19:36, Manohar Vanga wrote:
> Remove unreachable code from vme_register_bridge
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>

Yeah - that's there from development. If the function needed to be extended,
that's the next part of the error path.

Not sure how leaving lines like this are viewed in the kernel code, I'm happy
for this to be removed if it's not considered good practice. In that case:

Acked-by: Martyn Welch <martyn.welch@ge.com>

Martyn

> ---
>  drivers/staging/vme/vme.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index d9fc864..88bf455 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -1363,7 +1363,6 @@ int vme_register_bridge(struct vme_bridge *bridge)
>  
>  	return retval;
>  
> -	i = VME_SLOTS_MAX;
>  err_reg:
>  	while (i > -1) {
>  		dev = &bridge->dev[i];


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

Re: [PATCH 1/2] Staging: vme: remove unreachable code

[Dan Carpenter]

On Wed, Feb 23, 2011 at 09:19:06AM +0000, Martyn Welch wrote:
> On 22/02/11 19:36, Manohar Vanga wrote:
> > Remove unreachable code from vme_register_bridge
> > 
> > Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>

Please always CC the list.  It's going to be Greg to commit this
code so CC Greg as well.

./scripts/get_maintainer.pl patch.diff

(Although you should generally remove Tejun from the CC list.)

> 
> Yeah - that's there from development. If the function needed to be extended,
> that's the next part of the error path.

Don't do that.  Kernel style is austere.  Anything that isn't needed
here and now should be removed.

> > --- a/drivers/staging/vme/vme.c
> > +++ b/drivers/staging/vme/vme.c
> > @@ -1363,7 +1363,6 @@ int vme_register_bridge(struct vme_bridge *bridge)
> >  
> >  	return retval;
> >  
> > -	i = VME_SLOTS_MAX;
> >  err_reg:
> >  	while (i > -1) {
> >  		dev = &bridge->dev[i];

There are two problems with this loop.  1) It unregisters the device
which failed to register.  2) It is a forever loop.

It should be:
	while (--i >= 0) {
		dev = &bridge->dev[i];

Can you fix that up as well and resend.

regards,
dan carpenter

Re: [PATCH 1/2] Staging: vme: remove unreachable code

[Martyn Welch]

On 23/02/11 12:17, Dan Carpenter wrote:
> On Wed, Feb 23, 2011 at 09:19:06AM +0000, Martyn Welch wrote:
>> On 22/02/11 19:36, Manohar Vanga wrote:
>>> Remove unreachable code from vme_register_bridge
>>>
>>> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
> 
> Please always CC the list.  It's going to be Greg to commit this
> code so CC Greg as well.
> 
> ./scripts/get_maintainer.pl patch.diff
> 
> (Although you should generally remove Tejun from the CC list.)
> 
>>
>> Yeah - that's there from development. If the function needed to be extended,
>> that's the next part of the error path.
> 
> Don't do that.  Kernel style is austere.  Anything that isn't needed
> here and now should be removed.
> 
>>> --- a/drivers/staging/vme/vme.c
>>> +++ b/drivers/staging/vme/vme.c
>>> @@ -1363,7 +1363,6 @@ int vme_register_bridge(struct vme_bridge *bridge)
>>>  
>>>  	return retval;
>>>  
>>> -	i = VME_SLOTS_MAX;
>>>  err_reg:
>>>  	while (i > -1) {
>>>  		dev = &bridge->dev[i];
> 
> There are two problems with this loop.  1) It unregisters the device
> which failed to register.  2) It is a forever loop.
> 

Even if "i" is a signed int?

> It should be:
> 	while (--i >= 0) {
> 		dev = &bridge->dev[i];
> 
> Can you fix that up as well and resend.
> 
> regards,
> dan carpenter


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

Re: [PATCH 1/2] Staging: vme: remove unreachable code

[Manohar Vanga]

> > There are two problems with this loop.  1) It unregisters the device
> > which failed to register.  2) It is a forever loop.
> > 
> 
> Even if "i" is a signed int?

The infinite loop is because "i" is not decremented anywhere in the loop. Not
because of signedness.

Thanks,
Manohar Vanga

Re: [PATCH 1/2] Staging: vme: remove unreachable code

[Martyn Welch]

On 23/02/11 15:16, Manohar Vanga wrote:
>>> There are two problems with this loop.  1) It unregisters the device
>>> which failed to register.  2) It is a forever loop.
>>>
>>
>> Even if "i" is a signed int?
> 
> The infinite loop is because "i" is not decremented anywhere in the loop. Not
> because of signedness.
> 

Ah, yeah, that would help.

Martyn

> Thanks,
> Manohar Vanga


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189

Re: [PATCH 2/2] Staging: vme: fix potential infinite loop

[Manohar Vanga]

On Tue, Feb 22, 2011 at 08:36:16PM +0100, Manohar Vanga wrote:
> Fix a bug in vme_register_bridge that resulted in an infinite loop if
> device_register failed.
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
> ---
>  drivers/staging/vme/vme.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index 88bf455..7eb4568 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -1364,9 +1364,14 @@ int vme_register_bridge(struct vme_bridge *bridge)
>  	return retval;
>  
>  err_reg:
> +	/*
> +	 * Ignore the failed driver and unregister the rest
> +	 */
> +	i--

Ugh. Missing semicolon here. I'll resend the corrected patch.

Thanks,
Manohar Vanga

[PATCH 0/2] staging: vme: fix issues in vme_register_bridge

[Manohar Vanga]

This patchset fixes two issues I found in vme_register_bridge. 
The first gets rid of some unreachable code and the second one fixes 
a bug that could potentially lead to an infinite loop in the event
that device_register fails.

Please skip the previous patchset I sent and reacknowledge these.
Apologies for the noise pollution.

Thanks,
Manohar Vanga

Manohar Vanga (2):
  staging: vme: remove unreachable code
  staging: vme: fix loop condition

 drivers/staging/vme/vme.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

[PATCH 1/2] staging: vme: remove unreachable code

[Manohar Vanga]

Removed some unreachable code from vme_register_bridge

Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 drivers/staging/vme/vme.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
index d9fc864..88bf455 100644
--- a/drivers/staging/vme/vme.c
+++ b/drivers/staging/vme/vme.c
@@ -1363,7 +1363,6 @@ int vme_register_bridge(struct vme_bridge *bridge)
 
 	return retval;
 
-	i = VME_SLOTS_MAX;
 err_reg:
 	while (i > -1) {
 		dev = &bridge->dev[i];
-- 
1.7.1

Re: [PATCH 1/2] staging: vme: remove unreachable code

[Dan Carpenter]

On Wed, Feb 23, 2011 at 02:25:27PM +0100, Manohar Vanga wrote:
> Removed some unreachable code from vme_register_bridge
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>

Acked-by: Dan Carpenter <error27@gmail.com>

Re: [PATCH 1/2] staging: vme: remove unreachable code

[Martyn Welch]

On 23/02/11 13:25, Manohar Vanga wrote:
> Removed some unreachable code from vme_register_bridge
> 
> Signed-off-by: Manohar Vanga <manohar.vanga@cern.ch>

Acked-by: Martyn Welch <martyn.welch@ge.com>

> ---
>  drivers/staging/vme/vme.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/vme/vme.c b/drivers/staging/vme/vme.c
> index d9fc864..88bf455 100644
> --- a/drivers/staging/vme/vme.c
> +++ b/drivers/staging/vme/vme.c
> @@ -1363,7 +1363,6 @@ int vme_register_bridge(struct vme_bridge *bridge)
>  
>  	return retval;
>  
> -	i = VME_SLOTS_MAX;
>  err_reg:
>  	while (i > -1) {
>  		dev = &bridge->dev[i];


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)127322748                          | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189