component_unbind_all() splat..

[Rob Clark]

On Wed, May 6, 2015 at 12:01 PM, Russell King - ARM Linux
<linux at arm.linux.org.uk> wrote:
> 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:

Right, what to do to fix it in msm is clear enough.. what I was
(trying to) get at was, since error paths are perpetually undertested,
would it make more sense to handle component_unbind_all() called from
->bind().. (although I also didn't go audit the other component users
yet to see if any might have the same issue)

BR,
-R

> 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.