On Tue, 17 Sep 2024, Raag Jadav <raag.jadav@xxxxxxxxx> wrote:
> Introduce device wedged event, which will notify userspace of wedged
> (hanged/unusable) state of the DRM device through a uevent. This is
> useful especially in cases where the device is no longer operating as
> expected and has become unrecoverable from driver context.
>
> Purpose of this implementation is to provide drivers a way to recover
> through userspace intervention. Different drivers may have different
> ideas of a "wedged device" depending on their hardware implementation,
> and hence the vendor agnostic nature of the event. It is upto the drivers
> to decide when they see the need for recovery and how they want to recover
> from the available methods.
>
> Current implementation defines three recovery methods, out of which,
> drivers can choose to support any one or multiple of them. Preferred
> recovery method will be sent in the uevent environment as WEDGED=<method>.
> Userspace consumers (sysadmin) can define udev rules to parse this event
> and take respective action to recover the device.
>
> Method | Consumer expectations
> -----------|-----------------------------------
> rebind | unbind + rebind driver
> bus-reset | unbind + reset bus device + rebind
> reboot | reboot system
>
> v4: s/drm_dev_wedged/drm_dev_wedged_event
> Use drm_info() (Jani)
> Kernel doc adjustment (Aravind)
> v5: Send recovery method with uevent (Lina)
>
> Signed-off-by: Raag Jadav <raag.jadav@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_drv.c | 37 +++++++++++++++++++++++++++++++++++++
> include/drm/drm_device.h | 24 ++++++++++++++++++++++++
> include/drm/drm_drv.h | 1 +
> 3 files changed, 62 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index ac30b0ec9d93..1e850a9f608d 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -497,6 +497,43 @@ void drm_dev_unplug(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_dev_unplug);
>
> +const char *const wedge_recovery_opts[] = {
> + [DRM_WEDGE_RECOVERY_REBIND] = "rebind",
> + [DRM_WEDGE_RECOVERY_BUS_RESET] = "bus-reset",
> + [DRM_WEDGE_RECOVERY_REBOOT] = "reboot",
> +};
> +
> +/**
> + * drm_dev_wedged_event - generate a device wedged uevent
> + * @dev: DRM device
> + * @method: method to be used for recovery
> + *
> + * This generates a device wedged uevent for the DRM device specified by @dev.
> + * Recovery @method from wedge_recovery_opts[] (if supprted by the device) is
> + * sent in the uevent environment as WEDGED=<method>, on the basis of which,
> + * userspace may take respective action to recover the device.
> + *
> + * Returns: 0 on success, or negative error code otherwise.
> + */
> +int drm_dev_wedged_event(struct drm_device *dev, enum wedge_recovery_method method)
> +{
> + char event_string[32] = "WEDGED=";
> + char *envp[] = { event_string, NULL };
> + bool supported;
> +
> + supported = test_bit(method, &dev->wedge_recovery);
> + if (unlikely(!supported)) {
The unlikely is unnecessary.
> + drm_err(dev, "device wedged, recovery method not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + strcat(event_string, wedge_recovery_opts[method]);
Emphasizing here too, you need bounds checking for
wedge_recovery_opts. And avoid strcat, it's hardly ever the right
choice, as you'll need bounds checking on event_string.
> +
> + drm_info(dev, "device wedged, generating uevent\n");
> + return kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp);
> +}
> +EXPORT_SYMBOL(drm_dev_wedged_event);
> +
> /*
> * DRM internal mount
> * We want to be able to allocate our own "struct address_space" to control
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index c91f87b5242d..e4f32967b5ae 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -40,6 +40,27 @@ enum switch_power_state {
> DRM_SWITCH_POWER_DYNAMIC_OFF = 3,
> };
>
> +/**
> + * enum wedge_recovery_method - Recovery method for wedged device in order
> + * of severity. To be set as bit fields in drm_device.wedge_recovery variable.
> + * Drivers can choose to support any one or multiple of them depending on their
> + * needs.
> + */
> +
> +enum wedge_recovery_method {
> + /** @DRM_WEDGE_RECOVERY_REBIND: unbind + rebind driver */
> + DRM_WEDGE_RECOVERY_REBIND = 0,
I don't see a need to initialize the enumerations, it's automatic.
> +
> + /** @DRM_WEDGE_RECOVERY_BUS_RESET: unbind + reset bus device + rebind */
> + DRM_WEDGE_RECOVERY_BUS_RESET = 1,
> +
> + /** @DRM_WEDGE_RECOVERY_REBOOT: reboot system */
> + DRM_WEDGE_RECOVERY_REBOOT = 2,
> +
> + /** @DRM_WEDGE_RECOVERY_MAX: for bounds checking, do not use */
> + DRM_WEDGE_RECOVERY_MAX = 3,
> +};
> +
> /**
> * struct drm_device - DRM device structure
> *
> @@ -317,6 +338,9 @@ struct drm_device {
> * Root directory for debugfs files.
> */
> struct dentry *debugfs_root;
> +
> + /** @wedge_recovery: Supported recovery methods for wedged device */
> + unsigned long wedge_recovery;
> };
>
> #endif
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 02ea4e3248fd..6e02187f1f6c 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -461,6 +461,7 @@ void drm_put_dev(struct drm_device *dev);
> bool drm_dev_enter(struct drm_device *dev, int *idx);
> void drm_dev_exit(int idx);
> void drm_dev_unplug(struct drm_device *dev);
> +int drm_dev_wedged_event(struct drm_device *dev, enum wedge_recovery_method method);
>
> /**
> * drm_dev_is_unplugged - is a DRM device unplugged
--
Jani Nikula, Intel
