Re: [PATCH v4 1/8] drm/rockchip: vop2: Switch impossible format conditional to WARN_ON

[David Laight <david.laight.linux@gmail.com>]

On Mon, 15 Dec 2025 17:55:13 +0000
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> 
> On Fri, 12 Dec 2025 at 12:46, Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> > On Thursday, 11 December 2025 23:38:22 Central European Standard Time David Laight wrote:  
> > > Except that all the systems with PANIC_ON_WARN set will panic.
> > > I believe that is somewhere over 90% of systems.  
> >
> > I also like making up statistics. Warning here is the correct move
> > in my opinion because this warning being triggered indicates a bug
> > in the kernel code, and with PANIC_ON_WARN the user explicitly says
> > they would rather panic in such a case than treat it as an abnormal
> > condition that is recoverable.
> >
> > The reason why this condition ever occurring should be treated as an
> > abnormal condition is because the DRM subsystem should guarantee we
> > don't get a framebuffer of a format we didn't explicitly declare
> > support for in the first place. So this condition being hit either
> > means drm_universal_plane_init is broken, or the array of formats
> > that's passed to it is out of sync with the conversion code, which
> > is also a bug. Or someone managed to thoroughly hose DRM's internal
> > kernel-side data structures, which is precisely the kind of thing
> > PANIC_ON_WARN users want to abort for.  
> 
> Yes, that's exactly it. We make all kinds of load-bearing assumptions
> everywhere: that PM code won't pass in a NULL struct device pointer to
> the resume handler, that our driver callbacks won't get called whilst
> the device is runtime-suspended, etc. We could try to handle every
> single one of those with if (clk == NULL) return 0; /* ??? */, or we
> could not.
> 
> If you'd like, we could just delete every one of these checks and
> replace them with comments, explaining what we assume the invariants
> to be, and wait for an OOPS due to dereferencing invalid pointers. But
> the MISRA style of 'handling' every possible impossible case is not
> tractable.

Especially since it is often easier to debug the NULL pointer reference
that the work out why a BUG_ON(!ptr) happened.
(In the former case you should have the contents of all the registers
making it easier to backtrack to where the NULL came from.)

	David

> 
> Cheers,
> Daniel