Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: dri-devel@lists.freedesktop.org, Rob Herring <robh+dt@kernel.org>,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
	Steven Price <steven.price@arm.com>
Subject: Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object
Date: Fri, 29 Nov 2019 21:43:45 +0200	[thread overview]
Message-ID: <20191129194345.GG1208@intel.com> (raw)
In-Reply-To: <20191129202437.0a3c6c25@collabora.com>

On Fri, Nov 29, 2019 at 08:24:37PM +0100, Boris Brezillon wrote:
> On Fri, 29 Nov 2019 19:40:38 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Nov 29, 2019 at 03:19:36PM +0100, Boris Brezillon wrote:
> > > On Fri, 29 Nov 2019 14:13:33 +0000
> > > Steven Price <steven.price@arm.com> wrote:
> > >   
> > > > On 29/11/2019 13:56, Boris Brezillon wrote:  
> > > > > I've spent hours chasing a memory corruption that was caused by
> > > > > insertion of an extra field field before ->base. Let's document the
> > > > > fact that base has to be the first field in panfrost_gem_object.
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>    
> > > > 
> > > > This seems to be a limitation imposed by the gem_create_object()
> > > > callback - e.g. it's assumed that kfree() can be directly called on the
> > > > result. Useful to have the documentation though.  
> > > 
> > > Oh, you're right, I didn't catch that one.  
> > 
> > As a general rule of thumb, never insert anything before a struct member
> > called base. Even more so if it's of the same kind of $thing, but less
> > spezialed. This pattern is so common it's fairly often not documented
> > anywhere.
> 
> I could argue that anything using container_of() in its to_xxx() helper
> is misleading the user into thinking the position of the base field
> doesn't matter (which is exactly what happened here), but I feel like I
> already lost this battle, so I'll simply drop the patch.

I agree that this is a bit annoying. Apart from the "let's pass
the wrong thing to kfree()" issue the other problem is NULL
pointers. If those don't get preserved when going between the
base and derived class the code will turn to bad spaghetti.

Not a problem for the typical to_foo_crtc() since we could
hide an explicit NULL check in there. But the other direction
generally just uses &foo_crtc->base so not going to work.
Shouldn't be an impossible task to wrap all of those as well,
but I guess no one's motivated enough to actually do it.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel