Re: [PATCH] ata: pata_hpt37x: fix potential forever loop

[Sergey Shtylyov <s.shtylyov@xxxxxx>]

On 2/1/23 5:59 PM, Sergey Shtylyov wrote:
[...]
>> This code accidentally reuses the "tries" iterator for both the inside
>> and outside loops.  It could result in a forever loop if the "tries"
>> variable gets reset to 0x1000 and never reaches 0x5000.

   Have you actually seen this happening?

>>
>> Fixes: 669a5db411d8 ("[libata] Add a bunch of PATA drivers.")
>> Signed-off-by: Dan Carpenter <error27@xxxxxxxxx>
>> ---
>>  drivers/ata/pata_hpt37x.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/pata_hpt37x.c b/drivers/ata/pata_hpt37x.c
>> index ce3c5eaa7e76..35be9a095b18 100644
>> --- a/drivers/ata/pata_hpt37x.c
>> +++ b/drivers/ata/pata_hpt37x.c
>> @@ -621,14 +621,14 @@ static int hpt37x_calibrate_dpll(struct pci_dev *dev)
>>  {
>>  	u8 reg5b;
>>  	u32 reg5c;
>> -	int tries;
>> +	int tries, tries2;
>>  
>>  	for (tries = 0; tries < 0x5000; tries++) {
>>  		udelay(50);
>>  		pci_read_config_byte(dev, 0x5b, &reg5b);
>>  		if (reg5b & 0x80) {
>>  			/* See if it stays set */
>> -			for (tries = 0; tries < 0x1000; tries++) {
>> +			for (tries2 = 0; tries2 < 0x1000; tries2++) {
>>  				pci_read_config_byte(dev, 0x5b, &reg5b);
>>  				/* Failed ? */
>>  				if ((reg5b & 0x80) == 0)
> 
>    Looks like Alan tried to "fix" the DPLL calibration code imported from drivers/ide/hpt366.c
> and failed at that... :-) The iriginal code has 2 sequential loops (thus 'tries' could be used
> for both). In principle, I agree with a simplistic patch:

   Err, wait! Once we have bit 7 of the PCI register at 0x5b set, we'll never
return to the outer loop... So, I'm ot seeing the bug here...

> Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx>

   So I have to take this back...

MBR, Sergey