On Thu, Dec 12, 2024 at 03:31:01PM -0300, André Almeida wrote:
> Hi Raag,
>
> Thank you for your patch.
>
> Em 28/11/2024 12:37, Raag Jadav escreveu:
>
> [...]
>
> > +int drm_dev_wedged_event(struct drm_device *dev, unsigned long method)
> > +{
> > + const char *recovery = NULL;
> > + unsigned int len, opt;
> > + /* Event string length up to 28+ characters with available methods */
> > + char event_string[32];
> > + char *envp[] = { event_string, NULL };
> > +
> > + len = scnprintf(event_string, sizeof(event_string), "%s", "WEDGED=");
> > +
> > + for_each_set_bit(opt, &method, BITS_PER_TYPE(method)) {
> > + recovery = drm_get_wedge_recovery(opt);
> > + if (drm_WARN(dev, !recovery, "device wedged, invalid recovery method %u\n", opt))
> > + break;
> > +
> > + len += scnprintf(event_string + len, sizeof(event_string), "%s,", recovery);
> > + }
> > +
> > + if (recovery)
> > + /* Get rid of trailing comma */
> > + event_string[len - 1] = '\0';
> > + else
> > + /* Caller is unsure about recovery, do the best we can at this point. */
> > + snprintf(event_string, sizeof(event_string), "%s", "WEDGED=unknown");
> > +
> > + drm_info(dev, "device wedged, needs recovery\n");
>
> As documented in the commit message "No explicit device recovery is expected
> from the consumer in this case", I think this should be like this:
>
> if (method != DRM_WEDGE_RECOVERY_NONE)
> drm_info(dev, "device wedged, needs recovery\n");
>
> and maybe a note like this:
>
> else
> drm_info(dev, "device reseted, but managed to recover\n");
Or perhaps
drm_info(dev, "device wedged, but recovered through reset\n");
> Either way, this patch is:
>
> Reviewed-by: André Almeida <andrealmeid@xxxxxxxxxx>
Thanks for the review.
Raag
