by Philipp Zabel

[permalink] [raw]

Subject: Re: [PATCH v16 03/12] drm: imx: imx-hdmi: convert imx-hdmi to drm_bridge mode

Hi Russell,

Am Mittwoch, den 03.12.2014, 16:30 +0000 schrieb Russell King - ARM
Linux:
> On Wed, Dec 03, 2014 at 05:20:15PM +0100, Philipp Zabel wrote:
> > Hi Andy,
> >
> > It would be better if the bind function would not have to care about
> > platform resources, that should be handled in the probe function. I had
> > a patch to move them:
> >
> > http://lists.freedesktop.org/archives/dri-devel/2014-May/059630.html
> >
> > Maybe you could incorporate something like this?
>
> Personally, I hate this idea. Having a two-layered setup means that
> the when the bind() method is called, the state of struct imx_hdmi is
> indeterminant.
>
> If it's called immediately from probe, most of the structure will be
> zeroed, and only those members initialised by the probe function will
> be set to non-zero values.
>
> However, if the HDMI interface has been previously bound, and is
> subsequently re-bound, then the structure will most definitely no
> longer be in a known state on the second bind() call.
>
> This is fragile.
>
> Now, people have tried to tell me that this isn't fragile, but, I now
> have proof that it is as fragile as I fear. The component helper
> doesn't yet have that many users, and already we have one user (okay,
> it's not part of the mainline kernel - it's etnaviv) which contained
> exactly this kind of bug: it expected its private structures to be
> zeroed on the bind() call.
>
> So, I /really/ hate this idea. If you really want to do this, then
> please ensure that the bind() call explicitly zeros the bits of the
> struct which aren't initialised by the probe() call, so we know that
> the driver will always start off with a known initial state.

You are right, no I don't want this. When I initially wrote this patch I
was under the impression that the memory allocated by devm_kzalloc in
bind() wouldn't be freed on unbind(). I remember I stopped pursuing this
patch when I got aware of the devres_open/close/remove_group dance in
the component framework code, but somehow forgot to drop it altogether
locally.

regards
Philipp

2014-12-04 10:10:21

by Russell King - ARM Linux

[permalink] [raw]

Subject: Re: [PATCH v16 03/12] drm: imx: imx-hdmi: convert imx-hdmi to drm_bridge mode

On Thu, Dec 04, 2014 at 09:40:10AM +0100, Philipp Zabel wrote:
> You are right, no I don't want this. When I initially wrote this patch I
> was under the impression that the memory allocated by devm_kzalloc in
> bind() wouldn't be freed on unbind().

Resources claimed inside bind() will be freed in unbind(). Resources
claimed in the driver's probe() will be freed in driver's remove().

They nest, and each is properly dealt with at the appropriate time due
to...

> I remember I stopped pursuing this
> patch when I got aware of the devres_open/close/remove_group dance in
> the component framework code, but somehow forgot to drop it altogether
> locally.

... the use of devres grouping.

So, if you use devm_kzalloc() in the driver's probe() function, then
that memory will be freed after the driver's remove() function is
called. That's fine.

The point I was making is:

probe()
mem = devm_kzalloc();
mem->mmio = ...;
...
bind()
mem->mmio is set
other members of mem are zero
...
unbind()
...
bind()
mem->mmio is set
other members of mem are indeterminant
...
unbind()
...
remove()
mem will be freed automatically

That's where the problem happens - the second time the bind() function
gets called: you might as well not use devm_k*z*alloc() initially,
because having the structure initialised to zero _could_ very well
hide bugs.

When you consider that it's not just the driver code which you have
to audit, but also any code the driver calls (eg, because you've embedded
some subsystem's struct in your driver private data) it quickly becomes
very easy for a bug to creep in here.

If we want to go down the route of having the probe function deal with
resources etc, then I would recommend against using devm_kzalloc() to
allocate the private structure: use devm_kmalloc() instead, which will
leave the memory uninitialised. That means you get the same condition
on each bind(), which means you have to think more about how to ensure
that the initial state of members is dealt with throughout your driver.

Alternatively, separate the struct into two sections: one which contains
everything initialised by the probe() function, and everything else, and
arrange for everything else to get memset() on entry to bind().

--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.