[PATCH] drm/mxsfb: fix missing rollback on failure in mxsfb_probe()

[PATCH] drm/mxsfb: fix missing rollback on failure in mxsfb_probe()

[Luca Ceresoli]

When aperture_remove_all_conflicting_devices() fails, the current code
returns without going through the rollback actions at the end of the
function, thus the actions done by drm_dev_alloc() and mxsfb_load() are not
undone.

Fix by using a goto statament, as done for the previous and following error
conditions.

Fixes: c8e7b185d45b ("drm/mxsfb: Remove generic DRM drivers in probe function")
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
The offending commit is not yet merged into master, and even less in a
released kernel, so this does not need to go through stable.
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index c183b1112bc4e9fe4f3b048a2b6e4c98d1d47cb3..b4273e678d26dbc3dee2014266d61470da4e8010 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -365,9 +365,10 @@ static int mxsfb_probe(struct platform_device *pdev)
 	 * located anywhere in RAM
 	 */
 	ret = aperture_remove_all_conflicting_devices(mxsfb_driver.name);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "can't kick out existing framebuffers\n");
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "can't kick out existing framebuffers\n");
+		goto err_unload;
+	}
 
 	ret = drm_dev_register(drm, 0);
 	if (ret)

---
base-commit: f9f087d946266bc5da7c3a17bd8fd9d01969e3cf
change-id: 20250313-mxsfb_probe-fix-rollback-on-error-3074b9080f34

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

Re: [PATCH] drm/mxsfb: fix missing rollback on failure in mxsfb_probe()

[Thomas Zimmermann]

Hi

Am 13.03.25 um 15:25 schrieb Luca Ceresoli:
> When aperture_remove_all_conflicting_devices() fails, the current code
> returns without going through the rollback actions at the end of the
> function, thus the actions done by drm_dev_alloc() and mxsfb_load() are not
> undone.
>
> Fix by using a goto statament, as done for the previous and following error
> conditions.
>
> Fixes: c8e7b185d45b ("drm/mxsfb: Remove generic DRM drivers in probe function")
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> The offending commit is not yet merged into master, and even less in a
> released kernel, so this does not need to go through stable.
> ---
>   drivers/gpu/drm/mxsfb/mxsfb_drv.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index c183b1112bc4e9fe4f3b048a2b6e4c98d1d47cb3..b4273e678d26dbc3dee2014266d61470da4e8010 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -365,9 +365,10 @@ static int mxsfb_probe(struct platform_device *pdev)
>   	 * located anywhere in RAM
>   	 */
>   	ret = aperture_remove_all_conflicting_devices(mxsfb_driver.name);
> -	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -				     "can't kick out existing framebuffers\n");
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "can't kick out existing framebuffers\n");
> +		goto err_unload;
> +	}

I must have missed that when I reviewed the patch. But this call should 
happen much earlier. right at the top of the probe function before 
drm_dev_alloc(). Conflicting drivers need to be kicked out before 
setting up DRM. Could you please send a patch to move the call to the 
top? No extra cleanup would be required then.

Best regards
Thomas

>   
>   	ret = drm_dev_register(drm, 0);
>   	if (ret)
>
> ---
> base-commit: f9f087d946266bc5da7c3a17bd8fd9d01969e3cf
> change-id: 20250313-mxsfb_probe-fix-rollback-on-error-3074b9080f34
>
> Best regards,

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH] drm/mxsfb: fix missing rollback on failure in mxsfb_probe()

[Luca Ceresoli]

Hello Thomas,

On Thu, 13 Mar 2025 15:40:43 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> > @@ -365,9 +365,10 @@ static int mxsfb_probe(struct platform_device *pdev)
> >   	 * located anywhere in RAM
> >   	 */
> >   	ret = aperture_remove_all_conflicting_devices(mxsfb_driver.name);
> > -	if (ret)
> > -		return dev_err_probe(&pdev->dev, ret,
> > -				     "can't kick out existing framebuffers\n");
> > +	if (ret) {
> > +		dev_err_probe(&pdev->dev, ret, "can't kick out existing framebuffers\n");
> > +		goto err_unload;
> > +	}  
> 
> I must have missed that when I reviewed the patch. But this call should 
> happen much earlier. right at the top of the probe function before 
> drm_dev_alloc(). Conflicting drivers need to be kicked out before 
> setting up DRM. Could you please send a patch to move the call to the 
> top? No extra cleanup would be required then.

Sure, sending v2 in a moment.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] drm/mxsfb: fix missing rollback on failure in mxsfb_probe()

[Dario Binacchi]

Hi All,

