[PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error

[PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error

[Martin Blumenstingl]

meson_drv_bind_master() does not free resources in the order they are
allocated. This can lead to crashes such as:
    Unable to handle kernel NULL pointer dereference at virtual address 00000000000000c8
    [...]
    Hardware name: Beelink GT-King Pro (DT)
    pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
    pc : meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
    lr : component_unbind+0x38/0x60
    [...]
    Call trace:
     meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
     component_unbind+0x38/0x60
     component_unbind_all+0xb8/0xc4
     meson_drv_bind_master+0x1ec/0x514 [meson_drm]
     meson_drv_bind+0x14/0x20 [meson_drm]
     try_to_bring_up_aggregate_device+0xa8/0x160
     __component_add+0xb8/0x1a8
     component_add+0x14/0x20
     meson_dw_hdmi_probe+0x1c/0x28 [meson_dw_hdmi]
     platform_probe+0x68/0xdc
     really_probe+0xc0/0x39c
     __driver_probe_device+0x7c/0x14c
     driver_probe_device+0x3c/0x120
     __driver_attach+0xc4/0x200
     bus_for_each_dev+0x78/0xd8
     driver_attach+0x24/0x30
     bus_add_driver+0x110/0x240
     driver_register+0x68/0x124
     __platform_driver_register+0x24/0x30
     meson_dw_hdmi_platform_driver_init+0x20/0x1000 [meson_dw_hdmi]
     do_one_initcall+0x50/0x1bc
     do_init_module+0x54/0x1fc
     load_module+0x788/0x884
     init_module_from_file+0x88/0xd4
     __arm64_sys_finit_module+0x248/0x340
     invoke_syscall+0x48/0x104
     el0_svc_common.constprop.0+0x40/0xe0
     do_el0_svc+0x1c/0x28
     el0_svc+0x30/0xcc
     el0t_64_sync_handler+0x120/0x12c
     el0t_64_sync+0x190/0x194

Clean up resources in the error path in the same order and under the
same conditions as they were allocated to fix this.

Reported-by: Furkan Kardame <f.kardame@manjaro.org>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
This issue was reported off-list so I'm unable to provide a link to the
report.

I'm not sure which Fixes tag fits best. My preference so far is
Fixes: 6a044642988b ("drm/meson: fix unbind path if HDMI fails to bind")
but I'll happily take other suggestions as well.


 drivers/gpu/drm/meson/meson_drv.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index 81d2ee37e773..031686fd4104 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -314,35 +314,35 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
 			dev_err(drm->dev, "Couldn't bind all components\n");
 			/* Do not try to unbind */
 			has_components = false;
-			goto exit_afbcd;
+			goto cvbs_encoder_remove;
 		}
 	}
 
 	ret = meson_encoder_hdmi_probe(priv);
 	if (ret)
-		goto exit_afbcd;
+		goto unbind_components;
 
 	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
 		ret = meson_encoder_dsi_probe(priv);
 		if (ret)
-			goto exit_afbcd;
+			goto hdmi_encoder_remove;
 	}
 
 	ret = meson_plane_create(priv);
 	if (ret)
-		goto exit_afbcd;
+		goto dsi_encoder_remove;
 
 	ret = meson_overlay_create(priv);
 	if (ret)
-		goto exit_afbcd;
+		goto dsi_encoder_remove;
 
 	ret = meson_crtc_create(priv);
 	if (ret)
-		goto exit_afbcd;
+		goto dsi_encoder_remove;
 
 	ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name, drm);
 	if (ret)
-		goto exit_afbcd;
+		goto dsi_encoder_remove;
 
 	drm_mode_config_reset(drm);
 
@@ -360,6 +360,16 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
 
 uninstall_irq:
 	free_irq(priv->vsync_irq, drm);
+dsi_encoder_remove:
+	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
+		meson_encoder_dsi_remove(priv);
+hdmi_encoder_remove:
+	meson_encoder_hdmi_remove(priv);
+unbind_components:
+	if (has_components)
+		component_unbind_all(dev, drm);
+cvbs_encoder_remove:
+	meson_encoder_cvbs_remove(priv);
 exit_afbcd:
 	if (priv->afbcd.ops)
 		priv->afbcd.ops->exit(priv);
@@ -374,13 +384,6 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
 free_drm:
 	drm_dev_put(drm);
 
