Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1721478 > unrolled thread

[PATCH v6 3/7] acpi: apei: remove the unused code

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6 3/7] acpi: apei: remove the unused code Dongjiu Geng <gengdongjiu@huawei.com> - 2017-08-28 12:20 +0200
    Re: [PATCH v6 3/7] acpi: apei: remove the unused code James Morse <james.morse@arm.com> - 2017-08-31 20:00 +0200
      Re: [PATCH v6 3/7] acpi: apei: remove the unused code gengdongjiu <gengdongjiu@huawei.com> - 2017-09-04 13:50 +0200
        Re: [PATCH v6 3/7] acpi: apei: remove the unused code James Morse <james.morse@arm.com> - 2017-09-08 20:20 +0200
          Re: [PATCH v6 3/7] acpi: apei: remove the unused code gengdongjiu <gengdongjiu@huawei.com> - 2017-09-11 14:10 +0200
            Re: [PATCH v6 3/7] acpi: apei: remove the unused code James Morse <james.morse@arm.com> - 2017-09-14 14:40 +0200
              Re: [PATCH v6 3/7] acpi: apei: remove the unused code gengdongjiu <gengdongjiu@huawei.com> - 2017-09-14 15:00 +0200

#1721478 — [PATCH v6 3/7] acpi: apei: remove the unused code

In current code logic, the two functions ghes_sea_add() and
ghes_sea_remove() are only called when CONFIG_ACPI_APEI_SEA
is defined. If not, it will return errors in the ghes_probe()
and not contiue. Hence, remove the unnecessary handling when
CONFIG_ACPI_APEI_SEI is not defined.

change since v5:
1. remove the SEI notification type handling, because the SEI is
   asynchronous exception and the address is not accurate. so
   not call memory_failure() to handle it.

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
 drivers/acpi/apei/ghes.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index d661d452b238..c15a08db2c7c 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -813,7 +813,6 @@ static struct notifier_block ghes_notifier_hed = {
 	.notifier_call = ghes_notify_hed,
 };
 
