| Message ID | 20240917040235.197019-3-raag.jadav@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce DRM device wedged event | expand |
On Tue, 17 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > Now that we have device wedged event in place, add wedge_recovery sysfs > attribute which will expose recovery methods supported by the DRM device. > This is useful for userspace consumers in cases where the device supports > multiple recovery methods which can be used as fallbacks. > > $ cat /sys/class/drm/card0/wedge_recovery > rebind > bus-reset > reboot > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > --- > drivers/gpu/drm/drm_sysfs.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index fb3bbb6adcd1..b88cdbfa3b5e 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -36,6 +36,8 @@ > #define to_drm_minor(d) dev_get_drvdata(d) > #define to_drm_connector(d) dev_get_drvdata(d) > > +extern const char *const wedge_recovery_opts[]; Data is not an interface. Please add a function for this. Side note, extern declarations for outside stuff don't belong in .c files anyway. BR, Jani. > + > /** > * DOC: overview > * > @@ -508,6 +510,26 @@ void drm_sysfs_connector_property_event(struct drm_connector *connector, > } > EXPORT_SYMBOL(drm_sysfs_connector_property_event); > > +static ssize_t wedge_recovery_show(struct device *device, > + struct device_attribute *attr, char *buf) > +{ > + struct drm_minor *minor = to_drm_minor(device); > + struct drm_device *dev = minor->dev; > + int opt, count = 0; > + > + for_each_set_bit(opt, &dev->wedge_recovery, DRM_WEDGE_RECOVERY_MAX) > + count += sysfs_emit_at(buf, count, "%s\n", wedge_recovery_opts[opt]); > + > + return count; > +} > +static DEVICE_ATTR_RO(wedge_recovery); > + > +static struct attribute *minor_dev_attrs[] = { > + &dev_attr_wedge_recovery.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(minor_dev); > + > struct device *drm_sysfs_minor_alloc(struct drm_minor *minor) > { > const char *minor_str; > @@ -532,6 +554,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor) > kdev->devt = MKDEV(DRM_MAJOR, minor->index); > kdev->class = drm_class; > kdev->type = &drm_sysfs_device_minor; > + kdev->groups = minor_dev_groups; > } > > kdev->parent = minor->dev->dev;
On Tue, Sep 17, 2024 at 10:49:07AM +0300, Jani Nikula wrote: > On Tue, 17 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > > Now that we have device wedged event in place, add wedge_recovery sysfs > > attribute which will expose recovery methods supported by the DRM device. > > This is useful for userspace consumers in cases where the device supports > > multiple recovery methods which can be used as fallbacks. > > > > $ cat /sys/class/drm/card0/wedge_recovery > > rebind > > bus-reset > > reboot > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > --- > > drivers/gpu/drm/drm_sysfs.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > > index fb3bbb6adcd1..b88cdbfa3b5e 100644 > > --- a/drivers/gpu/drm/drm_sysfs.c > > +++ b/drivers/gpu/drm/drm_sysfs.c > > @@ -36,6 +36,8 @@ > > #define to_drm_minor(d) dev_get_drvdata(d) > > #define to_drm_connector(d) dev_get_drvdata(d) > > > > +extern const char *const wedge_recovery_opts[]; > > Data is not an interface. Please add a function for this. For a single user? > Side note, extern declarations for outside stuff don't belong in .c > files anyway. Sure. Raag
On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > On Tue, Sep 17, 2024 at 10:49:07AM +0300, Jani Nikula wrote: >> On Tue, 17 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: >> > Now that we have device wedged event in place, add wedge_recovery sysfs >> > attribute which will expose recovery methods supported by the DRM device. >> > This is useful for userspace consumers in cases where the device supports >> > multiple recovery methods which can be used as fallbacks. >> > >> > $ cat /sys/class/drm/card0/wedge_recovery >> > rebind >> > bus-reset >> > reboot >> > >> > Signed-off-by: Raag Jadav <raag.jadav@intel.com> >> > --- >> > drivers/gpu/drm/drm_sysfs.c | 23 +++++++++++++++++++++++ >> > 1 file changed, 23 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >> > index fb3bbb6adcd1..b88cdbfa3b5e 100644 >> > --- a/drivers/gpu/drm/drm_sysfs.c >> > +++ b/drivers/gpu/drm/drm_sysfs.c >> > @@ -36,6 +36,8 @@ >> > #define to_drm_minor(d) dev_get_drvdata(d) >> > #define to_drm_connector(d) dev_get_drvdata(d) >> > >> > +extern const char *const wedge_recovery_opts[]; >> >> Data is not an interface. Please add a function for this. > > For a single user? Yes. Well, you kind of have two, and both places need to do bounds checking on indexing the array. You also need to do bounds checking on the string manipulation, you can't just strcat and assume it'll be all right. BR, Jani. > >> Side note, extern declarations for outside stuff don't belong in .c >> files anyway. > > Sure. > > Raag
On Thu, Sep 19, 2024 at 10:38:51AM +0300, Jani Nikula wrote: > On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > > On Tue, Sep 17, 2024 at 10:49:07AM +0300, Jani Nikula wrote: > >> On Tue, 17 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > >> > Now that we have device wedged event in place, add wedge_recovery sysfs > >> > attribute which will expose recovery methods supported by the DRM device. > >> > This is useful for userspace consumers in cases where the device supports > >> > multiple recovery methods which can be used as fallbacks. > >> > > >> > $ cat /sys/class/drm/card0/wedge_recovery > >> > rebind > >> > bus-reset > >> > reboot > >> > > >> > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > >> > --- > >> > drivers/gpu/drm/drm_sysfs.c | 23 +++++++++++++++++++++++ > >> > 1 file changed, 23 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > >> > index fb3bbb6adcd1..b88cdbfa3b5e 100644 > >> > --- a/drivers/gpu/drm/drm_sysfs.c > >> > +++ b/drivers/gpu/drm/drm_sysfs.c > >> > @@ -36,6 +36,8 @@ > >> > #define to_drm_minor(d) dev_get_drvdata(d) > >> > #define to_drm_connector(d) dev_get_drvdata(d) > >> > > >> > +extern const char *const wedge_recovery_opts[]; > >> > >> Data is not an interface. Please add a function for this. > > > > For a single user? > > Yes. > > Well, you kind of have two, and both places need to do bounds checking > on indexing the array. You also need to do bounds checking on the string > manipulation, you can't just strcat and assume it'll be all right. Which would be true if we were to receive an unknown string. Here we sorta know it offhand so we're not gonna shoot in our foot :D Anyway, would you prefer strlcat instead? Raag
On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > On Thu, Sep 19, 2024 at 10:38:51AM +0300, Jani Nikula wrote: >> On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: >> > On Tue, Sep 17, 2024 at 10:49:07AM +0300, Jani Nikula wrote: >> >> On Tue, 17 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: >> >> > >> >> > +extern const char *const wedge_recovery_opts[]; >> >> >> >> Data is not an interface. Please add a function for this. >> > >> > For a single user? >> >> Yes. >> >> Well, you kind of have two, and both places need to do bounds checking >> on indexing the array. You also need to do bounds checking on the string >> manipulation, you can't just strcat and assume it'll be all right. > > Which would be true if we were to receive an unknown string. Here we sorta > know it offhand so we're not gonna shoot in our foot :D The thing about long term code maintenance is that "we know" often turns into "not too obvious" and "probably" somewhere down the line, as features get added and code gets refactored and moved about. Here, it only takes a new, longer string, and failure to manually check that the lengths don't exceed the magic 32 bytes. Just be safe from the start, and you don't have to worry about it later. > Anyway, would you prefer strlcat instead? I think the cleaner option is: char event_string[32]; snprintf(event_string, sizeof(event_string), "WEDGED=%s", wedge_name(method)); which is also what most other code constructing environments for kobject_uevent_env() do. BR, Jani.
On Thu, Sep 19, 2024 at 12:24:09PM +0300, Jani Nikula wrote: > On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > > On Thu, Sep 19, 2024 at 10:38:51AM +0300, Jani Nikula wrote: > >> On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > >> > On Tue, Sep 17, 2024 at 10:49:07AM +0300, Jani Nikula wrote: > >> >> On Tue, 17 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > >> >> > > >> >> > +extern const char *const wedge_recovery_opts[]; > >> >> > >> >> Data is not an interface. Please add a function for this. > >> > > >> > For a single user? > >> > >> Yes. > >> > >> Well, you kind of have two, and both places need to do bounds checking > >> on indexing the array. You also need to do bounds checking on the string > >> manipulation, you can't just strcat and assume it'll be all right. > > > > Which would be true if we were to receive an unknown string. Here we sorta > > know it offhand so we're not gonna shoot in our foot :D > > The thing about long term code maintenance is that "we know" often turns > into "not too obvious" and "probably" somewhere down the line, as > features get added and code gets refactored and moved about. > > Here, it only takes a new, longer string, and failure to manually check > that the lengths don't exceed the magic 32 bytes. Just be safe from the > start, and you don't have to worry about it later. On that note... > > Anyway, would you prefer strlcat instead? > > I think the cleaner option is: > > char event_string[32]; > > snprintf(event_string, sizeof(event_string), "WEDGED=%s", wedge_name(method)); > > which is also what most other code constructing environments for > kobject_uevent_env() do. ...should we use kasprintf instead of hardcoding size? Raag
On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: > On Thu, Sep 19, 2024 at 12:24:09PM +0300, Jani Nikula wrote: >> On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: >> > On Thu, Sep 19, 2024 at 10:38:51AM +0300, Jani Nikula wrote: >> >> On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: >> >> > On Tue, Sep 17, 2024 at 10:49:07AM +0300, Jani Nikula wrote: >> >> >> On Tue, 17 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote: >> >> >> > >> >> >> > +extern const char *const wedge_recovery_opts[]; >> >> >> >> >> >> Data is not an interface. Please add a function for this. >> >> > >> >> > For a single user? >> >> >> >> Yes. >> >> >> >> Well, you kind of have two, and both places need to do bounds checking >> >> on indexing the array. You also need to do bounds checking on the string >> >> manipulation, you can't just strcat and assume it'll be all right. >> > >> > Which would be true if we were to receive an unknown string. Here we sorta >> > know it offhand so we're not gonna shoot in our foot :D >> >> The thing about long term code maintenance is that "we know" often turns >> into "not too obvious" and "probably" somewhere down the line, as >> features get added and code gets refactored and moved about. >> >> Here, it only takes a new, longer string, and failure to manually check >> that the lengths don't exceed the magic 32 bytes. Just be safe from the >> start, and you don't have to worry about it later. > > On that note... > >> > Anyway, would you prefer strlcat instead? >> >> I think the cleaner option is: >> >> char event_string[32]; >> >> snprintf(event_string, sizeof(event_string), "WEDGED=%s", wedge_name(method)); >> >> which is also what most other code constructing environments for >> kobject_uevent_env() do. > > ...should we use kasprintf instead of hardcoding size? You can if you want. > > Raag
On Thu, Sep 19, 2024 at 11:38:50AM +0300, Raag Jadav wrote: > On Thu, Sep 19, 2024 at 10:38:51AM +0300, Jani Nikula wrote: ... > Anyway, would you prefer strlcat instead? FYI: strl*() are subject to remove. They are bad, no-one should really prefer them in the Linux kernel.
On Thu, Sep 19, 2024 at 04:45:28PM +0300, Andy Shevchenko wrote: > On Thu, Sep 19, 2024 at 11:38:50AM +0300, Raag Jadav wrote: > > On Thu, Sep 19, 2024 at 10:38:51AM +0300, Jani Nikula wrote: > > ... > > > Anyway, would you prefer strlcat instead? > > FYI: strl*() are subject to remove. They are bad, no-one should really prefer > them in the Linux kernel. Not showing up on checkpatch (along with a few others from deprecated.rst). Raag
+Cc: Kees On Fri, Sep 20, 2024 at 02:18:19PM +0300, Raag Jadav wrote: > On Thu, Sep 19, 2024 at 04:45:28PM +0300, Andy Shevchenko wrote: > > On Thu, Sep 19, 2024 at 11:38:50AM +0300, Raag Jadav wrote: > > > On Thu, Sep 19, 2024 at 10:38:51AM +0300, Jani Nikula wrote: ... > > > Anyway, would you prefer strlcat instead? > > > > FYI: strl*() are subject to remove. They are bad, no-one should really prefer > > them in the Linux kernel. > > Not showing up on checkpatch (along with a few others from deprecated.rst). Feel free to update! See d26270061ae6 ("string: Remove strlcpy()")
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index fb3bbb6adcd1..b88cdbfa3b5e 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -36,6 +36,8 @@ #define to_drm_minor(d) dev_get_drvdata(d) #define to_drm_connector(d) dev_get_drvdata(d) +extern const char *const wedge_recovery_opts[]; + /** * DOC: overview * @@ -508,6 +510,26 @@ void drm_sysfs_connector_property_event(struct drm_connector *connector, } EXPORT_SYMBOL(drm_sysfs_connector_property_event); +static ssize_t wedge_recovery_show(struct device *device, + struct device_attribute *attr, char *buf) +{ + struct drm_minor *minor = to_drm_minor(device); + struct drm_device *dev = minor->dev; + int opt, count = 0; + + for_each_set_bit(opt, &dev->wedge_recovery, DRM_WEDGE_RECOVERY_MAX) + count += sysfs_emit_at(buf, count, "%s\n", wedge_recovery_opts[opt]); + + return count; +} +static DEVICE_ATTR_RO(wedge_recovery); + +static struct attribute *minor_dev_attrs[] = { + &dev_attr_wedge_recovery.attr, + NULL +}; +ATTRIBUTE_GROUPS(minor_dev); + struct device *drm_sysfs_minor_alloc(struct drm_minor *minor) { const char *minor_str; @@ -532,6 +554,7 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor) kdev->devt = MKDEV(DRM_MAJOR, minor->index); kdev->class = drm_class; kdev->type = &drm_sysfs_device_minor; + kdev->groups = minor_dev_groups; } kdev->parent = minor->dev->dev;
Now that we have device wedged event in place, add wedge_recovery sysfs attribute which will expose recovery methods supported by the DRM device. This is useful for userspace consumers in cases where the device supports multiple recovery methods which can be used as fallbacks. $ cat /sys/class/drm/card0/wedge_recovery rebind bus-reset reboot Signed-off-by: Raag Jadav <raag.jadav@intel.com> --- drivers/gpu/drm/drm_sysfs.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)