change for to while(1) in 3c515.c
Subsystems (2)
PATCH 1/1 change for to while(1) in 3c515.c
G'day list,
from http://linuxicc.sourceforge.net/
ICC complains about: Pointless comparison of unsigned integer with zero. @ line 1028 of 3c515.c
1007 unsigned long flags, i;
1028 for (i = 20; i >= 0; i--)
1029 if ((inw(ioaddr + EL3_STATUS) & CmdInProgress) == 0)
1030 break;
This makes the 'for' loop effectively 'while (1)' as i >= 0 cant fail.
This seems like a reasonable objective as the loop seems to be waiting for a command to finish.
As it is the current behaviour, and to make the code more readable, the patch below replaces the 'for' with a 'while(1)' and removes the 'unsigned long i', as it is no longer used.
Compile tested.
drivers/net/3c515.c renamed from drivers/net/3c515.c.orig +2 -2
@@ -1004,7 +1004,7 @@ static int corkscrew_start_xmit(struct s/* Calculate the next Tx descriptor entry. */ int entry = vp->cur_tx %TX_RING_SIZE ;struct boom_tx_desc *prev_entry; -unsigned long flags, i; +unsigned long flags;if (vp->tx_full)/* No room to transmit with */ return 1 ;
@@ -1025,7 +1025,7 @@ static int corkscrew_start_xmit(struct sspin_lock_irqsave (&vp->lock, flags);outw (DownStall, ioaddr +EL3_CMD );/* Wait for the stall to complete. */ -for (i =20 ; i >=0 ; i--) +while (1 )if ((inw (ioaddr +EL3_STATUS ) & CmdInProgress) ==0 )break ;if (prev_entry)
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 11 lines · Darren Jenkins\
G'day list,
from http://linuxicc.sourceforge.net/
ICC complains about: Pointless comparison of unsigned integer with zero. @ line 1028 of
3c515.c1007 unsigned long flags, i;
Quoted 6 lines
1028 for (i = 20; i >= 0; i--)
1029 if ((inw(ioaddr + EL3_STATUS)& CmdInProgress) == 0)
1030 break;This makes the 'for' loop effectively 'while (1)' as i >= 0 cant fail.
This seems like a reasonable objective as the loop seems to be waiting for a command to finish.
Yes, but not endless. So s/>=/>/ should be more in spirit of the old solution.
Quoted 2 lines
As it is the current behaviour, and to make the code more readable, the patch below replaces the 'for' with a 'while(1)' and removes the 'unsigned long i', as it is no longer used.
Bernd
Signature · 3 lines
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and Services_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 2 lines · Bernd Petrovitsch
Yes, but not endless. So s/>=/>/ should be more in spirit of the old solution.
I am not sure what the old solution was trying to do, wait for a command to complete, or 20 loops checking ?
It seems more reasonable to me to simply wait for the command to finish as a timeout would/should be checking against a time, not 20 iterations of a loop which could take any random time to complete. Also continuing on at that point before the command has finished my cause problems, I don't know, but the code was waiting for it to finish for a reason.
Darren J.
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 7 lines · Darren Jenkins
Quoted 2 lines · Bernd Petrovitsch
Yes, but not endless. So s/>=/>/ should be more in spirit of the old solution.
I am not sure what the old solution was trying to do, wait for a command to complete, or 20 loops checking ?
It seems to me that way.
Quoted 3 lines
It seems more reasonable to me to simply wait for the command to finish as a timeout would/should be checking against a time, not 20 iterations of a loop which could take any random time to complete.
That may be an even better solution.
But that doesn't solve the problem of a potential infinite loop there with your patch and the impression I got before your patch (even if it is a suboptimal solution).
Quoted 3 lines
Also continuing on at that point before the command has finished my cause problems, I don't know, but the code was waiting for it to finish for a reason.
Yes, we need someone with knowledge about that driver/subsystem/concerned hardware to decide what is better and what is worse.
Bernd
Signature · 3 lines
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and Services_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 8 lines · Bernd Petrovitsch
Quoted 3 lines · Darren Jenkins
I am not sure what the old solution was trying to do, wait for a command to complete, or 20 loops checking ?
It seems to me that way.
Quoted 8 lines
Quoted 3 lines
It seems more reasonable to me to simply wait for the command to finish as a timeout would/should be checking against a time, not 20 iterations of a loop which could take any random time to complete.
That may be an even better solution.
But that doesn't solve the problem of a potential infinite loop there with your patch and the impression I got before your patch (even if it is a suboptimal solution).
While there is the potential of an infinite loop here, I think this will only happen on hardware failure, not sure. Also the old code worked in the same manor so I imagine if this were going to happen, there would of been reports of it, and the driver would have been fixed.(perhaps)
Quoted 7 lines
Quoted 3 lines
Also continuing on at that point before the command has finished my cause problems, I don't know, but the code was waiting for it to finish for a reason.
Yes, we need someone with knowledge about that driver/subsystem/concerned hardware to decide what is better and what is worse.
You are right. Additionally though I think if my patch is applied, and this behaviour is a problem (and I admit I think it might be) someone is more likely to see what is going on and fix the code, without the patch it is not likely someone will see this problem at all.
Darren J.
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors