[PATCH] ath10k: sdio: remove reduntant check in for loop

[PATCH] ath10k: sdio: remove reduntant check in for loop

[Alex Dewar]

The for loop checks whether cur_section is NULL on every iteration, but
we know it can never be NULL as there is another check towards the
bottom of the loop body. Remove this unnecessary check.

Also change i to start at 1, so that we don't need an extra +1 when we
use it.

Addresses-Coverity: 1496984 ("Null pointer dereferences)
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
 drivers/net/wireless/ath/ath10k/sdio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 81ddaafb6721..f31ab2ec2c48 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -2308,7 +2308,7 @@ static int ath10k_sdio_dump_memory_section(struct ath10k *ar,
 
 	count = 0;
 
-	for (i = 0; cur_section; i++) {
+	for (i = 1; ; i++) {
 		section_size = cur_section->end - cur_section->start;
 
 		if (section_size <= 0) {
@@ -2318,7 +2318,7 @@ static int ath10k_sdio_dump_memory_section(struct ath10k *ar,
 			break;
 		}
 
-		if ((i + 1) == mem_region->section_table.size) {
+		if (i == mem_region->section_table.size) {
 			/* last section */
 			next_section = NULL;
 			skip_size = 0;
-- 
2.28.0

Re: [PATCH] ath10k: sdio: remove reduntant check in for loop

[Saeed Mahameed]

On Mon, 2020-09-14 at 20:19 +0100, Alex Dewar wrote:
> The for loop checks whether cur_section is NULL on every iteration,
> but
> we know it can never be NULL as there is another check towards the
> bottom of the loop body. Remove this unnecessary check.
> 
> Also change i to start at 1, so that we don't need an extra +1 wheno
> we
> use it.
> 
> Addresses-Coverity: 1496984 ("Null pointer dereferences)
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
>  drivers/net/wireless/ath/ath10k/sdio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c
> b/drivers/net/wireless/ath/ath10k/sdio.c
> index 81ddaafb6721..f31ab2ec2c48 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -2308,7 +2308,7 @@ static int
> ath10k_sdio_dump_memory_section(struct ath10k *ar,
>  
>  	count = 0;
>  
> -	for (i = 0; cur_section; i++) {
> +	for (i = 1; ; i++) {
'i' is only referenced once inside the loop to check boundary,

the loop is actually iterating over cur_section, so i would make it
clear in the loop statement, e.g.:
Remove the break condition and the cur_section assignment at the end of
the loop and use the loop statement to do it for you

for (; cur_section; cur_section = next_section)


>  		section_size = cur_section->end - cur_section->start;
>  
>  		if (section_size <= 0) {
> @@ -2318,7 +2318,7 @@ static int
> ath10k_sdio_dump_memory_section(struct ath10k *ar,
>  			break;
>  		}
>  
> -		if ((i + 1) == mem_region->section_table.size) {

And for i you can just increment it inline:
if (++i == ...)
    

> +		if (i == mem_region->section_table.size) {
>  			/* last section */
>  			next_section = NULL;
>  			skip_size = 0;

[PATCH v2] ath10k: sdio: remove redundant check in for loop

[Alex Dewar]

The for loop checks whether cur_section is NULL on every iteration, but
we know it can never be NULL as there is another check towards the
bottom of the loop body. Refactor to avoid this unnecessary check.

Also, increment the variable i inline for clarity

Addresses-Coverity: 1496984 ("Null pointer dereferences)
Suggested-by: Saeed Mahameed <saeedm@nvidia.com>
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
v2: refactor in the manner suggested by Saeed

 drivers/net/wireless/ath/ath10k/sdio.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 81ddaafb6721..486886c74e6a 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -2307,8 +2307,8 @@ static int ath10k_sdio_dump_memory_section(struct ath10k *ar,
 	}
 
 	count = 0;
-
-	for (i = 0; cur_section; i++) {
+	i = 0;
+	for (; cur_section; cur_section = next_section) {
 		section_size = cur_section->end - cur_section->start;
 
 		if (section_size <= 0) {
@@ -2318,7 +2318,7 @@ static int ath10k_sdio_dump_memory_section(struct ath10k *ar,
 			break;
 		}
 
-		if ((i + 1) == mem_region->section_table.size) {
+		if (++i == mem_region->section_table.size) {
 			/* last section */
 			next_section = NULL;
 			skip_size = 0;
@@ -2361,12 +2361,6 @@ static int ath10k_sdio_dump_memory_section(struct ath10k *ar,
 		}
 
 		count += skip_size;
-
-		if (!next_section)
-			/* this was the last section */
-			break;
-
-		cur_section = next_section;
 	}
 
 	return count;
-- 
2.28.0

Re: [PATCH v2] ath10k: sdio: remove redundant check in for loop

[Julian Calaby]

Hi Alex,

On Thu, Sep 17, 2020 at 3:09 AM Alex Dewar <alex.dewar90@gmail.com> wrote:
>
> The for loop checks whether cur_section is NULL on every iteration, but
> we know it can never be NULL as there is another check towards the
> bottom of the loop body. Refactor to avoid this unnecessary check.
>
> Also, increment the variable i inline for clarity

Comments below.

> Addresses-Coverity: 1496984 ("Null pointer dereferences)
> Suggested-by: Saeed Mahameed <saeedm@nvidia.com>
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
> v2: refactor in the manner suggested by Saeed
>
>  drivers/net/wireless/ath/ath10k/sdio.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index 81ddaafb6721..486886c74e6a 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -2307,8 +2307,8 @@ static int ath10k_sdio_dump_memory_section(struct ath10k *ar,
>         }
>
>         count = 0;
> -
> -       for (i = 0; cur_section; i++) {
> +       i = 0;
> +       for (; cur_section; cur_section = next_section) {

You can have multiple statements in each section of a for() if you need to, e.g.

for (i = 1; cur_section; cur_section = next_section, i++) {

which means that the increment of i isn't hidden deep in the function body.


That said, this function is a mess. Something (approximately) like
this might be more readable:

prev_end = memregion->start;
for (i = 0; i < mem_region->section_table.size; i++) {
    cur_section = &mem_region->section_table.sections[i];

    // fail if prev_end is greater than cur_section->start - message
from line 2329 and 2294
    // check section size - from line 2315

    skip_size = cur_section->start - prev_end;

    // check buffer size - from line 2339 - needs to account for the
skip size too.
    // fill in the skip size amount - from line 2358 and 2304
    // ath10k_sdio_read_mem - from line 2346

    prev_end = cur_section->end;
}

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH v2] ath10k: sdio: remove redundant check in for loop

[Kalle Valo]

Julian Calaby <julian.calaby@gmail.com> writes:

> On Thu, Sep 17, 2020 at 3:09 AM Alex Dewar <alex.dewar90@gmail.com> wrote:
>>
>> The for loop checks whether cur_section is NULL on every iteration, but
>> we know it can never be NULL as there is another check towards the
>> bottom of the loop body. Refactor to avoid this unnecessary check.
>>
>> Also, increment the variable i inline for clarity
>
> Comments below.
>
>> Addresses-Coverity: 1496984 ("Null pointer dereferences)
>> Suggested-by: Saeed Mahameed <saeedm@nvidia.com>
>> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
>> ---
>> v2: refactor in the manner suggested by Saeed
>>
>>  drivers/net/wireless/ath/ath10k/sdio.c | 12 +++---------
>>  1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
>> index 81ddaafb6721..486886c74e6a 100644
>> --- a/drivers/net/wireless/ath/ath10k/sdio.c
>> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
>> @@ -2307,8 +2307,8 @@ static int ath10k_sdio_dump_memory_section(struct ath10k *ar,
>>         }
>>
>>         count = 0;
>> -
>> -       for (i = 0; cur_section; i++) {
>> +       i = 0;
>> +       for (; cur_section; cur_section = next_section) {
>
> You can have multiple statements in each section of a for() if you need to, e.g.
>
> for (i = 1; cur_section; cur_section = next_section, i++) {
>
> which means that the increment of i isn't hidden deep in the function body.

Yeah, I was thinking the same. But I'll apply this patch anyway, it's
still an improvement.

> That said, this function is a mess. Something (approximately) like
> this might be more readable:
>
> prev_end = memregion->start;
> for (i = 0; i < mem_region->section_table.size; i++) {
>     cur_section = &mem_region->section_table.sections[i];
>
>     // fail if prev_end is greater than cur_section->start - message
> from line 2329 and 2294
>     // check section size - from line 2315
>
>     skip_size = cur_section->start - prev_end;
>
>     // check buffer size - from line 2339 - needs to account for the
> skip size too.
>     // fill in the skip size amount - from line 2358 and 2304
>     // ath10k_sdio_read_mem - from line 2346
>
>     prev_end = cur_section->end;
> }

I agree. Anyone can come up with a patch?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2] ath10k: sdio: remove redundant check in for loop

[Alex Dewar]

> I agree. Anyone can come up with a patch?

Hi Kalle,

I was thinking of having a go at this. Have you applied the v2 of this
patch yet though? I couldn't see it in wireless-drivers-next. I just
don't want to have to rebase the patch if you were going to apply this
v2.

Best,
Alex

> 
> -- 
> https://patchwork.kernel.org/project/linux-wireless/list/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2] ath10k: sdio: remove redundant check in for loop

[Kalle Valo]

Alex Dewar <alex.dewar90@gmail.com> writes:

>> I agree. Anyone can come up with a patch?
>
> Hi Kalle,
>
> I was thinking of having a go at this. Have you applied the v2 of this
> patch yet though? I couldn't see it in wireless-drivers-next. I just
> don't want to have to rebase the patch if you were going to apply this
> v2.

I have not applied this yet. It's in my pending branch but I can easily
drop it. Just let me know what you prefer.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2] ath10k: sdio: remove redundant check in for loop

[Kalle Valo]

Alex Dewar <alex.dewar90@gmail.com> wrote:

> The for loop checks whether cur_section is NULL on every iteration, but
> we know it can never be NULL as there is another check towards the
> bottom of the loop body. Refactor to avoid this unnecessary check.
> 
> Also, increment the variable i inline for clarity
> 
> Addresses-Coverity: 1496984 ("Null pointer dereferences)
> Suggested-by: Saeed Mahameed <saeedm@nvidia.com>
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

dbeb101d28eb ath10k: sdio: remove redundant check in for loop

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20200916165748.20927-1-alex.dewar90@gmail.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] ath10k: sdio: remove reduntant check in for loop

[Alex Dewar]

[snip]
> 'i' is only referenced once inside the loop to check boundary,
>
> the loop is actually iterating over cur_section, so i would make it
> clear in the loop statement, e.g.:
> Remove the break condition and the cur_section assignment at the end of
> the loop and use the loop statement to do it for you
>
> for (; cur_section; cur_section = next_section)
>
>
>>   		section_size = cur_section->end - cur_section->start;
>>   
>>   		if (section_size <= 0) {
>> @@ -2318,7 +2318,7 @@ static int
>> ath10k_sdio_dump_memory_section(struct ath10k *ar,
>>   			break;
>>   		}
>>   
>> -		if ((i + 1) == mem_region->section_table.size) {
> And for i you can just increment it inline:
> if (++i == ...)

Good suggestions! I've sent a v2 with these changes.

>      
>
>> +		if (i == mem_region->section_table.size) {
>>   			/* last section */
>>   			next_section = NULL;
>>   			skip_size = 0;