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

From: David Laight <david.laight.linux@gmail.com>
To: Daniel Stone <daniel@fooishbar.org>
Cc: "Nicolas Frattaroli" <nicolas.frattaroli@collabora.com>,
	"Sandy Huang" <hjc@rock-chips.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Andy Yan" <andy.yan@rock-chips.com>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"David Airlie" <airlied@gmail.com>,
	"Simona Vetter" <simona@ffwll.ch>,
	"Chaoyi Chen" <chaoyi.chen@rock-chips.com>,
	dri-devel@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	"Daniel Stone" <daniels@collabora.com>,
	kernel@collabora.com
Subject: Re: [PATCH v4 1/8] drm/rockchip: vop2: Switch impossible format conditional to WARN_ON
Date: Mon, 15 Dec 2025 18:16:05 +0000	[thread overview]
Message-ID: <20251215181605.29f6afcc@pumpkin> (raw)
In-Reply-To: <CAPj87rMeJWLm9J=7kMrEvbpzVOMiQc-TThSNE-Gfac7nhUB3bw@mail.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