Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd()
@ 2019-10-11  9:02 Andy Shevchenko
  2019-11-07 15:15 ` Andy Shevchenko
  2019-11-07 22:37 ` Keith Busch
  0 siblings, 2 replies; 7+ messages in thread
From: Andy Shevchenko @ 2019-10-11  9:02 UTC (permalink / raw)
  To: Bjorn Helgaas, linux-pci, Stuart Hayes, Alexandru Gagniuc; +Cc: Andy Shevchenko

Infinite timeout loops are hard to read. 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.

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd()
  2019-10-11  9:02 [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd() Andy Shevchenko
@ 2019-11-07 15:15 ` Andy Shevchenko
  2019-11-07 17:21   ` Alex G.
  2019-11-07 22:37 ` Keith Busch
  1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2019-11-07 15:15 UTC (permalink / raw)
  To: Bjorn Helgaas, linux-pci, Stuart Hayes, Alexandru Gagniuc

On Fri, Oct 11, 2019 at 12:02:30PM +0300, Andy Shevchenko wrote:
> Infinite timeout loops are hard to read. 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
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd()
  2019-11-07 15:15 ` Andy Shevchenko
@ 2019-11-07 17:21   ` Alex G.
  2019-11-08 11:08     ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Alex G. @ 2019-11-07 17:21 UTC (permalink / raw)
  To: Andy Shevchenko, Bjorn Helgaas, linux-pci, Stuart Hayes


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
>>
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd()
  2019-10-11  9:02 [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd() Andy Shevchenko
  2019-11-07 15:15 ` Andy Shevchenko
@ 2019-11-07 22:37 ` Keith Busch
  2019-11-08 10:18   ` Andrew Murray
  1 sibling, 1 reply; 7+ messages in thread
From: Keith Busch @ 2019-11-07 22:37 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Bjorn Helgaas, linux-pci, Stuart Hayes, Alexandru Gagniuc

On Fri, Oct 11, 2019 at 12:02:30PM +0300, Andy Shevchenko wrote:
> No functional changes implied.

[snip] 

> -	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 */
>  }

If you really want to ensure no funcitonal change, I think the end of
the loop needs to be 'while (timeout >= 0);'

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd()
  2019-11-07 22:37 ` Keith Busch
@ 2019-11-08 10:18   ` Andrew Murray
  2019-11-08 11:10     ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Murray @ 2019-11-08 10:18 UTC (permalink / raw)
  To: Keith Busch
  Cc: Andy Shevchenko, Bjorn Helgaas, linux-pci, Stuart Hayes,
	Alexandru Gagniuc

On Fri, Nov 08, 2019 at 07:37:26AM +0900, Keith Busch wrote:
> On Fri, Oct 11, 2019 at 12:02:30PM +0300, Andy Shevchenko wrote:
> > No functional changes implied.
> 
> [snip] 
> 
> > -	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 */
> >  }
> 
> If you really want to ensure no funcitonal change, I think the end of
> the loop needs to be 'while (timeout >= 0);'

With this suggested change, you can add:

Reviewed-by: Andrew Murray <andrew.murray@arm.com>

I can't get too excited by coding styles, however I find this more
readable now, due to the fact that the loop is clearly bounded.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd()
  2019-11-07 17:21   ` Alex G.
@ 2019-11-08 11:08     ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2019-11-08 11:08 UTC (permalink / raw)
  To: Alex G.; +Cc: Bjorn Helgaas, linux-pci, Stuart Hayes

On Thu, Nov 07, 2019 at 11:21:12AM -0600, Alex G. wrote:
> 
> 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) {

Invariant conditional inside loop? okay...

> 			complain();
> 			break()
> 		}
> 	} while (what_we_expect_has_not_happened());

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd()
  2019-11-08 10:18   ` Andrew Murray
@ 2019-11-08 11:10     ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2019-11-08 11:10 UTC (permalink / raw)
  To: Andrew Murray
  Cc: Keith Busch, Bjorn Helgaas, linux-pci, Stuart Hayes, Alexandru Gagniuc

On Fri, Nov 08, 2019 at 10:18:16AM +0000, Andrew Murray wrote:
> On Fri, Nov 08, 2019 at 07:37:26AM +0900, Keith Busch wrote:
> > On Fri, Oct 11, 2019 at 12:02:30PM +0300, Andy Shevchenko wrote:
> > > No functional changes implied.
> > 
> > [snip] 
> > 
> > > -	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 */
> > >  }
> > 
> > If you really want to ensure no funcitonal change, I think the end of
> > the loop needs to be 'while (timeout >= 0);'
> 
> With this suggested change, you can add:
> 
> Reviewed-by: Andrew Murray <andrew.murray@arm.com>
> 
> I can't get too excited by coding styles, however I find this more
> readable now, due to the fact that the loop is clearly bounded.

Thank you, Keith and Andrew, I'll submit v2 soon.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-11  9:02 [PATCH v1] PCI: pciehp: Refactor infinite loop in pcie_poll_cmd() Andy Shevchenko
2019-11-07 15:15 ` Andy Shevchenko
2019-11-07 17:21   ` Alex G.
2019-11-08 11:08     ` Andy Shevchenko
2019-11-07 22:37 ` Keith Busch
2019-11-08 10:18   ` Andrew Murray
2019-11-08 11:10     ` Andy Shevchenko

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git