| Message ID | 20190812101149.26036-1-shiju.jose@huawei.com |
|---|---|
| Headers | show |
| Series | ACPI: APEI: Add support to notify the vendor specific HW errors | expand |
Hi, On 12/08/2019 11:11, Shiju Jose wrote: > Presently the vendor specific HW errors, in the non-standard format, > are not reported to the vendor drivers for the recovery. > > This patch adds support to notify the vendor specific HW errors to the > registered kernel drivers. > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index a66e00f..374d197 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -477,6 +477,77 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) > #endif > } > > +struct ghes_error_notify { > + struct list_head list;> + struct rcu_head rcu_head; > + guid_t sec_type; /* guid of the error record */ > + error_handle handle; /* error handler function */ ghes_error_handler_t error_handler; ? > + void *data; /* handler driver's private data if any */ > +}; > + > +/* List to store the registered error handling functions */ > +static DEFINE_MUTEX(ghes_error_notify_mutex); > +static LIST_HEAD(ghes_error_notify_list); > +static refcount_t ghes_ref_count; I don't think this refcount is needed. > +/** > + * ghes_error_notify_register - register an error handling function > + * for the hw errors. > + * @sec_type: sec_type of the corresponding CPER to be notified. > + * @handle: pointer to the error handling function. > + * @data: handler driver's private data. > + * > + * return 0 : SUCCESS, non-zero : FAIL > + */ > +int ghes_error_notify_register(guid_t sec_type, error_handle handle, void *data) > +{ > + struct ghes_error_notify *err_notify; > + > + mutex_lock(&ghes_error_notify_mutex); > + err_notify = kzalloc(sizeof(*err_notify), GFP_KERNEL); > + if (!err_notify) > + return -ENOMEM; Leaving the mutex locked. You may as well allocate the memory before taking the lock. > + > + err_notify->handle = handle; > + guid_copy(&err_notify->sec_type, &sec_type); > + err_notify->data = data; > + list_add_rcu(&err_notify->list, &ghes_error_notify_list); > + mutex_unlock(&ghes_error_notify_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(ghes_error_notify_register); Could we leave exporting this to modules until there is a user? > +/** > + * ghes_error_notify_unregister - unregister an error handling function. > + * @sec_type: sec_type of the corresponding CPER. > + * @handle: pointer to the error handling function. > + * > + * return none. > + */ > +void ghes_error_notify_unregister(guid_t sec_type, error_handle handle) Why do we need the handle(r) a second time? Surely there can only be one callback for a given guid. > +{ > + struct ghes_error_notify *err_notify; > + bool found = 0; > + > + mutex_lock(&ghes_error_notify_mutex); > + rcu_read_lock(); > + list_for_each_entry_rcu(err_notify, &ghes_error_notify_list, list) { > + if (guid_equal(&err_notify->sec_type, &sec_type) && > + err_notify->handle == handle) { > + list_del_rcu(&err_notify->list); > + found = 1; > + break; > + } > + } > + rcu_read_unlock(); > + synchronize_rcu(); Is this for the kfree()? Please keep them together so its obvious what its for. Putting it outside the mutex will also save any contended waiter some time. > + mutex_unlock(&ghes_error_notify_mutex); > + if (found) > + kfree(err_notify); > +} > +EXPORT_SYMBOL_GPL(ghes_error_notify_unregister); > + > static void ghes_do_proc(struct ghes *ghes, > const struct acpi_hest_generic_status *estatus) > {> @@ -512,11 +585,29 @@ static void ghes_do_proc(struct ghes *ghes, > > log_arm_hw_error(err); > } else { > - void *err = acpi_hest_get_payload(gdata); > - > - log_non_standard_event(sec_type, fru_id, fru_text, > - sec_sev, err, > - gdata->error_data_length); > + rcu_read_lock(); > + list_for_each_entry_rcu(err_notify, > + &ghes_error_notify_list, list) { > + if (guid_equal(&err_notify->sec_type, > + sec_type)) { > + /* The notification is called in the > + * interrupt context, thus the handler > + * functions should be take care of it. > + */ I read this as "the handler will be called", which doesn't seem to be a useful comment. > + err_notify->handle(gdata, sev, > + err_notify->data); > + is_notify = 1; break; > + } > + } > + rcu_read_unlock(); > + if (!is_notify) { if (!found) Seems more natural. > + void *err = acpi_hest_get_payload(gdata); > + > + log_non_standard_event(sec_type, fru_id, > + fru_text, sec_sev, err, > + gdata->error_data_length); > + } This is tricky to read as its so bunched up. Please pull it out into a separate function. ghes_handle_non_standard_event() ? Because you skip log_non_standard_event(), rasdaemon will no longer see these in user-space. For any kernel consumer of these, we need to know we aren't breaking the user-space component. > } > } > } > @@ -1217,6 +1308,11 @@ static int ghes_probe(struct platform_device *ghes_dev) > > ghes_edac_register(ghes, &ghes_dev->dev); > > + if (!refcount_read(&ghes_ref_count)) > + refcount_set(&ghes_ref_count, 1); What stops this from racing with itself if two ghes platform devices are probed at the same time? If the refcount needs initialising, please do it in ghes_init().... > + else > + refcount_inc(&ghes_ref_count); .. but I don't think this refcount is needed. > /* Handle any pending errors right away */ > spin_lock_irqsave(&ghes_notify_lock_irq, flags); > ghes_proc(ghes); > @@ -1279,6 +1376,17 @@ static int ghes_remove(struct platform_device *ghes_dev) > > ghes_fini(ghes); > > + if (refcount_dec_and_test(&ghes_ref_count) && > + !list_empty(&ghes_error_notify_list)) { > + mutex_lock(&ghes_error_notify_mutex);> + list_for_each_entry_safe(err_notify, tmp, > + &ghes_error_notify_list, list) { > + list_del_rcu(&err_notify->list); > + kfree_rcu(err_notify, rcu_head); > + } > + mutex_unlock(&ghes_error_notify_mutex); > + } ... If someone unregisters, and re-registers all the GHES platform devices, the last one out flushes the vendor-specific error handlers away. Then we re-probe the devices again, but this time the vendor-specific error handlers don't work. As you have an add/remove API for drivers, its up to drivers to cleanup when they are removed. The comings and goings of GHES platform devices isn't relevant. > ghes_edac_unregister(ghes); > > kfree(ghes); > diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h > index e3f1cdd..d480537 100644 > --- a/include/acpi/ghes.h > +++ b/include/acpi/ghes.h > @@ -50,6 +50,53 @@ enum { > GHES_SEV_PANIC = 0x3, > }; > > +/** > + * error_handle - error handling function for the hw errors. Fatal errors get dealt with earlier, so drivers will never see them. | error handling function for non-fatal hardware errors. > + * This handle function is called in the interrupt context. As this overrides ghes's logging of the error, we should mention: | The handler is responsible for any logging of the error. > + * @gdata: acpi_hest_generic_data. > + * @sev: error severity of the entire error event defined in the > + * ACPI spec table generic error status block. > + * @data: handler driver's private data. > + * > + * return : none. > + */ > +typedef void (*error_handle)(struct acpi_hest_generic_data *gdata, int sev, > + void *data); Thanks, James
Hi James, Thanks for the feedback. >-----Original Message----- >From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi- >owner@vger.kernel.org] On Behalf Of James Morse >Sent: 21 August 2019 18:23 >To: Shiju Jose <shiju.jose@huawei.com> >Cc: linux-acpi@vger.kernel.org; linux-edac@vger.kernel.org; linux- >kernel@vger.kernel.org; rjw@rjwysocki.net; lenb@kernel.org; >tony.luck@intel.com; bp@alien8.de; baicar@os.amperecomputing.com; >Linuxarm <linuxarm@huawei.com>; Jonathan Cameron ><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com> >Subject: Re: [PATCH RFC 0/4] ACPI: APEI: Add support to notify the vendor >specific HW errors > >Hi, > >On 12/08/2019 11:11, Shiju Jose wrote: >> Presently kernel does not support reporting the vendor specific HW >> errors, in the non-standard format, to the vendor drivers for the recovery. > >'non standard' here is probably a little jarring to the casual reader. You're >referring to the UEFI spec's "N.2.3 Non-standard Section Body", which refers to >any section type published somewhere other than the UEFI spec. OK. I will change it. > >These still have to have a GUID to identify them, so they still have the same >section header format. Yes. > > >> This patch set add this support and also move the existing handler >> functions for the standard errors to the new callback method. > >Could you give an example of where this would be useful? You're adding an API >with no caller to justify its existence. One such example is handling the local errors occurred in a device controller, such as PCIe. > > >GUIDs should only belong to one driver. UEFI spec's N.2.3 Non-standard Section Body mentioned, "The type (e.g. format) of a non-standard section is identified by the GUID populated in the Section Descriptor's Section Type field." There is a possibility to define common non-standard error section format which will be used for more than one driver if the error data to be reported is in the same format. Then can the same GUID belong to multiple drivers? > >I don't think we should call drivers for something described as a fatal error. >(which is the case with what you have here) The notification is intended only for the recoverable errors as the ghes_proc() call panic for the fatal errors in the early stage. > > >> Also the CCIX RAS patches could be move to the proposed callback method. > >Presumably for any vendor-specific stuff? This information was related to the proposal to replace the number of if(guid_equal(...)) else if(guid_equal(...)) checks in the ghes_do_proc() for the existing UEFI spec defined error sections(such as PCIe, Memory, ARM HW error) by registering the corresponding handler functions to the proposed notification method. The same apply to the CCIX error sections and any other error sections defined by the UEFI spec in the future. > > >Thanks, > >James Thanks, Shiju
Hi James, >-----Original Message----- >From: James Morse [mailto:james.morse@arm.com] >Sent: 21 August 2019 18:24 >To: Shiju Jose <shiju.jose@huawei.com> >Cc: linux-acpi@vger.kernel.org; linux-edac@vger.kernel.org; linux- >kernel@vger.kernel.org; rjw@rjwysocki.net; lenb@kernel.org; >tony.luck@intel.com; bp@alien8.de; baicar@os.amperecomputing.com; >Linuxarm <linuxarm@huawei.com>; Jonathan Cameron ><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com> >Subject: Re: [PATCH RFC 1/4] ACPI: APEI: Add support to notify the vendor >specific HW errors > >Hi, > >On 12/08/2019 11:11, Shiju Jose wrote: >> Presently the vendor specific HW errors, in the non-standard format, >> are not reported to the vendor drivers for the recovery. >> >> This patch adds support to notify the vendor specific HW errors to the >> registered kernel drivers. > >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index >> a66e00f..374d197 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -477,6 +477,77 @@ static void ghes_handle_aer(struct >> acpi_hest_generic_data *gdata) #endif } >> >> +struct ghes_error_notify { >> + struct list_head list;> + struct rcu_head rcu_head; >> + guid_t sec_type; /* guid of the error record */ > >> + error_handle handle; /* error handler function */ > >ghes_error_handler_t error_handler; ? Sure. > > >> + void *data; /* handler driver's private data if any */ }; >> + >> +/* List to store the registered error handling functions */ static >> +DEFINE_MUTEX(ghes_error_notify_mutex); >> +static LIST_HEAD(ghes_error_notify_list); > >> +static refcount_t ghes_ref_count; > >I don't think this refcount is needed. refcount was added to register standard error handlers with this notification method one time when multiple ghes platform devices are probed. > > >> +/** >> + * ghes_error_notify_register - register an error handling function >> + * for the hw errors. >> + * @sec_type: sec_type of the corresponding CPER to be notified. >> + * @handle: pointer to the error handling function. >> + * @data: handler driver's private data. >> + * >> + * return 0 : SUCCESS, non-zero : FAIL */ int >> +ghes_error_notify_register(guid_t sec_type, error_handle handle, void >> +*data) { >> + struct ghes_error_notify *err_notify; >> + >> + mutex_lock(&ghes_error_notify_mutex); >> + err_notify = kzalloc(sizeof(*err_notify), GFP_KERNEL); >> + if (!err_notify) >> + return -ENOMEM; > >Leaving the mutex locked. >You may as well allocate the memory before taking the lock. Good spot. I will fix. > > >> + >> + err_notify->handle = handle; >> + guid_copy(&err_notify->sec_type, &sec_type); >> + err_notify->data = data; >> + list_add_rcu(&err_notify->list, &ghes_error_notify_list); >> + mutex_unlock(&ghes_error_notify_mutex); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(ghes_error_notify_register); > >Could we leave exporting this to modules until there is a user? > > >> +/** >> + * ghes_error_notify_unregister - unregister an error handling function. >> + * @sec_type: sec_type of the corresponding CPER. >> + * @handle: pointer to the error handling function. >> + * >> + * return none. >> + */ >> +void ghes_error_notify_unregister(guid_t sec_type, error_handle >> +handle) > >Why do we need the handle(r) a second time? Surely there can only be one >callback for a given guid. There is a possibility of sharing the guid between drivers if the non-standard error section format is common for more than one devices if the error data to be reported is in the same format. > > >> +{ >> + struct ghes_error_notify *err_notify; >> + bool found = 0; >> + >> + mutex_lock(&ghes_error_notify_mutex); >> + rcu_read_lock(); >> + list_for_each_entry_rcu(err_notify, &ghes_error_notify_list, list) { >> + if (guid_equal(&err_notify->sec_type, &sec_type) && >> + err_notify->handle == handle) { >> + list_del_rcu(&err_notify->list); >> + found = 1; >> + break; >> + } >> + } >> + rcu_read_unlock(); > >> + synchronize_rcu(); > >Is this for the kfree()? Please keep them together so its obvious what its for. >Putting it outside the mutex will also save any contended waiter some time. Yes. I will move synchronize_rcu () just before kfree. > > >> + mutex_unlock(&ghes_error_notify_mutex); >> + if (found) >> + kfree(err_notify); >> +} >> +EXPORT_SYMBOL_GPL(ghes_error_notify_unregister); >> + > >> static void ghes_do_proc(struct ghes *ghes, >> const struct acpi_hest_generic_status *estatus) {> >@@ -512,11 >> +585,29 @@ static void ghes_do_proc(struct ghes *ghes, >> >> log_arm_hw_error(err); >> } else { >> - void *err = acpi_hest_get_payload(gdata); >> - >> - log_non_standard_event(sec_type, fru_id, fru_text, >> - sec_sev, err, >> - gdata->error_data_length); > >> + rcu_read_lock(); >> + list_for_each_entry_rcu(err_notify, >> + &ghes_error_notify_list, list) { >> + if (guid_equal(&err_notify->sec_type, >> + sec_type)) { > >> + /* The notification is called in the >> + * interrupt context, thus the handler >> + * functions should be take care of it. >> + */ > >I read this as "the handler will be called", which doesn't seem to be a useful >comment. Ok. I will correct the comment. > > >> + err_notify->handle(gdata, sev, >> + err_notify->data); >> + is_notify = 1; > > break; > >> + } >> + } >> + rcu_read_unlock(); > >> + if (!is_notify) { > >if (!found) Seems more natural. Ok. I will change to "is_notify" to "found". > > >> + void *err = acpi_hest_get_payload(gdata); >> + >> + log_non_standard_event(sec_type, fru_id, >> + fru_text, sec_sev, err, >> + gdata->error_data_length); >> + } > >This is tricky to read as its so bunched up. Please pull it out into a separate >function. >ghes_handle_non_standard_event() ? Ok. I will add to new ghes_handle_non_standard_event() function. > > >Because you skip log_non_standard_event(), rasdaemon will no longer see >these in user-space. For any kernel consumer of these, we need to know we >aren't breaking the user-space component. > > >> } >> } >> } >> @@ -1217,6 +1308,11 @@ static int ghes_probe(struct platform_device >> *ghes_dev) >> >> ghes_edac_register(ghes, &ghes_dev->dev); >> >> + if (!refcount_read(&ghes_ref_count)) >> + refcount_set(&ghes_ref_count, 1); > >What stops this from racing with itself if two ghes platform devices are probed >at the same time? yes. It is an issue. > >If the refcount needs initialising, please do it in ghes_init().... refcount was added to register the standard error handlers to the notification method only for the first time when the ghes device probed multiple times. I will check is it possible to avoid using refcount by moving the above registration of standard error handlers to the ghes_init(). > >> + else >> + refcount_inc(&ghes_ref_count); > >.. but I don't think this refcount is needed. > > >> /* Handle any pending errors right away */ >> spin_lock_irqsave(&ghes_notify_lock_irq, flags); >> ghes_proc(ghes); > >> @@ -1279,6 +1376,17 @@ static int ghes_remove(struct platform_device >> *ghes_dev) >> >> ghes_fini(ghes); >> >> + if (refcount_dec_and_test(&ghes_ref_count) && >> + !list_empty(&ghes_error_notify_list)) { >> + mutex_lock(&ghes_error_notify_mutex);> + > list_for_each_entry_safe(err_notify, tmp, >> + &ghes_error_notify_list, list) { >> + list_del_rcu(&err_notify->list); >> + kfree_rcu(err_notify, rcu_head); >> + } >> + mutex_unlock(&ghes_error_notify_mutex); >> + } > >... If someone unregisters, and re-registers all the GHES platform devices, the >last one out flushes the vendor-specific error handlers away. Then we re-probe >the devices again, but this time the vendor-specific error handlers don't work. > >As you have an add/remove API for drivers, its up to drivers to cleanup when >they are removed. The comings and goings of GHES platform devices isn't >relevant. Ok. Got it. I will either keep the unregister for the standard error handlers only if the standard error handlers can be part of the notification method or remove completely if the registration can be done in the ghes_init(). > > >> ghes_edac_unregister(ghes); >> >> kfree(ghes); >> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h index >> e3f1cdd..d480537 100644 >> --- a/include/acpi/ghes.h >> +++ b/include/acpi/ghes.h >> @@ -50,6 +50,53 @@ enum { >> GHES_SEV_PANIC = 0x3, >> }; >> >> +/** >> + * error_handle - error handling function for the hw errors. > >Fatal errors get dealt with earlier, so drivers will never see them. >| error handling function for non-fatal hardware errors. Ok. I will change the comment as recoverable HW errors. > > >> + * This handle function is called in the interrupt context. > >As this overrides ghes's logging of the error, we should mention: >| The handler is responsible for any logging of the error. Ok. I will add in the comment. > > >> + * @gdata: acpi_hest_generic_data. >> + * @sev: error severity of the entire error event defined in the >> + * ACPI spec table generic error status block. >> + * @data: handler driver's private data. >> + * >> + * return : none. >> + */ >> +typedef void (*error_handle)(struct acpi_hest_generic_data *gdata, int sev, >> + void *data); > > >Thanks, > >James Thanks, Shiju
Presently kernel does not support reporting the vendor specific HW errors, in the non-standard format, to the vendor drivers for the recovery. This patch set add this support and also move the existing handler functions for the standard errors to the new callback method. Also the CCIX RAS patches could be move to the proposed callback method. https://www.spinics.net/lists/linux-edac/msg10508.html https://patchwork.kernel.org/patch/10979491/ Shiju Jose (4): ACPI: APEI: Add support to notify the vendor specific HW errors ACPI: APEI: Add ghes_handle_memory_failure to the new notification method ACPI: APEI: Add ghes_handle_aer to the new notification method ACPI: APEI: Add log_arm_hw_error to the new notification method drivers/acpi/apei/ghes.c | 170 +++++++++++++++++++++++++++++++++++++++++------ drivers/ras/ras.c | 5 +- include/acpi/ghes.h | 47 +++++++++++++ include/linux/ras.h | 7 +- 4 files changed, 205 insertions(+), 24 deletions(-) -- 1.9.1