Re: [PATCH v2] drm/fbdev-shmem: don't warn on a transient dirty callback failure

[Jiahao Xu]

Hi Thomas,

thanks for the quick look.

> Cosmetic? Failing to allocate memory seems like a significant problem to me.

You are right, and "cosmetic" was a poor choice of words on my side.  The
allocation failure is significant -- what I meant is that the patch does not
downgrade it: the error is still returned to the caller unchanged.

What I am after is the drm_WARN_ONCE() call.  It fires for a failure that
userspace can trigger by writing to the fbdev fd under memory pressure, and
it produces a splat that panics with panic_on_warn and that syzbot turns into
a kernel bug report (extid=369ee6a6e9d0bbf14b37).  The same failure is
already reported to userspace through the return value, so the WARN does not
add any information that would identify a driver problem.

For the record, v1 got this wrong in a way that is worth keeping in mind:
with

	if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", ret))
		return ret;

the propagation is gated on the value of the WARN macro, which yields 0 when
the warning is disabled, i.e. -ENOMEM was silently dropped.  v2 keeps the
propagation unconditional and only skips the splat for -ENOMEM:

	drm_WARN_ONCE(dev, ret && ret != -ENOMEM,
		      "Dirty helper failed: ret=%d\n", ret);
	if (ret)
		return ret;

If you would rather keep some diagnostics here, I am glad to resend with
either of these instead:

  a) as-is: keep the warning for failures other than -ENOMEM, propagate all;
  b) replace the WARN with drm_dbg_ratelimited()/drm_err_ratelimited();
  c) or drop the patch, if you consider the WARN appropriate in this path.

Which one would you prefer?

Thanks,
Jiahao