[Intel-xe] [PATCH v4 07/12] drm/xe: Notify userspace about GSC HW errors.

[Welty, Brian]

On 10/19/2023 9:55 PM, Himal Prasad Ghimiray wrote:
> Send uevent incase of nonfatal errors reported by gsc.
> 
> v2
> - No need to provide tile info in uevent because error is
> accepted only in tile0. (Aravind)
> 
> v3
> - Use pci node to send uevent.
> - Pass RESET_REASON for uevent.
> 
> v4
> - Use XE_RESET_REQUIRED_UEVENT for GSC HW error.
> - Define reason for reset.
> 
> Cc: Aravind Iddamsetty <aravind.iddamsetty at linux.intel.com>
> Cc: Brian Welty <brian.welty at intel.com>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> ---
>   drivers/gpu/drm/xe/xe_device_types.h |  3 +++
>   drivers/gpu/drm/xe/xe_hw_error.c     | 17 +++++++++++++++++
>   drivers/gpu/drm/xe/xe_hw_error.h     |  3 ++-
>   drivers/gpu/drm/xe/xe_irq.c          |  4 ++++
>   include/uapi/drm/xe_drm.h            |  6 ++++++
>   5 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 3b371e0f6168..bc7b545ce37b 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -196,6 +196,9 @@ struct xe_tile {
>   	struct tile_hw_errors {
>   		struct xarray hw_error;
>   	} errors;
> +
> +	/** @gsc_hw_err_work: worker for uevent to report GSC HW errors */
> +	struct work_struct gsc_hw_err_work;
>   };
>   
>   /**
> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
> index 84533ef6a51e..b9d669c150a1 100644
> --- a/drivers/gpu/drm/xe/xe_hw_error.c
> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
> @@ -3,6 +3,8 @@
>    * Copyright © 2023 Intel Corporation
>    */
>   
> +#include <drm/xe_drm.h>
> +
>   #include "xe_hw_error.h"
>   
>   #include "regs/xe_regs.h"
> @@ -378,6 +380,20 @@ xe_gt_hw_error_handler(struct xe_gt *gt, const enum hardware_error hw_err)
>   		xe_gt_hw_error_log_vector_reg(gt, hw_err);
>   }
>   
> +void xe_gsc_hw_error_work(struct work_struct *work)
> +{
> +	struct xe_tile *tile = container_of(work, typeof(*tile), gsc_hw_err_work);
> +	struct pci_dev *pdev = to_pci_dev(tile_to_xe(tile)->drm.dev);
> +	char *csc_hw_error_event[3];
> +
> +	csc_hw_error_event[0] = XE_RESET_REQUIRED_UEVENT;
> +	csc_hw_error_event[1] = XE_RESET_REQUIRED_UEVENT_REASON_GSC;
> +	csc_hw_error_event[2] = NULL;
> +
> +	kobject_uevent_env(&pdev->dev.kobj, KOBJ_CHANGE,
> +			   csc_hw_error_event);

Sorry to drag this on, and I meant to mention before if you combined 
these....
But it looks unsafe now to have both gt_reset() and new worker above 
sending uevents to dev.kobj?
Unlikely, but if they are sending concurrently then it seems that
will corrupt the uevent.  You can double-check, but I don't see 
kobject_uevent_env holding a lock.

I guess you could either introduce a lock.  Or not allow the gt_reset
worker and above worker to run concurrently....

Actually, I think maybe problem already exists in that gt_reset workers
run on a per-GT workqueue.  Two GT resets could fail at same time, and 
each will send a uevent for failed reset?  Ugh.

Well, I'm okay if you fix this separately.

For your patch here:
   Reviewed-by: Brian Welty <brian.welty at intel.com>

-Brian



> +}
> +
>   static void
>   xe_gsc_hw_error_handler(struct xe_tile *tile, const enum hardware_error hw_err)
>   {
> @@ -435,6 +451,7 @@ xe_gsc_hw_error_handler(struct xe_tile *tile, const enum hardware_error hw_err)
>   			drm_err_ratelimited(&tile_to_xe(tile)->drm, HW_ERR
>   					    "Tile0 reported GSC %s %s error, bit[%d] is set\n",
>   					    name, hw_err_str, errbit);
> +			schedule_work(&tile->gsc_hw_err_work);
>   		}
>   		if (indx != XE_HW_ERR_TILE_UNSPEC)
>   			xe_update_hw_error_cnt(&tile_to_xe(tile)->drm,
> diff --git a/drivers/gpu/drm/xe/xe_hw_error.h b/drivers/gpu/drm/xe/xe_hw_error.h
> index e82e7c00a7e1..f8d859c82d72 100644
> --- a/drivers/gpu/drm/xe/xe_hw_error.h
> +++ b/drivers/gpu/drm/xe/xe_hw_error.h
> @@ -7,6 +7,7 @@
>   
>   #include <linux/stddef.h>
>   #include <linux/types.h>
> +#include <linux/workqueue.h>
>   
>   #define XE_RAS_REG_SIZE 32
>   
> @@ -104,5 +105,5 @@ struct xe_tile;
>   void xe_hw_error_irq_handler(struct xe_tile *tile, const u32 master_ctl);
>   void xe_assign_hw_err_regs(struct xe_device *xe);
>   void xe_process_hw_errors(struct xe_device *xe);
> -
> +void xe_gsc_hw_error_work(struct work_struct *work);
>   #endif
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index 8365a4cb0c45..bc0f01a2abc1 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -614,6 +614,10 @@ int xe_irq_install(struct xe_device *xe)
>   	irq_handler_t irq_handler;
>   	int err, irq;
>   
> +	struct xe_tile *tile = xe_device_get_root_tile(xe);
> +
> +	INIT_WORK(&tile->gsc_hw_err_work, xe_gsc_hw_error_work);
> +
>   	irq_handler = xe_irq_handler(xe);
>   	if (!irq_handler) {
>   		drm_err(&xe->drm, "No supported interrupt handler");
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index ae1b1c7528d5..60cc6418d9a7 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -32,6 +32,12 @@ extern "C" {
>    */
>   #define XE_RESET_REQUIRED_UEVENT_REASON_GT    "REASON=GT_RESET_FAILED"
>   
> +/**
> + * XE_RESET_REQUIRED_UEVENT_REASON_GSC - Reason provided to XE_RESET_REQUIRED_UEVENT
> + * incase of GSC HW reporting Uncorrectable errors. The GSC errors are reported only
> + * on TILE0, therefore no additional information is supplied for this reason.
> + */
> +#define XE_RESET_REQUIRED_UEVENT_REASON_GSC    "REASON=GSC_HW_ERROR"
>   
>   /**
>    * struct xe_user_extension - Base class for defining a chain of extensions