RE: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor specific HW errors

[Shiju Jose]

Hi James,

Thanks for reviewing the patch and the modifications.
 
>-----Original Message-----
>From: James Morse [mailto:james.morse@xxxxxxx]
>Sent: 18 June 2020 19:20
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
>Cc: linux-acpi@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; rjw@xxxxxxxxxxxxx; bp@xxxxxxxxx; lenb@xxxxxxxxxx;
>tony.luck@xxxxxxxxx; dan.carpenter@xxxxxxxxxx;
>zhangliguang@xxxxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx;
>Wangkefeng (OS Kernel Lab) <wangkefeng.wang@xxxxxxxxxx>;
>jroedel@xxxxxxx; yangyicong <yangyicong@xxxxxxxxxx>; Jonathan Cameron
><jonathan.cameron@xxxxxxxxxx>; tanxiaofei <tanxiaofei@xxxxxxxxxx>
>Subject: Re: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor
>specific HW errors
>
>Hi Shiju,
>
>On 15/06/2020 10:53, Shiju Jose wrote:
>> Add support to notify the vendor specific non-fatal HW errors to the
>> drivers for the error recovery.
>
>This doesn't apply cleanly to v5.8-rc1... thanks for waiting for the merge
>window to finish, but please rebase onto the latest and greatest kernel!

V10 was posted based on v5.8-rc1.
>
>I'm glad the notifier chains for stuff that should be built-in has gone.
>(In my opinion, the RAS code should be moving in the direction of having less
>code run between being told of an error, and the handler running. Notifier
>chains for things like memory-errors was moving in the wrong direction!)
>
>
>The Kfifo and pool are adding complexity I don't think you need.
>Please make it clear from the naming this is for vendor records. (what is an
>event?)
>
>The memcpy() for the records is annoying, but eliminating it takes some
>really invasive changes. Lets live with it for now.
Ok.

>
>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index
>> 24c9642e8fc7..854d8115cdfc 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -63,6 +64,11 @@
>>  #define GHES_ESTATUS_CACHES_SIZE	4
>>
>>  #define GHES_ESTATUS_IN_CACHE_MAX_NSEC	10000000000ULL
>> +
>> +#define GHES_EVENT_RING_SIZE	256
>> +#define GHES_GDATA_POOL_MIN_ALLOC_ORDER	3
>> +#define GHES_GDATA_POOL_MIN_SIZE	65536
>
>Huh. Another pool of memory, and we don't know if this will ever be used.
>Can we allocate from ghes_estatus_pool instead?
>
>ghes_estatus_pool is already scaled with the number of error sources
>firmware describes in ghes_estatus_pool_init(), so it should be big enough.
>
>ghes_estatus_pool already has multiple users, estatus_nodes for work
>deferred from NMI come from here, as do ghes_estatus_caches for the low-
>pass filter thing.

