[3/4] fbdev: imsttfb: Move a variable assignment for an error code in imsttfb_probe()

Commit Message

Markus Elfring May 23, 2023, 5:42 p.m. UTC

From: Markus Elfring <[e-mail]>
Date: Tue, 23 May 2023 18:30:26 +0200

The value “-ENOMEM” was assigned to the variable “ret”
at the beginning.
Move this statement directly before the first ioremap() call.

Signed-off-by: Markus Elfring <[e-mail]>
---
 drivers/video/fbdev/imsttfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.40.1

Comments

Helge Deller May 24, 2023, 6:11 p.m. UTC | #1

On 5/23/23 19:42, Markus Elfring wrote:
> From: Markus Elfring <[e-mail]>
> Date: Tue, 23 May 2023 18:30:26 +0200
>
> The value “-ENOMEM” was assigned to the variable “ret”
> at the beginning.
> Move this statement directly before the first ioremap() call.

Please do not move such variables without real need.
It makes backporting (of this and maybe follow-up patches) much more
complicated and the compiler will optimize it anyway.

Thanks!
Helge

>
> Signed-off-by: Markus Elfring <[e-mail]>
> ---
>   drivers/video/fbdev/imsttfb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
> index 6490f544f8eb..90c72428e8d7 100644
> --- a/drivers/video/fbdev/imsttfb.c
> +++ b/drivers/video/fbdev/imsttfb.c
> @@ -1484,7 +1484,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	ret = aperture_remove_conflicting_pci_devices(pdev, "imsttfb");
>   	if (ret)
>   		return ret;
> -	ret = -ENOMEM;
>
>   	dp = pci_device_to_OF_node(pdev);
>   	if(dp)
> @@ -1525,6 +1524,7 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	}
>
>   	info->fix.smem_start = addr;
> +	ret = -ENOMEM;
>   	info->screen_base = (__u8 *)ioremap(addr, par->ramdac == IBM ?
>   					    0x400000 : 0x800000);
>   	if (!info->screen_base)
> --
> 2.40.1
>

Markus Elfring May 25, 2023, 5:03 a.m. UTC | #2

>> The value “-ENOMEM” was assigned to the variable “ret”
>> at the beginning.
>> Move this statement directly before the first ioremap() call.
>
> Please do not move such variables without real need.

Is there a need to explain desirable effects better?


> It makes backporting (of this and maybe follow-up patches) much more complicated

I propose to reconsider such development concerns a bit more.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/video/fbdev/imsttfb.c?h=v6.4-rc3


> and the compiler will optimize it anyway.

How much do expectations fit to supported and documented software optimisations?

Regards,
Markus

Patch

diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index 6490f544f8eb..90c72428e8d7 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1484,7 +1484,6 @@  static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	ret = aperture_remove_conflicting_pci_devices(pdev, "imsttfb");
 	if (ret)
 		return ret;
-	ret = -ENOMEM;

 	dp = pci_device_to_OF_node(pdev);
 	if(dp)
@@ -1525,6 +1524,7 @@  static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}

 	info->fix.smem_start = addr;
+	ret = -ENOMEM;
 	info->screen_base = (__u8 *)ioremap(addr, par->ramdac == IBM ?
 					    0x400000 : 0x800000);
 	if (!info->screen_base)