component_unbind_all() splat..

component_unbind_all() splat..

[Rob Clark]

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?

[    2.329829] msm 1a00000.qcom,mdss_mdp: failed to allocate VRAM
[    2.334027] ------------[ cut here ]------------
[    2.339754] WARNING: CPU: 1 PID: 1 at
../drivers/base/component.c:356 component_unbind.isra.2+0x5c/0x68()
[    2.344536] Modules linked in:
[    2.356918] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.0.0-dirty #5
[    2.356996] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
[    2.363420] Call trace:
[    2.370110] [<ffffffc000089c78>] dump_backtrace+0x0/0x118
[    2.372276] [<ffffffc000089da0>] show_stack+0x10/0x20
[    2.377840] [<ffffffc0006686bc>] dump_stack+0x84/0xc4
[    2.382865] [<ffffffc0000b1afc>] warn_slowpath_common+0x94/0xd0
[    2.387910] [<ffffffc0000b1bdc>] warn_slowpath_null+0x14/0x20
[    2.393631] [<ffffffc00042a450>] component_unbind.isra.2+0x58/0x68
[    2.399543] [<ffffffc00042a4e4>] component_unbind_all+0x84/0xc0
[    2.405619] [<ffffffc000419c70>] msm_unload+0x118/0x1a0
[    2.411425] [<ffffffc00041a2f8>] msm_load+0xe0/0x3b8
[    2.416643] [<ffffffc0003e6a00>] drm_dev_register+0xb8/0x110
[    2.421843] [<ffffffc0003e8c5c>] drm_platform_init+0x44/0xe0
[    2.427493] [<ffffffc000419490>] msm_drm_bind+0x18/0x28
[    2.433125] [<ffffffc00042a160>] try_to_bring_up_master.part.1+0xc0/0x108
[    2.438087] [<ffffffc00042a258>] component_master_add_with_match+0xb0/0x138
[    2.445031] [<ffffffc000419eb4>] msm_pdev_probe+0x64/0x78
[    2.451791] [<ffffffc000430c68>] platform_drv_probe+0x48/0xb8
[    2.457355] [<ffffffc00042f0ec>] driver_probe_device+0x8c/0x248
[    2.463074] [<ffffffc00042f398>] __driver_attach+0x98/0xa0
[    2.468811] [<ffffffc00042d52c>] bus_for_each_dev+0x5c/0xa0
[    2.474366] [<ffffffc00042ec04>] driver_attach+0x1c/0x28
[    2.479826] [<ffffffc00042e864>] bus_add_driver+0x14c/0x208
[    2.485390] [<ffffffc00042fa2c>] driver_register+0x5c/0x120
[    2.490679] [<ffffffc000430b98>] __platform_driver_register+0x58/0x68
[    2.496246] [<ffffffc000a4b268>] msm_drm_register+0x48/0x54
[    2.502831] [<ffffffc0000828e0>] do_one_initcall+0x88/0x1a8
[    2.508223] [<ffffffc000a2dac4>] kernel_init_freeable+0x144/0x1ec
[    2.513769] [<ffffffc0006663ac>] kernel_init+0xc/0xe0
[    2.520034] ---[ end trace 7e1590a270796f2f ]---
[    2.525063] Unable to handle kernel NULL pointer dereference at
virtual address 00000028
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: 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@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@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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: component_unbind_all() splat..

[Rob Clark]

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

Re: component_unbind_all() splat..

[Russell King - ARM Linux]

On Wed, May 06, 2015 at 12:45:04PM -0400, Rob Clark wrote:
> On Wed, May 6, 2015 at 12:01 PM, Russell King - ARM Linux
> <linux@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)

You can call it from ->bind() provided the preceding component_bind_all()
call succeeded.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel