2021-06-21 13:52:18

by Coiby Xu

Subject: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

Since wait_count=30 > 0, the for loop is equivalent to do while
loop. This commit also replaces 100 with UDELAY_DELAY.

Signed-off-by: Coiby Xu <[email protected]>
---
drivers/staging/qlge/qlge_main.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index c5e161595b1f..2d2405be38f5 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
{
unsigned int wait_count = 30;
+ int count;

- do {
+ for (count = 0; count < wait_count; count++) {
if (!qlge_sem_trylock(qdev, sem_mask))
return 0;
- udelay(100);
- } while (--wait_count);
+ udelay(UDELAY_DELAY);
+ }
return -ETIMEDOUT;
}

--
2.32.0

2021-06-22 07:22:05

by Dan Carpenter

Subject: Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
> Since wait_count=30 > 0, the for loop is equivalent to do while
> loop. This commit also replaces 100 with UDELAY_DELAY.
>
> Signed-off-by: Coiby Xu <[email protected]>
> ---
> drivers/staging/qlge/qlge_main.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index c5e161595b1f..2d2405be38f5 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
> int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
> {
> unsigned int wait_count = 30;
> + int count;
>
> - do {
> + for (count = 0; count < wait_count; count++) {
> if (!qlge_sem_trylock(qdev, sem_mask))
> return 0;
> - udelay(100);
> - } while (--wait_count);
> + udelay(UDELAY_DELAY);

This is an interesting way to silence the checkpatch udelay warning. ;)

regards,
dan carpenter

2021-06-24 11:27:08

by Coiby Xu

Subject: Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
>> Since wait_count=30 > 0, the for loop is equivalent to do while
>> loop. This commit also replaces 100 with UDELAY_DELAY.
>>
>> Signed-off-by: Coiby Xu <[email protected]>
>> ---
>> drivers/staging/qlge/qlge_main.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>> index c5e161595b1f..2d2405be38f5 100644
>> --- a/drivers/staging/qlge/qlge_main.c
>> +++ b/drivers/staging/qlge/qlge_main.c
>> @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
>> int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
>> {
>> unsigned int wait_count = 30;
>> + int count;
>>
>> - do {
>> + for (count = 0; count < wait_count; count++) {
>> if (!qlge_sem_trylock(qdev, sem_mask))
>> return 0;
>> - udelay(100);
>> - } while (--wait_count);
>> + udelay(UDELAY_DELAY);
>
>This is an interesting way to silence the checkpatch udelay warning. ;)

I didn't know this could silence the warning :)

>
>regards,
>dan carpenter
>

--
Best regards,
Coiby

2021-06-30 10:59:30

by Joe Perches

Subject: Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
> On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
> > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
> > > Since wait_count=30 > 0, the for loop is equivalent to do while
> > > loop. This commit also replaces 100 with UDELAY_DELAY.
[]
> > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
[]
> > > @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
> > > ?int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
> > > ?{
> > > ? unsigned int wait_count = 30;
> > > + int count;
> > >
> > > - do {
> > > + for (count = 0; count < wait_count; count++) {
> > > ? if (!qlge_sem_trylock(qdev, sem_mask))
> > > ? return 0;
> > > - udelay(100);
> > > - } while (--wait_count);
> > > + udelay(UDELAY_DELAY);
> >
> > This is an interesting way to silence the checkpatch udelay warning. ;)
>
> I didn't know this could silence the warning :)

It also seems odd to have unsigned int wait_count and int count.

Maybe just use 30 in the loop without using wait_count at all.

I also think using UDELAY_DELAY is silly and essentially misleading
as it's also used as an argument value for mdelay

$ git grep -w UDELAY_DELAY
drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
drivers/staging/qlge/qlge_main.c: udelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_main.c: udelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY); /* 100ms */

2021-07-01 00:14:31

by Coiby Xu

Subject: Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

On Wed, Jun 30, 2021 at 03:58:06AM -0700, Joe Perches wrote:
>On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
>> On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
>> > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
>> > > Since wait_count=30 > 0, the for loop is equivalent to do while
>> > > loop. This commit also replaces 100 with UDELAY_DELAY.
>[]
>> > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>[]
>> > > @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
>> > > ?int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
>> > > ?{
>> > > ? unsigned int wait_count = 30;
>> > > + int count;
>> > >
>> > > - do {
>> > > + for (count = 0; count < wait_count; count++) {
>> > > ? if (!qlge_sem_trylock(qdev, sem_mask))
>> > > ? return 0;
>> > > - udelay(100);
>> > > - } while (--wait_count);
>> > > + udelay(UDELAY_DELAY);
>> >
>> > This is an interesting way to silence the checkpatch udelay warning. ;)
>>
>> I didn't know this could silence the warning :)
>
>It also seems odd to have unsigned int wait_count and int count.
>
>Maybe just use 30 in the loop without using wait_count at all.

Thanks for the suggestion. I will apply it to v1.
>
>I also think using UDELAY_DELAY is silly and essentially misleading
>as it's also used as an argument value for mdelay
>
>$ git grep -w UDELAY_DELAY
>drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
>drivers/staging/qlge/qlge_main.c: udelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_main.c: udelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY); /* 100ms */

Thanks for spotting this issue! How about "#define MDELAY_DELAY 100" for
mdelay?

>
>

--
Best regards,
Coiby

2021-07-01 04:38:35

by Joe Perches

Subject: Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

On Thu, 2021-07-01 at 07:33 +0800, Coiby Xu wrote:
> On Wed, Jun 30, 2021 at 03:58:06AM -0700, Joe Perches wrote:
> > On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
> > > On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
> > > > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
> > > > > Since wait_count=30 > 0, the for loop is equivalent to do while
> > > > > loop. This commit also replaces 100 with UDELAY_DELAY.
> > []
> > > > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> > []
> > I also think using UDELAY_DELAY is silly and essentially misleading
> > as it's also used as an argument value for mdelay
> >
> > $ git grep -w UDELAY_DELAY
> > drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
> > drivers/staging/qlge/qlge_main.c: udelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_main.c: udelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY); /* 100ms */
>
> Thanks for spotting this issue! How about "#define MDELAY_DELAY 100" for
> mdelay?

I think the define is pointless and it'd be more readable
to just use 100 in all the cases.

IMO: There really aren't enough cases to justify using defines.

2021-07-03 00:04:19

by Coiby Xu

Subject: Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

On Wed, Jun 30, 2021 at 09:35:31PM -0700, Joe Perches wrote:
>On Thu, 2021-07-01 at 07:33 +0800, Coiby Xu wrote:
>> On Wed, Jun 30, 2021 at 03:58:06AM -0700, Joe Perches wrote:
>> > On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
>> > > On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
>> > > > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
>> > > > > Since wait_count=30 > 0, the for loop is equivalent to do while
>> > > > > loop. This commit also replaces 100 with UDELAY_DELAY.
>> > []
>> > > > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>> > []
>> > I also think using UDELAY_DELAY is silly and essentially misleading
>> > as it's also used as an argument value for mdelay
>> >
>> > $ git grep -w UDELAY_DELAY
>> > drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
>> > drivers/staging/qlge/qlge_main.c: udelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_main.c: udelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_mpi.c: mdelay(UDELAY_DELAY); /* 100ms */
>>
>> Thanks for spotting this issue! How about "#define MDELAY_DELAY 100" for
>> mdelay?
>
>I think the define is pointless and it'd be more readable
>to just use 100 in all the cases.
>
>IMO: There really aren't enough cases to justify using defines.

I thought magic number should be avoided if possible. This case is new
to me. Thanks for the explanation!

>
>

--
Best regards,
Coiby