-	meson_encoder_dsi_remove(priv);
-	meson_encoder_hdmi_remove(priv);
-	meson_encoder_cvbs_remove(priv);
-
-	if (has_components)
-		component_unbind_all(dev, drm);
-
 	return ret;
 }
 
-- 
2.49.0

Re: [PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error

[Anand Moon]

Hi Martin,

On Thu, 10 Apr 2025 at 03:30, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> meson_drv_bind_master() does not free resources in the order they are
> allocated. This can lead to crashes such as:
>     Unable to handle kernel NULL pointer dereference at virtual address 00000000000000c8
>     [...]
>     Hardware name: Beelink GT-King Pro (DT)
>     pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>     pc : meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
>     lr : component_unbind+0x38/0x60
>     [...]
>     Call trace:
>      meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
>      component_unbind+0x38/0x60
>      component_unbind_all+0xb8/0xc4
>      meson_drv_bind_master+0x1ec/0x514 [meson_drm]
>      meson_drv_bind+0x14/0x20 [meson_drm]
>      try_to_bring_up_aggregate_device+0xa8/0x160
>      __component_add+0xb8/0x1a8
>      component_add+0x14/0x20
>      meson_dw_hdmi_probe+0x1c/0x28 [meson_dw_hdmi]
>      platform_probe+0x68/0xdc
>      really_probe+0xc0/0x39c
>      __driver_probe_device+0x7c/0x14c
>      driver_probe_device+0x3c/0x120
>      __driver_attach+0xc4/0x200
>      bus_for_each_dev+0x78/0xd8
>      driver_attach+0x24/0x30
>      bus_add_driver+0x110/0x240
>      driver_register+0x68/0x124
>      __platform_driver_register+0x24/0x30
>      meson_dw_hdmi_platform_driver_init+0x20/0x1000 [meson_dw_hdmi]
>      do_one_initcall+0x50/0x1bc
>      do_init_module+0x54/0x1fc
>      load_module+0x788/0x884
>      init_module_from_file+0x88/0xd4
>      __arm64_sys_finit_module+0x248/0x340
>      invoke_syscall+0x48/0x104
>      el0_svc_common.constprop.0+0x40/0xe0
>      do_el0_svc+0x1c/0x28
>      el0_svc+0x30/0xcc
>      el0t_64_sync_handler+0x120/0x12c
>      el0t_64_sync+0x190/0x194
>
> Clean up resources in the error path in the same order and under the
> same conditions as they were allocated to fix this.
>
> Reported-by: Furkan Kardame <f.kardame@manjaro.org>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> This issue was reported off-list so I'm unable to provide a link to the
> report.
>
> I'm not sure which Fixes tag fits best. My preference so far is
> Fixes: 6a044642988b ("drm/meson: fix unbind path if HDMI fails to bind")
> but I'll happily take other suggestions as well.
>
Thanks for this Fix.

Reviewed-by: Anand Moon <linux.amoon@gmail.com>
Tested-by: Anand Moon <linux.amoon@gmail.com>

I have tested with the following script on AML-S905X-CC
---------
#! /bin/bash
set +x

cd /sys/bus/platform/drivers/meson-drm/
for i in $(seq 1 10); do
        echo "========================" $i
        echo d0100000.vpu > unbind
        /usr/bin/sleep 1
        echo d0100000.vpu > bind
        /usr/bin/sleep 1
done

Thanks
-Anand

RE: [PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error

[linux]

Hi Martin,

Thank you for the patch.

I encountered this issue some time ago as well and had a possible fix in my tree (see
below). 
My apologies for not upstreaming it earlier.

While my fix is not as symmetric as yours—I like symmetry—it is somewhat simpler. It
did make the assumption that only  calling component_unbind_all() was at fault and the the rest of the 
code was correct. Therefore, calling one of the following functions:
meson_encoder_dsi_remove()
meson_encoder_hdmi_remove()
meson_encoder_cvbs_remove()
in case their counterpart was not called, should not result in any issues.

I just verified, and, as far as I understand, all of these functions do a check to confirm
whether the encoder was initialized before proceeding with cleanup.

-----------------------------------------------------
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index 81d2ee37e773..4e2d45a271c2 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -184,6 +184,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
        const struct meson_drm_match_data *match;
        struct meson_drm *priv;
        struct drm_device *drm;
+       bool do_unbind = false;
        struct resource *res;
        void __iomem *regs;
        int ret, i;
@@ -312,10 +313,9 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
                ret = component_bind_all(dev, drm);
                if (ret) {
                        dev_err(drm->dev, "Couldn't bind all components\n");
-                       /* Do not try to unbind */
-                       has_components = false;
                        goto exit_afbcd;
                }
+               do_unbind = true;
        }

        ret = meson_encoder_hdmi_probe(priv);
@@ -378,7 +378,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
        meson_encoder_hdmi_remove(priv);
        meson_encoder_cvbs_remove(priv);

-       if (has_components)
+       if (do_unbind)
                component_unbind_all(dev, drm);

        return ret;
-----------------------------

This patch has somewhat less code redundancy. I don’t have a strong
preference for either patch, so please feel free to choose whichever you 
prefer. 

If you decide to go with yours, I’ve provided one minor comment inline
for your consideration.

> -----Original Message-----
> From: linux-amlogic <linux-amlogic-bounces@lists.infradead.org> On Behalf
> Of Martin Blumenstingl
> Sent: Wednesday, April 9, 2025 11:44 PM
> To: linux-amlogic@lists.infradead.org; dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> jbrunet@baylibre.com; neil.armstrong@linaro.org; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Furkan Kardame
> <f.kardame@manjaro.org>
> Subject: [PATCH] drm/meson: fix resource cleanup in
> meson_drv_bind_master() on error
> 
> meson_drv_bind_master() does not free resources in the order they are
> allocated. This can lead to crashes such as:
>     Unable to handle kernel NULL pointer dereference at virtual address
> 00000000000000c8
>     [...]
>     Hardware name: Beelink GT-King Pro (DT)
>     pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>     pc : meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
>     lr : component_unbind+0x38/0x60
>     [...]
>     Call trace:
>      meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
>      component_unbind+0x38/0x60
>      component_unbind_all+0xb8/0xc4
>      meson_drv_bind_master+0x1ec/0x514 [meson_drm]
>      meson_drv_bind+0x14/0x20 [meson_drm]
>      try_to_bring_up_aggregate_device+0xa8/0x160
>      __component_add+0xb8/0x1a8
>      component_add+0x14/0x20
>      meson_dw_hdmi_probe+0x1c/0x28 [meson_dw_hdmi]
>      platform_probe+0x68/0xdc
>      really_probe+0xc0/0x39c
>      __driver_probe_device+0x7c/0x14c
>      driver_probe_device+0x3c/0x120
>      __driver_attach+0xc4/0x200
>      bus_for_each_dev+0x78/0xd8
>      driver_attach+0x24/0x30
>      bus_add_driver+0x110/0x240
>      driver_register+0x68/0x124
>      __platform_driver_register+0x24/0x30
>      meson_dw_hdmi_platform_driver_init+0x20/0x1000 [meson_dw_hdmi]
>      do_one_initcall+0x50/0x1bc
>      do_init_module+0x54/0x1fc
>      load_module+0x788/0x884
>      init_module_from_file+0x88/0xd4
>      __arm64_sys_finit_module+0x248/0x340
>      invoke_syscall+0x48/0x104
>      el0_svc_common.constprop.0+0x40/0xe0
>      do_el0_svc+0x1c/0x28
>      el0_svc+0x30/0xcc
>      el0t_64_sync_handler+0x120/0x12c
>      el0t_64_sync+0x190/0x194
> 
> Clean up resources in the error path in the same order and under the
> same conditions as they were allocated to fix this.
> 
> Reported-by: Furkan Kardame <f.kardame@manjaro.org>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> This issue was reported off-list so I'm unable to provide a link to the
> report.
> 
> I'm not sure which Fixes tag fits best. My preference so far is
> Fixes: 6a044642988b ("drm/meson: fix unbind path if HDMI fails to bind")
> but I'll happily take other suggestions as well.
> 
> 
>  drivers/gpu/drm/meson/meson_drv.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_drv.c
> b/drivers/gpu/drm/meson/meson_drv.c
> index 81d2ee37e773..031686fd4104 100644
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -314,35 +314,35 @@ static int meson_drv_bind_master(struct device
> *dev, bool has_components)
>  			dev_err(drm->dev, "Couldn't bind all
> components\n");
>  			/* Do not try to unbind */
>  			has_components = false;

Above two lines are no longer used, so can be removed.

> -			goto exit_afbcd;
> +			goto cvbs_encoder_remove;
>  		}
>  	}
> 
>  	ret = meson_encoder_hdmi_probe(priv);
>  	if (ret)
> -		goto exit_afbcd;
> +		goto unbind_components;
> 
>  	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
>  		ret = meson_encoder_dsi_probe(priv);
>  		if (ret)
> -			goto exit_afbcd;
> +			goto hdmi_encoder_remove;
>  	}
> 
>  	ret = meson_plane_create(priv);
>  	if (ret)
> -		goto exit_afbcd;
> +		goto dsi_encoder_remove;
> 
>  	ret = meson_overlay_create(priv);
>  	if (ret)
> -		goto exit_afbcd;
> +		goto dsi_encoder_remove;
> 
>  	ret = meson_crtc_create(priv);
>  	if (ret)
> -		goto exit_afbcd;
> +		goto dsi_encoder_remove;
> 
>  	ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name,
> drm);
>  	if (ret)
> -		goto exit_afbcd;
> +		goto dsi_encoder_remove;
> 
>  	drm_mode_config_reset(drm);
> 
> @@ -360,6 +360,16 @@ static int meson_drv_bind_master(struct device
> *dev, bool has_components)
> 
>  uninstall_irq:
>  	free_irq(priv->vsync_irq, drm);
> +dsi_encoder_remove:
> +	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> +		meson_encoder_dsi_remove(priv);
> +hdmi_encoder_remove:
> +	meson_encoder_hdmi_remove(priv);
> +unbind_components:
> +	if (has_components)
> +		component_unbind_all(dev, drm);
> +cvbs_encoder_remove:
> +	meson_encoder_cvbs_remove(priv);
>  exit_afbcd:
>  	if (priv->afbcd.ops)
>  		priv->afbcd.ops->exit(priv);
> @@ -374,13 +384,6 @@ static int meson_drv_bind_master(struct device
> *dev, bool has_components)
>  free_drm:
>  	drm_dev_put(drm);
> 
> -	meson_encoder_dsi_remove(priv);
> -	meson_encoder_hdmi_remove(priv);
> -	meson_encoder_cvbs_remove(priv);
> -
> -	if (has_components)
> -		component_unbind_all(dev, drm);
> -
>  	return ret;
>  }
> 
 --