On Thu, Mar 13, 2025 at 3:40 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 13.03.25 um 15:25 schrieb Luca Ceresoli:
> > When aperture_remove_all_conflicting_devices() fails, the current code
> > returns without going through the rollback actions at the end of the
> > function, thus the actions done by drm_dev_alloc() and mxsfb_load() are not
> > undone.
> >
> > Fix by using a goto statament, as done for the previous and following error
> > conditions.
> >
> > Fixes: c8e7b185d45b ("drm/mxsfb: Remove generic DRM drivers in probe function")
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---
> > The offending commit is not yet merged into master, and even less in a
> > released kernel, so this does not need to go through stable.
> > ---
> >   drivers/gpu/drm/mxsfb/mxsfb_drv.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > index c183b1112bc4e9fe4f3b048a2b6e4c98d1d47cb3..b4273e678d26dbc3dee2014266d61470da4e8010 100644
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > @@ -365,9 +365,10 @@ static int mxsfb_probe(struct platform_device *pdev)
> >        * located anywhere in RAM
> >        */
> >       ret = aperture_remove_all_conflicting_devices(mxsfb_driver.name);
> > -     if (ret)
> > -             return dev_err_probe(&pdev->dev, ret,
> > -                                  "can't kick out existing framebuffers\n");
> > +     if (ret) {
> > +             dev_err_probe(&pdev->dev, ret, "can't kick out existing framebuffers\n");
> > +             goto err_unload;
> > +     }
>
> I must have missed that when I reviewed the patch. But this call should
> happen much earlier. right at the top of the probe function before
> drm_dev_alloc().

I had added the call to aperture_remove_all_conflicting_devices()
after mxsfb_load() to
keep the splash screen loaded by U-Boot. So, IMHO, it would be better
to add the
goto in case of an error rather than moving
aperture_remove_all_conflicting_devices()
to the beginning of the probe function.

Thanks and regards,
Dario

> Conflicting drivers need to be kicked out before
> setting up DRM. Could you please send a patch to move the call to the
> top? No extra cleanup would be required then.
>
> Best regards
> Thomas
>
> >
> >       ret = drm_dev_register(drm, 0);
> >       if (ret)
> >
> > ---
> > base-commit: f9f087d946266bc5da7c3a17bd8fd9d01969e3cf
> > change-id: 20250313-mxsfb_probe-fix-rollback-on-error-3074b9080f34
> >
> > Best regards,
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
>


-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [PATCH] drm/mxsfb: fix missing rollback on failure in mxsfb_probe()

[Thomas Zimmermann]

Hi

Am 13.03.25 um 21:43 schrieb Dario Binacchi:
> Hi All,
>
> On Thu, Mar 13, 2025 at 3:40 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 13.03.25 um 15:25 schrieb Luca Ceresoli:
>>> When aperture_remove_all_conflicting_devices() fails, the current code
>>> returns without going through the rollback actions at the end of the
>>> function, thus the actions done by drm_dev_alloc() and mxsfb_load() are not
>>> undone.
>>>
>>> Fix by using a goto statament, as done for the previous and following error
>>> conditions.
>>>
>>> Fixes: c8e7b185d45b ("drm/mxsfb: Remove generic DRM drivers in probe function")
>>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>>> ---
>>> The offending commit is not yet merged into master, and even less in a
>>> released kernel, so this does not need to go through stable.
>>> ---
>>>    drivers/gpu/drm/mxsfb/mxsfb_drv.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
>>> index c183b1112bc4e9fe4f3b048a2b6e4c98d1d47cb3..b4273e678d26dbc3dee2014266d61470da4e8010 100644
>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
>>> @@ -365,9 +365,10 @@ static int mxsfb_probe(struct platform_device *pdev)
>>>         * located anywhere in RAM
>>>         */
>>>        ret = aperture_remove_all_conflicting_devices(mxsfb_driver.name);
>>> -     if (ret)
>>> -             return dev_err_probe(&pdev->dev, ret,
>>> -                                  "can't kick out existing framebuffers\n");
>>> +     if (ret) {
>>> +             dev_err_probe(&pdev->dev, ret, "can't kick out existing framebuffers\n");
>>> +             goto err_unload;
>>> +     }
>> I must have missed that when I reviewed the patch. But this call should
>> happen much earlier. right at the top of the probe function before
>> drm_dev_alloc().
> I had added the call to aperture_remove_all_conflicting_devices()
> after mxsfb_load() to
> keep the splash screen loaded by U-Boot. So, IMHO, it would be better
> to add the
> goto in case of an error rather than moving
> aperture_remove_all_conflicting_devices()
> to the beginning of the probe function.

But is that really safe?  Once you start touching any of the graphics 
registers, the previous framebuffer and display mode potentially becomes 
invalid. (Depends on the details of the driver and hardware, of course.)

Best regards
Thomas

>
> Thanks and regards,
> Dario
>
>> Conflicting drivers need to be kicked out before
>> setting up DRM. Could you please send a patch to move the call to the
>> top? No extra cleanup would be required then.
>>
>> Best regards
>> Thomas
>>
>>>        ret = drm_dev_register(drm, 0);
>>>        if (ret)
>>>
>>> ---
>>> base-commit: f9f087d946266bc5da7c3a17bd8fd9d01969e3cf
>>> change-id: 20250313-mxsfb_probe-fix-rollback-on-error-3074b9080f34
>>>
>>> Best regards,
>> --
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
>>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)