Ok.
>
>
>> @@ -122,6 +128,19 @@ static DEFINE_MUTEX(ghes_list_mutex);
>>   */
>>  static DEFINE_SPINLOCK(ghes_notify_lock_irq);
>>
>> +struct ghes_event_entry {
>
>ghes_vendor_record_entry ?
>
>> +	struct acpi_hest_generic_data *gdata;
>> +	int error_severity;
>> +};
>
>> +static DEFINE_KFIFO(ghes_event_ring, struct ghes_event_entry,
>> +		    GHES_EVENT_RING_SIZE);
>> +
>> +static DEFINE_SPINLOCK(ghes_event_ring_lock);
>
>Do you need the FIFO behaviour?
>If you put a work_struct in the struct and schedule_work() that, these would
>run in any order, and it would be less code.
>
>
>> +static struct gen_pool *ghes_gdata_pool; static unsigned long
>> +ghes_gdata_pool_size_request;
>> +
>>  static struct gen_pool *ghes_estatus_pool;  static unsigned long
>> ghes_estatus_pool_size_request;
>
>Please use the existing ghes_estatus_pool.
>
>
>> @@ -188,6 +207,40 @@ int ghes_estatus_pool_init(int num_ghes)
>
>[...]
>
>> +static int ghes_gdata_pool_init(void) {
>> +	unsigned long addr, len;
>> +	int rc;
>> +
>> +	ghes_gdata_pool =
>gen_pool_create(GHES_GDATA_POOL_MIN_ALLOC_ORDER, -1);
>> +	if (!ghes_gdata_pool)
>> +		return -ENOMEM;
>> +
>> +	if (ghes_gdata_pool_size_request < GHES_GDATA_POOL_MIN_SIZE)
>> +		ghes_gdata_pool_size_request =
>GHES_GDATA_POOL_MIN_SIZE;
>> +
>> +	len = ghes_gdata_pool_size_request;
>> +	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
>> +	if (!addr)
>> +		goto err_pool_alloc;
>
>> +	vmalloc_sync_mappings();
>(This isn't needed anymore. See commit 73f693c3a705 ("mm: remove
>vmalloc_sync_(un)mappings()"))
>
>
>> +	rc = gen_pool_add(ghes_gdata_pool, addr, PAGE_ALIGN(len), -1);
>> +	if (rc)
>> +		goto err_pool_add;
>> +
>> +	return 0;
>> +
>> +err_pool_add:
>> +	vfree((void *)addr);
>> +
>> +err_pool_alloc:
>> +	gen_pool_destroy(ghes_gdata_pool);
>> +
>> +	return -ENOMEM;
>> +}
>
>But: using ghes_estatus_pool would avoid this duplication.
>
>
>> @@ -247,6 +300,10 @@ static struct ghes *ghes_new(struct
>acpi_hest_generic *generic)
>>  		goto err_unmap_status_addr;
>>  	}
>>
>> +	ghes_gdata_pool_size_request += generic->records_to_preallocate *
>> +					generic->max_sections_per_record *
>> +					generic->max_raw_data_length;
>> +
>
>Careful, I think ghes_probe() can run in parallel on different CPUs. You can
>certainly unbind/rebind it from user-space.
>
>I recall these max this/that/preallocate stuff are junk values on some
>platform.
>You'd at least need to cap it to sane maximum value.
>
>But: Using ghes_estatus_pool would use ghes_estatus_pool_init()'s sizes,
>which allocates 64K for each error source.
>
>History: https://www.spinics.net/lists/linux-acpi/msg84238.html
>
>
>> @@ -490,6 +547,68 @@ static void ghes_handle_aer(struct
>> acpi_hest_generic_data *gdata)
>
>[...]
>
>> +static void ghes_event_work_func(struct work_struct *work) {
>> +	struct ghes_event_entry entry;
>> +	u32 len;
>> +
>> +	while (kfifo_get(&ghes_event_ring, &entry)) {
>> +		blocking_notifier_call_chain(&ghes_event_notify_list,
>> +					     entry.error_severity,
>> +					     entry.gdata);
>> +		len = acpi_hest_get_record_size(entry.gdata);
>> +		gen_pool_free(ghes_gdata_pool, (unsigned long)entry.gdata,
>len);
>> +	}
>> +}
>> +
>> +static DECLARE_WORK(ghes_event_work, ghes_event_work_func);
>> +
>> +static void ghes_handle_non_standard_event(struct
>acpi_hest_generic_data *gdata,
>> +					   int sev)
>> +{
>> +	u32 len;
>
>> +	struct ghes_event_entry event_entry;
>
>> +	len = acpi_hest_get_record_size(gdata);
>> +	event_entry.gdata = (void *)gen_pool_alloc(ghes_gdata_pool, len);
>> +	if (event_entry.gdata) {
>> +		memcpy(event_entry.gdata, gdata, len);
>> +		event_entry.error_severity = sev;
>> +
>> +		if (kfifo_in_spinlocked(&ghes_event_ring, &event_entry, 1,
>
>... event_entry is on the stack ...
>
Ok.

>
>> +					&ghes_event_ring_lock))
>> +			schedule_work(&ghes_event_work);
>> +		else
>> +			pr_warn(GHES_PFX "ghes event queue full\n");
>> +	}
>> +}
>
>
>I think the kfifo is adding un-needed complexity here.
Ok.
>
>
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h index
>> e3f1cddb4ac8..a3dd82069069 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -50,6 +50,34 @@ enum {
>
[...]
>> +void ghes_unregister_event_notifier(struct notifier_block *nb); #else
>
>Please make it clear from the names these are for vendor events, that the
>kernel would otherwise ignore. It looks like these are for everything. Drivers
>have no business trying to handle the errors that are handled by things like
>memory_failure().
>
>~
>
>I would post a version of this to illustrate, but there are comments on patch 2
>too.
>
>Something like:
>http://www.linux-arm.org/git?p=linux-
>jm.git;a=commitdiff;h=9c6859f3146001cd9f8edfaf965232cb99c7dc42
>
>(caveat emptor: I've only build tested it)

I tested your changes and worked fine.
Should I send this patch along with the updated patch 2?

>
>
>Thanks,
>
>James

Thanks,
Shiju