Best regards,

Martijn

Re: [PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error

[Martin Blumenstingl]

Hi Martijn, Hi Neil,

On Thu, Apr 10, 2025 at 8:46 PM <linux@martijnvandeventer.nl> wrote:
>
> Hi Martin,
>
> Thank you for the patch.
>
> I encountered this issue some time ago as well and had a possible fix in my tree (see
> below).
> My apologies for not upstreaming it earlier.
No worries, we're all busy with both, offline and online life ;-)

> While my fix is not as symmetric as yours—I like symmetry—it is somewhat simpler. It
> did make the assumption that only  calling component_unbind_all() was at fault and the the rest of the
> code was correct. Therefore, calling one of the following functions:
> meson_encoder_dsi_remove()
> meson_encoder_hdmi_remove()
> meson_encoder_cvbs_remove()
> in case their counterpart was not called, should not result in any issues.
>
> I just verified, and, as far as I understand, all of these functions do a check to confirm
> whether the encoder was initialized before proceeding with cleanup.
Yep, that seems to be the case right now.
Neil, would you like Martijn's more simple approach with a Fixes tag
and backport that?
Then I'd send my patch as a small cleanup which doesn't have to be
backported. Otherwise I'd spin a v2 with a fix for the issue that
Martijn found (and including Anand's Reviewed/Tested-by)?

[...]
> > diff --git a/drivers/gpu/drm/meson/meson_drv.c
> > b/drivers/gpu/drm/meson/meson_drv.c
> > index 81d2ee37e773..031686fd4104 100644
> > --- a/drivers/gpu/drm/meson/meson_drv.c
> > +++ b/drivers/gpu/drm/meson/meson_drv.c
> > @@ -314,35 +314,35 @@ static int meson_drv_bind_master(struct device
> > *dev, bool has_components)
> >                       dev_err(drm->dev, "Couldn't bind all
> > components\n");
> >                       /* Do not try to unbind */
> >                       has_components = false;
>
> Above two lines are no longer used, so can be removed.
Well spotted, thank you!


Best regards,
Martin

Re: [PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error

[neil.armstrong]

On 19/04/2025 23:32, Martin Blumenstingl wrote:
> Hi Martijn, Hi Neil,
> 
> On Thu, Apr 10, 2025 at 8:46 PM <linux@martijnvandeventer.nl> wrote:
>>
>> Hi Martin,
>>
>> Thank you for the patch.
>>
>> I encountered this issue some time ago as well and had a possible fix in my tree (see
>> below).
>> My apologies for not upstreaming it earlier.
> No worries, we're all busy with both, offline and online life ;-)
> 
>> While my fix is not as symmetric as yours—I like symmetry—it is somewhat simpler. It
>> did make the assumption that only  calling component_unbind_all() was at fault and the the rest of the
>> code was correct. Therefore, calling one of the following functions:
>> meson_encoder_dsi_remove()
>> meson_encoder_hdmi_remove()
>> meson_encoder_cvbs_remove()
>> in case their counterpart was not called, should not result in any issues.
>>
>> I just verified, and, as far as I understand, all of these functions do a check to confirm
>> whether the encoder was initialized before proceeding with cleanup.
> Yep, that seems to be the case right now.
> Neil, would you like Martijn's more simple approach with a Fixes tag
> and backport that?
> Then I'd send my patch as a small cleanup which doesn't have to be
> backported. Otherwise I'd spin a v2 with a fix for the issue that
> Martijn found (and including Anand's Reviewed/Tested-by)?

Please send a follow-up, I'll apply this one today.

Thanks,
Neil

> 
> [...]
>>> diff --git a/drivers/gpu/drm/meson/meson_drv.c
>>> b/drivers/gpu/drm/meson/meson_drv.c
>>> index 81d2ee37e773..031686fd4104 100644
>>> --- a/drivers/gpu/drm/meson/meson_drv.c
>>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>>> @@ -314,35 +314,35 @@ static int meson_drv_bind_master(struct device
>>> *dev, bool has_components)
>>>                        dev_err(drm->dev, "Couldn't bind all
>>> components\n");
>>>                        /* Do not try to unbind */
>>>                        has_components = false;
>>
>> Above two lines are no longer used, so can be removed.
> Well spotted, thank you!
> 
> 
> Best regards,
> Martin

Re: [PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error

[Neil Armstrong]

On 22/04/2025 09:04, neil.armstrong@linaro.org wrote:
> On 19/04/2025 23:32, Martin Blumenstingl wrote:
>> Hi Martijn, Hi Neil,
>>
>> On Thu, Apr 10, 2025 at 8:46 PM <linux@martijnvandeventer.nl> wrote:
>>>
>>> Hi Martin,
>>>
>>> Thank you for the patch.
>>>
>>> I encountered this issue some time ago as well and had a possible fix in my tree (see
>>> below).
>>> My apologies for not upstreaming it earlier.
>> No worries, we're all busy with both, offline and online life ;-)
>>
>>> While my fix is not as symmetric as yours—I like symmetry—it is somewhat simpler. It
>>> did make the assumption that only  calling component_unbind_all() was at fault and the the rest of the
>>> code was correct. Therefore, calling one of the following functions:
>>> meson_encoder_dsi_remove()
>>> meson_encoder_hdmi_remove()
>>> meson_encoder_cvbs_remove()
>>> in case their counterpart was not called, should not result in any issues.
>>>
>>> I just verified, and, as far as I understand, all of these functions do a check to confirm
>>> whether the encoder was initialized before proceeding with cleanup.
>> Yep, that seems to be the case right now.
>> Neil, would you like Martijn's more simple approach with a Fixes tag
>> and backport that?
>> Then I'd send my patch as a small cleanup which doesn't have to be
>> backported. Otherwise I'd spin a v2 with a fix for the issue that
>> Martijn found (and including Anand's Reviewed/Tested-by)?
> 
> Please send a follow-up, I'll apply this one today.
> 

Yeah finally please split this in two:
- patch 1 that can be backported
- patch 2 remaining changes

Thanks,
Neil

> Thanks,
> Neil
> 
>>
>> [...]
>>>> diff --git a/drivers/gpu/drm/meson/meson_drv.c
>>>> b/drivers/gpu/drm/meson/meson_drv.c
>>>> index 81d2ee37e773..031686fd4104 100644
>>>> --- a/drivers/gpu/drm/meson/meson_drv.c
>>>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>>>> @@ -314,35 +314,35 @@ static int meson_drv_bind_master(struct device
>>>> *dev, bool has_components)
>>>>                        dev_err(drm->dev, "Couldn't bind all
>>>> components\n");
>>>>                        /* Do not try to unbind */
>>>>                        has_components = false;
>>>
>>> Above two lines are no longer used, so can be removed.
>> Well spotted, thank you!
>>
>>
>> Best regards,
>> Martin
> 

Re: [PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error

[Martin Blumenstingl]

On Tue, Apr 22, 2025 at 9:23 AM Neil Armstrong
<neil.armstrong@linaro.org> wrote:
>
> On 22/04/2025 09:04, neil.armstrong@linaro.org wrote:
> > On 19/04/2025 23:32, Martin Blumenstingl wrote:
> >> Hi Martijn, Hi Neil,
> >>
> >> On Thu, Apr 10, 2025 at 8:46 PM <linux@martijnvandeventer.nl> wrote:
> >>>
> >>> Hi Martin,
> >>>
> >>> Thank you for the patch.
> >>>
> >>> I encountered this issue some time ago as well and had a possible fix in my tree (see
> >>> below).
> >>> My apologies for not upstreaming it earlier.
> >> No worries, we're all busy with both, offline and online life ;-)
> >>
> >>> While my fix is not as symmetric as yours—I like symmetry—it is somewhat simpler. It
> >>> did make the assumption that only  calling component_unbind_all() was at fault and the the rest of the
> >>> code was correct. Therefore, calling one of the following functions:
> >>> meson_encoder_dsi_remove()
> >>> meson_encoder_hdmi_remove()
> >>> meson_encoder_cvbs_remove()
> >>> in case their counterpart was not called, should not result in any issues.
> >>>
> >>> I just verified, and, as far as I understand, all of these functions do a check to confirm
> >>> whether the encoder was initialized before proceeding with cleanup.
> >> Yep, that seems to be the case right now.
> >> Neil, would you like Martijn's more simple approach with a Fixes tag
> >> and backport that?
> >> Then I'd send my patch as a small cleanup which doesn't have to be
> >> backported. Otherwise I'd spin a v2 with a fix for the issue that
> >> Martijn found (and including Anand's Reviewed/Tested-by)?
> >
> > Please send a follow-up, I'll apply this one today.
> >
>
> Yeah finally please split this in two:
> - patch 1 that can be backported
Martijn, I still haven't found the time to send a small patch that can
be easily backported.
If you want to submit your version of the patch: please go ahead!
Otherwise I'll need to find some time next week.


Best regards,
Martin

Re: [PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error

[Fedor Pchelkin]

Martin Blumenstingl wrote:
> @@ -360,6 +360,16 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
>  
>  uninstall_irq:
>  	free_irq(priv->vsync_irq, drm);
> +dsi_encoder_remove:
> +	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> +		meson_encoder_dsi_remove(priv);
> +hdmi_encoder_remove:
> +	meson_encoder_hdmi_remove(priv);
> +unbind_components:
> +	if (has_components)
> +		component_unbind_all(dev, drm);

As 6a044642988b ("drm/meson: fix unbind path if HDMI fails to bind")
states, it seems invalid to call component_unbind_all() before
drm_dev_put(). Wouldn't this patch reintroduce the problem here?

In that sense the diff proposed by Martijn <linux@martijnvandeventer.nl>
behaves more correctly.


I've also found this thread [1] with another error path fixing patch. It
was suggested to improve that fix with managed drm device [2] interfaces but
AFAICS using devm_drm_dev_alloc() will reintroduce the problem mentioned
in 6a044642988b, too.

I think [1] should be applied as well with Martijn's patch?

[1]: https://lore.kernel.org/dri-devel/20240809124725.17956-1-abelova@astralinux.ru/T/#u
[2]: https://lore.kernel.org/dri-devel/20240828110421.14956-1-abelova@astralinux.ru/T/#u


Thanks!

> +cvbs_encoder_remove:
> +	meson_encoder_cvbs_remove(priv);
>  exit_afbcd:
>  	if (priv->afbcd.ops)
>  		priv->afbcd.ops->exit(priv);
> @@ -374,13 +384,6 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
>  free_drm:
>  	drm_dev_put(drm);
>  
> -	meson_encoder_dsi_remove(priv);
> -	meson_encoder_hdmi_remove(priv);
> -	meson_encoder_cvbs_remove(priv);
> -
> -	if (has_components)
> -		component_unbind_all(dev, drm);
> -
>  	return ret;
>  }
>  
> -- 
> 2.49.0