> > + struct drm_modeset_acquire_ctx ctx; > > int i, r; > > + int ret; > > Could you please tuck this up with i & r? Done! > > - drm_modeset_unlock_all(dev); > > + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); > > You should check ret here Would it be save to return at this point if the lock fails? In other words, can I just add this? --> "if (ret) return ret;" > > + struct drm_modeset_acquire_ctx ctx; > > int r; > > + int ret; > > Same suggestion here, move up with r Done! > > - drm_modeset_unlock_all(dev); > > + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); > > Also check ret here Same question. Would "if (ret) return ret;" be safe here? > > int i; > > + int ret; > > Move up with i Done! > > - drm_modeset_unlock_all(dev); > > + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); > > return 0; I can also "return ret;" instead of "0". What happens when a DEFINE_SHOW_ATTRIBUTE'd function returns non-zero? Is it ok? Or do we want to always return "0" to print whatever we can?