On Wed, May 6, 2015 at 12:01 PM, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> 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@xxxxxxxxx>
>
> 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
