Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls
@ 2019-06-17  5:25 luanshi
  2019-06-21 15:18 ` Borislav Petkov
  0 siblings, 1 reply; 3+ messages in thread
From: luanshi @ 2019-06-17  5:25 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown
  Cc: linux-acpi, James Morse, Tony Luck, Borislav Petkov

Function __ghes_check_estatus() is called after __ghes_peek_estatus(),
but it is already called in __ghes_peek_estatus(). So we should remove
some needless __ghes_check_estatus() calls.

Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
---
 drivers/acpi/apei/ghes.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 993940d..1041a4d 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -372,10 +372,6 @@ static int ghes_read_estatus(struct ghes *ghes,
 	if (rc)
 		return rc;
 
-	rc = __ghes_check_estatus(ghes, estatus);
-	if (rc)
-		return rc;
-
 	return __ghes_read_estatus(estatus, *buf_paddr, fixmap_idx,
 				   cper_estatus_len(estatus));
 }
@@ -882,12 +878,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
 		return rc;
 	}
 
-	rc = __ghes_check_estatus(ghes, &tmp_header);
-	if (rc) {
-		ghes_clear_estatus(ghes, &tmp_header, buf_paddr, fixmap_idx);
-		return rc;
-	}
-
 	len = cper_estatus_len(&tmp_header);
 	node_len = GHES_ESTATUS_NODE_LEN(len);
 	estatus_node = (void *)gen_pool_alloc(ghes_estatus_pool, node_len);
-- 
1.8.3.1


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

* Re: [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls
  2019-06-17  5:25 [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls luanshi
@ 2019-06-21 15:18 ` Borislav Petkov
  2019-06-24 17:32   ` James Morse
  0 siblings, 1 reply; 3+ messages in thread
From: Borislav Petkov @ 2019-06-21 15:18 UTC (permalink / raw)
  To: luanshi, James Morse; +Cc: Rafael J. Wysocki, Len Brown, linux-acpi, Tony Luck

On Mon, Jun 17, 2019 at 01:25:29PM +0800, luanshi wrote:
> Function __ghes_check_estatus() is called after __ghes_peek_estatus(),
> but it is already called in __ghes_peek_estatus(). So we should remove
> some needless __ghes_check_estatus() calls.
> 
> Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
> ---
>  drivers/acpi/apei/ghes.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 993940d..1041a4d 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -372,10 +372,6 @@ static int ghes_read_estatus(struct ghes *ghes,
>  	if (rc)
>  		return rc;
>  
> -	rc = __ghes_check_estatus(ghes, estatus);
> -	if (rc)
> -		return rc;
> -
>  	return __ghes_read_estatus(estatus, *buf_paddr, fixmap_idx,
>  				   cper_estatus_len(estatus));
>  }
> @@ -882,12 +878,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
>  		return rc;
>  	}
>  
> -	rc = __ghes_check_estatus(ghes, &tmp_header);
> -	if (rc) {
> -		ghes_clear_estatus(ghes, &tmp_header, buf_paddr, fixmap_idx);
> -		return rc;
> -	}
> -
>  	len = cper_estatus_len(&tmp_header);
>  	node_len = GHES_ESTATUS_NODE_LEN(len);
>  	estatus_node = (void *)gen_pool_alloc(ghes_estatus_pool, node_len);
> -- 

Yah, looks correct to me.

James, I think the cleaner thing to do would be for
__ghes_peek_estatus() not to call __ghes_check_estatus() at the end but
to return success and we can keep the two functions - "peek" and "check"
status - separate and always do:

	if (peek)
		return ...;

	if (check)
		return ...;

because this way the checking remains separate in __ghes_check_estatus()
and so is the peeking in __ghes_peek_estatus().

We can merge the two functions because we always do peek and then check
but keeping them separate makes the code clearer.

Am I making some sense...?

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

* Re: [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls
  2019-06-21 15:18 ` Borislav Petkov
@ 2019-06-24 17:32   ` James Morse
  0 siblings, 0 replies; 3+ messages in thread
From: James Morse @ 2019-06-24 17:32 UTC (permalink / raw)
  To: Borislav Petkov, luanshi
  Cc: Rafael J. Wysocki, Len Brown, linux-acpi, Tony Luck

Hi Boris,

On 21/06/2019 16:18, Borislav Petkov wrote:
> On Mon, Jun 17, 2019 at 01:25:29PM +0800, luanshi wrote:
>> Function __ghes_check_estatus() is called after __ghes_peek_estatus(),
>> but it is already called in __ghes_peek_estatus(). So we should remove

>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 993940d..1041a4d 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -372,10 +372,6 @@ static int ghes_read_estatus(struct ghes *ghes,
>>  	if (rc)
>>  		return rc;
>>  
>> -	rc = __ghes_check_estatus(ghes, estatus);
>> -	if (rc)
>> -		return rc;
>> -
>>  	return __ghes_read_estatus(estatus, *buf_paddr, fixmap_idx,
>>  				   cper_estatus_len(estatus));
>>  }
>> @@ -882,12 +878,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
>>  		return rc;
>>  	}
>>  
>> -	rc = __ghes_check_estatus(ghes, &tmp_header);
>> -	if (rc) {
>> -		ghes_clear_estatus(ghes, &tmp_header, buf_paddr, fixmap_idx);
>> -		return rc;
>> -	}
>> -
>>  	len = cper_estatus_len(&tmp_header);
>>  	node_len = GHES_ESTATUS_NODE_LEN(len);
>>  	estatus_node = (void *)gen_pool_alloc(ghes_estatus_pool, node_len);
>> -- 
> 
> Yah, looks correct to me.

Yes, looks like I changed my mind halfway through about whether peek should just get the
values needed to allocate 'enough' memory, or do some validation too.


> James, I think the cleaner thing to do would be for
> __ghes_peek_estatus() not to call __ghes_check_estatus() at the end but
> to return success and we can keep the two functions - "peek" and "check"
> status - separate and always do:
> 
> 	if (peek)
> 		return ...;
> 
> 	if (check)
> 		return ...;
> 
> because this way the checking remains separate in __ghes_check_estatus()
> and so is the peeking in __ghes_peek_estatus().
> 
> We can merge the two functions because we always do peek and then check
> but keeping them separate makes the code clearer.
> 
> Am I making some sense...?

Makes sense to me.


Thanks,

James

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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17  5:25 [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls luanshi
2019-06-21 15:18 ` Borislav Petkov
2019-06-24 17:32   ` James Morse

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/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-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org linux-acpi@archiver.kernel.org
	public-inbox-index linux-acpi


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


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