component_unbind_all() splat..

[Russell King - ARM Linux]

On Wed, May 06, 2015 at 10:22:22AM -0400, Rob Clark wrote:
> Hey Russell,
> 
> I just noticed a splat triggered by component_unbind_all() called from
> ->bind()..  any opinions about whether component stuff should handle
> that case, or whether I should rearrange my error cleanup path to not
> component_unbind_all() in this case?

Essentially, if component_bind_all() returned an error, you should not
call component_unbind_all() - component_bind_all() has the effect that
it's either successful and all components have been bound, or when it
fails, no components are bound.

component_unbind_all() assumes that the components were previously
successfully bound.

When I look at the MSM code, I can see why this happens, and it's
basically because of broken error cleanup handling caused by this
commit:

commit 5bf9c0b614542d69fb9a8681a0411715cc3e8ba8
Author: Rob Clark <robdclark at gmail.com>
Date:   Tue Mar 3 15:04:24 2015 -0500

    drm/msm: split out vram initialization

    We'll want to extend this a bit to handle also a reserved-memory
    ("stolen") region, so that drm/msm can take-over bootloader splash
    screen.  First split it out into it's own fxn to reduce noise in
    the following patch.

    Signed-off-by: Rob Clark <robdclark at gmail.com>

Let's look at the clean-up paths:

        priv = kzalloc(sizeof(*priv), GFP_KERNEL);
        if (!priv) {
                dev_err(dev->dev, "failed to allocate private data\n");
                return -ENOMEM;

returns immediately.

        ret = msm_init_vram(dev);
        if (ret)
                goto fail;

calls msm_unload.

        /* Bind all our sub-components: */
        ret = component_bind_all(dev->dev, dev);
        if (ret)
                return ret;

doesn't call msm_unload.

Why would the second not need to run any cleanup if failing at
msm_init_vram() does?  This is totally mad.

I think you need to fix this driver to have a sane error cleanup
implementation...

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