On 22/02/11 19:36, Manohar Vanga wrote:
> Remove unreachable code from vme_register_bridge
>
> Signed-off-by: Manohar Vanga <[email protected]>
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 <[email protected]>
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 [email protected] | M2 3AB VAT:GB 927559189
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 <[email protected]>
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
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 <[email protected]>
>
> 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 [email protected] | M2 3AB VAT:GB 927559189
> > 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
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 [email protected] | M2 3AB VAT:GB 927559189