-#ifdef CONFIG_ACPI_APEI_SEA
 static LIST_HEAD(ghes_sea);
 
 /*
@@ -848,19 +847,6 @@ static void ghes_sea_remove(struct ghes *ghes)
 	mutex_unlock(&ghes_list_mutex);
 	synchronize_rcu();
 }
-#else /* CONFIG_ACPI_APEI_SEA */
-static inline void ghes_sea_add(struct ghes *ghes)
-{
-	pr_err(GHES_PFX "ID: %d, trying to add SEA notification which is not supported\n",
-	       ghes->generic->header.source_id);
-}
-
-static inline void ghes_sea_remove(struct ghes *ghes)
-{
-	pr_err(GHES_PFX "ID: %d, trying to remove SEA notification which is not supported\n",
-	       ghes->generic->header.source_id);
-}
-#endif /* CONFIG_ACPI_APEI_SEA */
 
 #ifdef CONFIG_HAVE_ACPI_APEI_NMI
 /*
-- 
2.14.1

[toc] | [next] | [standalone]

#1724463

Hi Dongjiu Geng,

On 28/08/17 11:38, Dongjiu Geng wrote:
> In current code logic, the two functions ghes_sea_add() and
> ghes_sea_remove() are only called when CONFIG_ACPI_APEI_SEA
> is defined. If not, it will return errors in the ghes_probe()
> and not contiue. Hence, remove the unnecessary handling when
> CONFIG_ACPI_APEI_SEI is not defined.

This doesn't match what the patch does. I get this feeling this is needed for
some future patch you haven't included.


> change since v5:
> 1. remove the SEI notification type handling, because the SEI is
>    asynchronous exception and the address is not accurate. so
>    not call memory_failure() to handle it.

Setting NOTIFY_SEI as the GHES entry's notification type means the OS should
check the GHES->ErrorStatusAddress for CPER records when it receives an
SError-Interrupt, as it may be a notification of an error from this error source.

If you aren't handling the notification, why is this is in the HEST at all?
(and if its not: its not firmware-first)


James


> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index d661d452b238..c15a08db2c7c 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -813,7 +813,6 @@ static struct notifier_block ghes_notifier_hed = {
>  	.notifier_call = ghes_notify_hed,
>  };
>
> -#ifdef CONFIG_ACPI_APEI_SEA
>  static LIST_HEAD(ghes_sea);
>
>  /*
> @@ -848,19 +847,6 @@ static void ghes_sea_remove(struct ghes *ghes)
>  	mutex_unlock(&ghes_list_mutex);
>  	synchronize_rcu();
>  }
> -#else /* CONFIG_ACPI_APEI_SEA */
> -static inline void ghes_sea_add(struct ghes *ghes)
> -{
> -	pr_err(GHES_PFX "ID: %d, trying to add SEA notification which is not
supported\n",
> -	       ghes->generic->header.source_id);
> -}
> -
> -static inline void ghes_sea_remove(struct ghes *ghes)
> -{
> -	pr_err(GHES_PFX "ID: %d, trying to remove SEA notification which is not
supported\n",
> -	       ghes->generic->header.source_id);
> -}
> -#endif /* CONFIG_ACPI_APEI_SEA */
>
>  #ifdef CONFIG_HAVE_ACPI_APEI_NMI
>  /*
>

[toc] | [prev] | [next] | [standalone]

#1726017

Hi James,

On 2017/9/1 1:50, James Morse wrote:
> Hi Dongjiu Geng,
> 
> On 28/08/17 11:38, Dongjiu Geng wrote:
>> In current code logic, the two functions ghes_sea_add() and
>> ghes_sea_remove() are only called when CONFIG_ACPI_APEI_SEA
>> is defined. If not, it will return errors in the ghes_probe()
>> and not contiue. Hence, remove the unnecessary handling when
>> CONFIG_ACPI_APEI_SEI is not defined.
> 
> This doesn't match what the patch does. I get this feeling this is needed for
> some future patch you haven't included.

James, let check the code, when the ghes_probe, if the CONFIG_ACPI_APEI_SEA is not defined.
it will return -ENOTSUPP and goto error, and the ghes_sea_add has no chance to execute.
similar, if the probe is failed, it should not have chance to execute ghes_sea_remove.

static int ghes_probe(struct platform_device *ghes_dev)
{
	struct acpi_hest_generic *generic;
	struct ghes *ghes = NULL;

	int rc = -EINVAL;

	generic = *(struct acpi_hest_generic **)ghes_dev->dev.platform_data;
	if (!generic->enabled)
		return -ENODEV;

	switch (generic->notify.type) {
	case ACPI_HEST_NOTIFY_POLLED:
	case ACPI_HEST_NOTIFY_EXTERNAL:
	case ACPI_HEST_NOTIFY_SCI:
	case ACPI_HEST_NOTIFY_GSIV:
	case ACPI_HEST_NOTIFY_GPIO:
		break;

	case ACPI_HEST_NOTIFY_SEA:
		if (!IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
			pr_warn(GHES_PFX "Generic hardware error source: %d notified via SEA is not supported\n",
				generic->header.source_id);
			rc = -ENOTSUPP;
			goto err;
		}
		break;

> 
> 
>> change since v5:
>> 1. remove the SEI notification type handling, because the SEI is
>>    asynchronous exception and the address is not accurate. so
>>    not call memory_failure() to handle it.
> 
> Setting NOTIFY_SEI as the GHES entry's notification type means the OS should
> check the GHES->ErrorStatusAddress for CPER records when it receives an
> SError-Interrupt, as it may be a notification of an error from this error source.
previously I added the NOTIFY_SEI support, but consider the error address in CPER is not accurate and calling memory_failure() may not make sense.
so I remove it.

> 
> If you aren't handling the notification, why is this is in the HEST at all?
> (and if its not: its not firmware-first)
For the SEI notification, may be we can parse and handle the CPER record other than the Error physical address

> 
> 
> James
> 
> 
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index d661d452b238..c15a08db2c7c 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -813,7 +813,6 @@ static struct notifier_block ghes_notifier_hed = {
>>  	.notifier_call = ghes_notify_hed,
>>  };
>>
>> -#ifdef CONFIG_ACPI_APEI_SEA
>>  static LIST_HEAD(ghes_sea);
>>
>>  /*
>> @@ -848,19 +847,6 @@ static void ghes_sea_remove(struct ghes *ghes)
>>  	mutex_unlock(&ghes_list_mutex);
>>  	synchronize_rcu();
>>  }
>> -#else /* CONFIG_ACPI_APEI_SEA */
>> -static inline void ghes_sea_add(struct ghes *ghes)
>> -{
>> -	pr_err(GHES_PFX "ID: %d, trying to add SEA notification which is not
> supported\n",
>> -	       ghes->generic->header.source_id);
>> -}
>> -
>> -static inline void ghes_sea_remove(struct ghes *ghes)
>> -{
>> -	pr_err(GHES_PFX "ID: %d, trying to remove SEA notification which is not
> supported\n",
>> -	       ghes->generic->header.source_id);
>> -}
>> -#endif /* CONFIG_ACPI_APEI_SEA */
>>
>>  #ifdef CONFIG_HAVE_ACPI_APEI_NMI
>>  /*
>>
> 
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1729177

Hi gengdongjiu,

On 04/09/17 12:43, gengdongjiu wrote:
> On 2017/9/1 1:50, James Morse wrote:
>> On 28/08/17 11:38, Dongjiu Geng wrote:
>>> In current code logic, the two functions ghes_sea_add() and
>>> ghes_sea_remove() are only called when CONFIG_ACPI_APEI_SEA
>>> is defined. If not, it will return errors in the ghes_probe()
>>> and not contiue. Hence, remove the unnecessary handling when
>>> CONFIG_ACPI_APEI_SEI is not defined.
>>
>> This doesn't match what the patch does. I get this feeling this is needed for
>> some future patch you haven't included.
> 
> James, let check the code, when the ghes_probe, if the CONFIG_ACPI_APEI_SEA is not defined.
> it will return -ENOTSUPP and goto error, and the ghes_sea_add has no chance to execute.
> similar, if the probe is failed, it should not have chance to execute ghes_sea_remove.

It's the 'unnecessary handling when CONFIG_ACPI_APEI_SEI' in the commit message
that confuses me: this patch doesn't reference that Kconfig symbol. I guess that
sentence needs removing for this v6?

Re-reading without that part of the commit-message:

You're relying on the compiler's dead-code elimination to spot unused static
functions and silently drop them. Great!
(there is the small risk that gcc 3.2[0] can't do this, x86 still has to support
this gcc version)

As this is just clean-up patch can you break it out of this series, it isn't
needed to add support for SEI.

(This series adds support for what should be an APEI notification, but the only
code that touches APEI removes some code from a different notification method.)


>> Setting NOTIFY_SEI as the GHES entry's notification type means the OS should
>> check the GHES->ErrorStatusAddress for CPER records when it receives an
>> SError-Interrupt, as it may be a notification of an error from this error source.

> previously I added the NOTIFY_SEI support,

(Yes, I saw that in v5 and expected this series to add some APEI support code )


> but consider the error address in CPER is not accurate and calling memory_failure() may not make sense.
> so I remove it.

'not accurate'... this is going to be a problem, but lets keep that discussion
on the cover-letter.


>> If you aren't handling the notification, why is this is in the HEST at all?
>> (and if its not: its not firmware-first)

> For the SEI notification, may be we can parse and handle the CPER record other than the Error physical address

Sure, but I only see this cleanup patch in this series, where does APEI learn
about NOTIFY_SEI? As this is nothing will ever touch those CPER records, if
you're using GHESv2 firmware will be prevented from delivering subsequent
notifications.


Thanks,

James

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/README.rst#n251

[toc] | [prev] | [next] | [standalone]

#1730309

James,
   Thanks for the review.

On 2017/9/9 2:17, James Morse wrote:
> Hi gengdongjiu,
> 
> On 04/09/17 12:43, gengdongjiu wrote:
>> On 2017/9/1 1:50, James Morse wrote:
>>> On 28/08/17 11:38, Dongjiu Geng wrote:
>>>> In current code logic, the two functions ghes_sea_add() and
>>>> ghes_sea_remove() are only called when CONFIG_ACPI_APEI_SEA
>>>> is defined. If not, it will return errors in the ghes_probe()
>>>> and not contiue. Hence, remove the unnecessary handling when
>>>> CONFIG_ACPI_APEI_SEI is not defined.
>>>
>>> This doesn't match what the patch does. I get this feeling this is needed for
>>> some future patch you haven't included.
>>
>> James, let check the code, when the ghes_probe, if the CONFIG_ACPI_APEI_SEA is not defined.
>> it will return -ENOTSUPP and goto error, and the ghes_sea_add has no chance to execute.
>> similar, if the probe is failed, it should not have chance to execute ghes_sea_remove.
> 
> It's the 'unnecessary handling when CONFIG_ACPI_APEI_SEI' in the commit message
> that confuses me: this patch doesn't reference that Kconfig symbol. I guess that
> sentence needs removing for this v6?
 thanks for the pointing out, That needs to be removed for v6.

> 
> Re-reading without that part of the commit-message:
> 
> You're relying on the compiler's dead-code elimination to spot unused static
> functions and silently drop them. Great!
> (there is the small risk that gcc 3.2[0] can't do this, x86 still has to support
> this gcc version)
> 
> As this is just clean-up patch can you break it out of this series, it isn't
> needed to add support for SEI.
sure, I will.

> 
> (This series adds support for what should be an APEI notification, but the only
> code that touches APEI removes some code from a different notification method.)

understand.

> 
> 
>>> Setting NOTIFY_SEI as the GHES entry's notification type means the OS should
>>> check the GHES->ErrorStatusAddress for CPER records when it receives an
>>> SError-Interrupt, as it may be a notification of an error from this error source.
> 
>> previously I added the NOTIFY_SEI support,
> 
> (Yes, I saw that in v5 and expected this series to add some APEI support code )
> 
> 
>> but consider the error address in CPER is not accurate and calling memory_failure() may not make sense.
>> so I remove it.
> 
> 'not accurate'... this is going to be a problem, but lets keep that discussion
> on the cover-letter.

Ok.


> 
> 
>>> If you aren't handling the notification, why is this is in the HEST at all?
>>> (and if its not: its not firmware-first)
> 
>> For the SEI notification, may be we can parse and handle the CPER record other than the Error physical address
> 
> Sure, but I only see this cleanup patch in this series, where does APEI learn
> about NOTIFY_SEI? As this is nothing will ever touch those CPER records, if
> you're using GHESv2 firmware will be prevented from delivering subsequent
> notifications.
James, whether it is possible you can review the previous v5 patch which adds the support for NOTIFY_SEI? thanks in advancecIn that patch, I share the SEI notification handling with the SEA notification handling to avoid duplicated code.

https://www.spinics.net/lists/arm-kernel/msg601767.html

> 
> 
> Thanks,
> 
> James
> 
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/README.rst#n251
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1732250

Hi gengdongjiu,

On 11/09/17 13:04, gengdongjiu wrote:
> On 2017/9/9 2:17, James Morse wrote:
>> On 04/09/17 12:43, gengdongjiu wrote:
>>> On 2017/9/1 1:50, James Morse wrote:
>>>> On 28/08/17 11:38, Dongjiu Geng wrote:
>>>> If you aren't handling the notification, why is this is in the HEST at all?
>>>> (and if its not: its not firmware-first)
>>
>>> For the SEI notification, may be we can parse and handle the CPER record other than the Error physical address
>>
>> Sure, but I only see this cleanup patch in this series, where does APEI learn
>> about NOTIFY_SEI? As this is nothing will ever touch those CPER records, if
>> you're using GHESv2 firmware will be prevented from delivering subsequent
>> notifications.

> James, whether it is possible you can review the previous v5 patch which adds the support for

Spreading 'current discussion' over two versions is a problem for anyone trying
to follow this series.

If you post a newer version its normal for people to delete the older versions.
When you post a new version you should be happy that its the latest and greatest.


> NOTIFY_SEI? thanks in advancecIn that patch, I share the SEI notification
handling with the SEA
> notification handling to avoid duplicated code.

You may be able to share some of the code, but I don't think you should share
the list of GHES between notification methods.
This leads to races between the firmware and OS: If CPU-A has received an SEI
firmware would have to avoid generating an SEA on CPU-B as the SEI-handler
running on CPU-A may find and process the second set of CPER records. CPU-B then
gets a spurious notification.

Why is this a problem? KVM needs to know if APEI handled the error, or whether
the Synchronous-External-Abort/SError-Interrupt was due to something else, in
which case we invoke todays default behaviour, which isn't appropriate for a RAS
event.


Thanks,

James

[toc] | [prev] | [next] | [standalone]

#1732262

On 2017/9/14 20:35, James Morse wrote:
>> James, whether it is possible you can review the previous v5 patch which adds the support for
> Spreading 'current discussion' over two versions is a problem for anyone trying
> to follow this series.
> 
> If you post a newer version its normal for people to delete the older versions.
> When you post a new version you should be happy that its the latest and greatest.
> 
>
James, today I paste the new version here, thanks.

https://patchwork.kernel.org/patch/9952495/
https://patchwork.kernel.org/patch/9950955/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web