Re: [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd()

["Alex G." <mr.nuke.me@gmail.com>]

Hi Andy,
On 11/7/19 9:15 AM, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 12:02:30PM +0300, Andy Shevchenko wrote:
>> Infinite timeout loops are hard to read.

Why do you find infinite timeout loops hard to read? I personally find 
that emphasizing the common case to be more redable. An ideal loop for 
me would look like:

	do {
		do_stuff();
		if (timeout) {
			complain();
			break()
		}
	} while (what_we_expect_has_not_happened());

Cheers,
Alex

>> Refactor it to plausible 'do {} while ()'.
>>
>> Note, the supplied timeout can't be negative for current use,
>> though if it's not dividable to 10, we may go below 0,
>> that's why type of the parameter is int. And thus, we may move
>> the check to the loop condition.
>>
>> No functional changes implied.
> 
> Bjorn, any comment on this? It would be nice to have in since contributors are
> unable to know which style to use. This patch makes similar places follow the
> same style.
> 
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   drivers/pci/hotplug/pciehp_hpc.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index 1a522c1c4177..e397c78ca232 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -68,7 +68,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
>>   	struct pci_dev *pdev = ctrl_dev(ctrl);
>>   	u16 slot_status;
>>   
>> -	while (true) {
>> +	do {
>>   		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
>>   		if (slot_status == (u16) ~0) {
>>   			ctrl_info(ctrl, "%s: no response from device\n",
>> @@ -81,11 +81,9 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
>>   						   PCI_EXP_SLTSTA_CC);
>>   			return 1;
>>   		}
>> -		if (timeout < 0)
>> -			break;
>>   		msleep(10);
>>   		timeout -= 10;
>> -	}
>> +	} while (timeout > 0);
>>   	return 0;	/* timeout */
>>   }
>>   
>> -- 
>> 2.23.0
>>
>