Skip to contentSkip to conversation

sa1100fb updates

PATCH New Russell King - ARM Linux 3 participants 13 messages Sep 29, 2017 – Nov 27, 2017 Framebuffer layer

lore
Share & tools
lore

Russell King - ARM Linux

author
#0 lore raw

cover sa1100fb updates

Hi Bart,

This series updates the sa1100fb fbdev driver's initialisation paths to be more robust. In doing these updates, I realised that we omitted to free some memory which was allocated for the framebuffer if initialisation fails - something that was hidden due to the complex cleanup that the driver performs. Switching to managed resources made this more obvious.

Tested on H3600 iPAQ and Assabet.

 drivers/video/fbdev/sa1100fb.c | 75 +++++++++++++++---------------------------
 drivers/video/fbdev/sa1100fb.h |  2 ++
 2 files changed, 29 insertions(+), 48 deletions(-)
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
#1 lore raw

PATCH 1/8 video: sa1100fb: use devm_kzalloc()

Use devm_kzalloc() when allocating the private data for the framebuffer device.

Signed-off-by Russell King <[email protected]>
---
 drivers/video/fbdev/sa1100fb.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/video/fbdev/sa1100fb.c +2 -4
index fc2aaa5aca23..05a80e08dcde 100644
@@ -1132,12 +1132,11 @@ static struct sa1100fb_info *sa1100fb_init_fbinfo(struct device *dev)
 	struct sa1100fb_info *fbi;
 	unsigned i;
 
-	fbi = kmalloc(sizeof(struct sa1100fb_info) + sizeof(u32) * 16,
-		      GFP_KERNEL);
+	fbi = devm_kzalloc(dev, sizeof(struct sa1100fb_info) + sizeof(u32) * 16,
+			   GFP_KERNEL);
 	if (!fbi)
 		return NULL;
 
-	memset(fbi, 0, sizeof(struct sa1100fb_info));
 	fbi->dev = dev;
 
 	strcpy(fbi->fb.fix.id, SA1100_NAME);

@@ -1292,7 +1291,6 @@ static int sa1100fb_probe(struct platform_device *pdev)
 		iounmap(fbi->base);
 	if (fbi->clk)
 		clk_put(fbi->clk);
-	kfree(fbi);
 	release_mem_region(res->start, resource_size(res));
 	return ret;
 }
Signature · 1 line
2.7.4
#2 lore raw

PATCH 2/8 video: sa1100fb: use devm_clk_get()

In reply to Russell King - ARM Linux

Use devm_clk_get() to get the clock for the LCD.

Signed-off-by Russell King <[email protected]>
---
 drivers/video/fbdev/sa1100fb.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/video/fbdev/sa1100fb.c +1 -4
index 05a80e08dcde..1562d7607dd2 100644
@@ -1230,10 +1230,9 @@ static int sa1100fb_probe(struct platform_device *pdev)
 	if (!fbi)
 		goto failed;
 
-	fbi->clk = clk_get(&pdev->dev, NULL);
+	fbi->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(fbi->clk)) {
 		ret = PTR_ERR(fbi->clk);
-		fbi->clk = NULL;
 		goto failed;
 	}
 

@@ -1289,8 +1288,6 @@ static int sa1100fb_probe(struct platform_device *pdev)
  failed:
 	if (fbi)
 		iounmap(fbi->base);
-	if (fbi->clk)
-		clk_put(fbi->clk);
 	release_mem_region(res->start, resource_size(res));
 	return ret;
 }
Signature · 1 line
2.7.4
#3 lore raw

PATCH 3/8 video: sa1100fb: use devm_ioremap_resource()

In reply to Russell King - ARM Linux

Use devm_ioremap_resource() to map the LCD controller memory region, and remove the unnecessary cleanup for this.

Signed-off-by Russell King <[email protected]>
---
 drivers/video/fbdev/sa1100fb.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/video/fbdev/sa1100fb.c +8 -12
index 1562d7607dd2..ab83970dbfd9 100644
@@ -1217,29 +1217,28 @@ static int sa1100fb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0 || !res)
+	if (irq < 0)
 		return -EINVAL;
 
-	if (!request_mem_region(res->start, resource_size(res), "LCD"))
-		return -EBUSY;
-
 	fbi = sa1100fb_init_fbinfo(&pdev->dev);
 	ret = -ENOMEM;
 	if (!fbi)
 		goto failed;
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	fbi->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(fbi->base)) {
+		ret = PTR_ERR(fbi->base);
+		goto failed;
+	}
+
 	fbi->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(fbi->clk)) {
 		ret = PTR_ERR(fbi->clk);
 		goto failed;
 	}
 
-	fbi->base = ioremap(res->start, resource_size(res));
-	if (!fbi->base)
-		goto failed;
-
 	/* Initialize video memory */
 	ret = sa1100fb_map_video_memory(fbi);
 	if (ret)

@@ -1286,9 +1285,6 @@ static int sa1100fb_probe(struct platform_device *pdev)
  err_free_irq:
 	free_irq(irq, fbi);
  failed:
-	if (fbi)
-		iounmap(fbi->base);
-	release_mem_region(res->start, resource_size(res));
 	return ret;
 }
Signature · 1 line
2.7.4
#4 lore raw

PATCH 4/8 video: sa1100fb: use devm_request_irq()

In reply to Russell King - ARM Linux

Use devm_request_irq() to request the interrupt (a little earlier too) so we can avoid having to manually clean this up.

Signed-off-by Russell King <[email protected]>
---
 drivers/video/fbdev/sa1100fb.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
drivers/video/fbdev/sa1100fb.c +8 -9
index ab83970dbfd9..a48fdb676f3e 100644
@@ -1239,22 +1239,23 @@ static int sa1100fb_probe(struct platform_device *pdev)
 		goto failed;
 	}
 
-	/* Initialize video memory */
-	ret = sa1100fb_map_video_memory(fbi);
-	if (ret)
-		goto failed;
-
-	ret = request_irq(irq, sa1100fb_handle_irq, 0, "LCD", fbi);
+	ret = devm_request_irq(&pdev->dev, irq, sa1100fb_handle_irq, 0,
+			       "LCD", fbi);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq failed: %d\n", ret);
 		goto failed;
 	}
 
+	/* Initialize video memory */
+	ret = sa1100fb_map_video_memory(fbi);
+	if (ret)
+		goto failed;
+
 	if (machine_is_shannon()) {
 		ret = gpio_request_one(SHANNON_GPIO_DISP_EN,
 			GPIOF_OUT_INIT_LOW, "display enable");
 		if (ret)
-			goto err_free_irq;
+			goto failed;
 	}
 
 	/*

@@ -1282,8 +1283,6 @@ static int sa1100fb_probe(struct platform_device *pdev)
  err_reg_fb:
 	if (machine_is_shannon())
 		gpio_free(SHANNON_GPIO_DISP_EN);
- err_free_irq:
-	free_irq(irq, fbi);
  failed:
 	return ret;
 }
Signature · 1 line
2.7.4
#5 lore raw

PATCH 5/8 video: sa1100fb: use devm_gpio_request_one()

In reply to Russell King - ARM Linux

Switch to using devm_gpio_request_one() to request the shannon gpio and move the request before the video memory allocation, so we request all device managed resources before this large allocation attempt.

Signed-off-by Russell King <[email protected]>
---
 drivers/video/fbdev/sa1100fb.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
drivers/video/fbdev/sa1100fb.c +7 -10
index a48fdb676f3e..7fa6c8f74ec6 100644
@@ -1246,18 +1246,18 @@ static int sa1100fb_probe(struct platform_device *pdev)
 		goto failed;
 	}
 
-	/* Initialize video memory */
-	ret = sa1100fb_map_video_memory(fbi);
-	if (ret)
-		goto failed;
-
 	if (machine_is_shannon()) {
-		ret = gpio_request_one(SHANNON_GPIO_DISP_EN,
+		ret = devm_gpio_request_one(&pdev->dev, SHANNON_GPIO_DISP_EN,
 			GPIOF_OUT_INIT_LOW, "display enable");
 		if (ret)
 			goto failed;
 	}
 
+	/* Initialize video memory */
+	ret = sa1100fb_map_video_memory(fbi);
+	if (ret)
+		goto failed;
+
 	/*
 	 * This makes sure that our colour bitfield
 	 * descriptors are correctly initialised.

@@ -1268,7 +1268,7 @@ static int sa1100fb_probe(struct platform_device *pdev)
 
 	ret = register_framebuffer(&fbi->fb);
 	if (ret < 0)
-		goto err_reg_fb;
+		goto failed;
 
 #ifdef CONFIG_CPU_FREQ
 	fbi->freq_transition.notifier_call = sa1100fb_freq_transition;

@@ -1280,9 +1280,6 @@ static int sa1100fb_probe(struct platform_device *pdev)
 	/* This driver cannot be unloaded at the moment */
 	return 0;
 
- err_reg_fb:
-	if (machine_is_shannon())
-		gpio_free(SHANNON_GPIO_DISP_EN);
  failed:
 	return ret;
 }
Signature · 1 line
2.7.4
#6 lore raw

PATCH 6/8 video: sa1100fb: clean up failure path

In reply to Russell King - ARM Linux

We merely return from the failed path, so remove all the gotos and use return statements instead.

Signed-off-by Russell King <[email protected]>
---
 drivers/video/fbdev/sa1100fb.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)
drivers/video/fbdev/sa1100fb.c +8 -13
index 7fa6c8f74ec6..16a974471c02 100644
@@ -1222,41 +1222,36 @@ static int sa1100fb_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	fbi = sa1100fb_init_fbinfo(&pdev->dev);
-	ret = -ENOMEM;
 	if (!fbi)
-		goto failed;
+		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	fbi->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(fbi->base)) {
-		ret = PTR_ERR(fbi->base);
-		goto failed;
-	}
+	if (IS_ERR(fbi->base))
+		return PTR_ERR(fbi->base);
 
 	fbi->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(fbi->clk)) {
-		ret = PTR_ERR(fbi->clk);
-		goto failed;
-	}
+	if (IS_ERR(fbi->clk))
+		return PTR_ERR(fbi->clk);
 
 	ret = devm_request_irq(&pdev->dev, irq, sa1100fb_handle_irq, 0,
 			       "LCD", fbi);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq failed: %d\n", ret);
-		goto failed;
+		return ret;
 	}
 
 	if (machine_is_shannon()) {
 		ret = devm_gpio_request_one(&pdev->dev, SHANNON_GPIO_DISP_EN,
 			GPIOF_OUT_INIT_LOW, "display enable");
 		if (ret)
-			goto failed;
+			return ret;
 	}
 
 	/* Initialize video memory */
 	ret = sa1100fb_map_video_memory(fbi);
 	if (ret)
-		goto failed;
+		return ret;
 
 	/*
 	 * This makes sure that our colour bitfield
Signature · 1 line
2.7.4
#7 lore raw

PATCH 7/8 video: sa1100fb: fix video memory allocation leak

In reply to Russell King - ARM Linux

Don't leak the video memory allocation if register_framebuffer() fails.

Signed-off-by Russell King <[email protected]>
---
 drivers/video/fbdev/sa1100fb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/video/fbdev/sa1100fb.c +5 -5
index 16a974471c02..56d514b5d252 100644
@@ -1262,8 +1262,11 @@ static int sa1100fb_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, fbi);
 
 	ret = register_framebuffer(&fbi->fb);
-	if (ret < 0)
-		goto failed;
+	if (ret < 0) {
+		dma_free_wc(fbi->dev, fbi->map_size, fbi->map_cpu,
+			    fbi->map_dma);
+		return ret;
+	}
 
 #ifdef CONFIG_CPU_FREQ
 	fbi->freq_transition.notifier_call = sa1100fb_freq_transition;

@@ -1274,9 +1277,6 @@ static int sa1100fb_probe(struct platform_device *pdev)
 
 	/* This driver cannot be unloaded at the moment */
 	return 0;
-
- failed:
-	return ret;
 }
 
 static struct platform_driver sa1100fb_driver = {
Signature · 1 line
2.7.4
#8 lore raw

PATCH 8/8 video: sa1100fb: move pseudo palette into sa1100fb_info structure

In reply to Russell King - ARM Linux

Move the pseudo palette inside the driver private data structure so we don't have to play tricks to cater for it.

Signed-off-by Russell King <[email protected]>
---
 drivers/video/fbdev/sa1100fb.c | 9 +++------
 drivers/video/fbdev/sa1100fb.h | 2 ++
 2 files changed, 5 insertions(+), 6 deletions(-)
drivers/video/fbdev/sa1100fb.c +3 -6
index 56d514b5d252..15ae50063296 100644
@@ -323,13 +323,11 @@ sa1100fb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 		 * according to the RGB bitfield information.
 		 */
 		if (regno < 16) {
-			u32 *pal = fbi->fb.pseudo_palette;
-
 			val  = chan_to_field(red, &fbi->fb.var.red);
 			val |= chan_to_field(green, &fbi->fb.var.green);
 			val |= chan_to_field(blue, &fbi->fb.var.blue);
 
-			pal[regno] = val;
+			fbi->pseudo_palette[regno] = val;
 			ret = 0;
 		}
 		break;

@@ -1132,8 +1130,7 @@ static struct sa1100fb_info *sa1100fb_init_fbinfo(struct device *dev)
 	struct sa1100fb_info *fbi;
 	unsigned i;
 
-	fbi = devm_kzalloc(dev, sizeof(struct sa1100fb_info) + sizeof(u32) * 16,
-			   GFP_KERNEL);
+	fbi = devm_kzalloc(dev, sizeof(struct sa1100fb_info), GFP_KERNEL);
 	if (!fbi)
 		return NULL;
 

@@ -1158,7 +1155,7 @@ static struct sa1100fb_info *sa1100fb_init_fbinfo(struct device *dev)
 	fbi->fb.fbops		= &sa1100fb_ops;
 	fbi->fb.flags		= FBINFO_DEFAULT;
 	fbi->fb.monspecs	= monspecs;
-	fbi->fb.pseudo_palette	= (fbi + 1);
+	fbi->fb.pseudo_palette	= fbi->pseudo_palette;
 
 	fbi->rgb[RGB_4]		= &rgb_4;
 	fbi->rgb[RGB_8]		= &rgb_8;
drivers/video/fbdev/sa1100fb.h +2 -0
index 0139d13377a5..7a1a9ca33cec 100644
@@ -69,6 +69,8 @@ struct sa1100fb_info {
 
 	const struct sa1100fb_mach_info *inf;
 	struct clk *clk;
+
+	u32 pseudo_palette[16];
 };
 
 #define TO_INF(ptr,member)	container_of(ptr,struct sa1100fb_info,member)
Signature · 1 line
2.7.4
Applied
#9 lore raw

cover sa1100fb updates

In reply to Russell King - ARM Linux
Quoted 1 line · Russell King - ARM Linux

Hi Bart,

Hi Russell,

Quoted 7 lines

This series updates the sa1100fb fbdev driver's initialisation paths to be more robust. In doing these updates, I realised that we omitted to free some memory which was allocated for the framebuffer if initialisation fails - something that was hidden due to the complex cleanup that the driver performs. Switching to managed resources made this more obvious.

Quoted 5 lines

Tested on H3600 iPAQ and Assabet.

 drivers/video/fbdev/sa1100fb.c | 75 +++++++++++++++---------------------------
 drivers/video/fbdev/sa1100fb.h |  2 ++
 2 files changed, 29 insertions(+), 48 deletions(-)

I queued all patches for 4.15, thanks!

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

author
#10 lore raw

cover sa1100fb updates

Quoted 12 lines · Bartlomiej Zolnierkiewicz
Quoted 1 line · Russell King - ARM Linux

Hi Bart,

Hi Russell,

Quoted 7 lines

This series updates the sa1100fb fbdev driver's initialisation paths to be more robust. In doing these updates, I realised that we omitted to free some memory which was allocated for the framebuffer if initialisation fails - something that was hidden due to the complex cleanup that the driver performs. Switching to managed resources made this more obvious.

Quoted 7 lines
Quoted 5 lines

Tested on H3600 iPAQ and Assabet.

 drivers/video/fbdev/sa1100fb.c | 75 +++++++++++++++---------------------------
 drivers/video/fbdev/sa1100fb.h |  2 ++
 2 files changed, 29 insertions(+), 48 deletions(-)

I queued all patches for 4.15, thanks!

Now that 4.15-rc1 is out, something tells me that was not actually the case. Any ideas what happened?

Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
#11 lore raw

cover sa1100fb updates

Quoted 15 lines · Russell King - ARM Linux
Quoted 14 lines · Bartlomiej Zolnierkiewicz
Quoted 1 line · Russell King - ARM Linux

Hi Bart,

Hi Russell,

Quoted 9 lines

This series updates the sa1100fb fbdev driver's initialisation paths to be more robust. In doing these updates, I realised that we omitted to free some memory which was allocated for the framebuffer if initialisation fails - something that was hidden due to the complex cleanup that the driver performs. Switching to managed resources made this more obvious.

Tested on H3600 iPAQ and Assabet.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
 drivers/video/fbdev/sa1100fb.c | 75 +++++++++++++++---------------------------
 drivers/video/fbdev/sa1100fb.h |  2 ++
 2 files changed, 29 insertions(+), 48 deletions(-)

I queued all patches for 4.15, thanks!

Now that 4.15-rc1 is out, something tells me that was not actually the case. Any ideas what happened?

Could you please explain the issue that you are seeing a bit more?

For me it looks all fine:

$ git log --oneline v4.15-rc1 drivers/video/fbdev/sa1100fb.c
cb6bc3f video: sa1100fb: move pseudo palette into sa1100fb_info structure
0ab7658 video: sa1100fb: fix video memory allocation leak
c244f8e video: sa1100fb: clean up failure path
5634cba video: sa1100fb: use devm_gpio_request_one()
f6fc8c9 video: sa1100fb: use devm_request_irq()
df6b228 video: sa1100fb: use devm_ioremap_resource()
e43064c video: sa1100fb: use devm_clk_get()
ba1d36b video: sa1100fb: use devm_kzalloc()
...

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

author
#12 lore raw

cover sa1100fb updates

Quoted 29 lines · Bartlomiej Zolnierkiewicz
Quoted 15 lines · Russell King - ARM Linux
Quoted 14 lines · Bartlomiej Zolnierkiewicz
Quoted 1 line · Russell King - ARM Linux

Hi Bart,

Hi Russell,

Quoted 9 lines

This series updates the sa1100fb fbdev driver's initialisation paths to be more robust. In doing these updates, I realised that we omitted to free some memory which was allocated for the framebuffer if initialisation fails - something that was hidden due to the complex cleanup that the driver performs. Switching to managed resources made this more obvious.

Tested on H3600 iPAQ and Assabet.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
 drivers/video/fbdev/sa1100fb.c | 75 +++++++++++++++---------------------------
 drivers/video/fbdev/sa1100fb.h |  2 ++
 2 files changed, 29 insertions(+), 48 deletions(-)

I queued all patches for 4.15, thanks!

Now that 4.15-rc1 is out, something tells me that was not actually the case. Any ideas what happened?

Could you please explain the issue that you are seeing a bit more?

For me it looks all fine:

Quoted 10 lines
$ git log --oneline v4.15-rc1 drivers/video/fbdev/sa1100fb.c
cb6bc3f video: sa1100fb: move pseudo palette into sa1100fb_info structure
0ab7658 video: sa1100fb: fix video memory allocation leak
c244f8e video: sa1100fb: clean up failure path
5634cba video: sa1100fb: use devm_gpio_request_one()
f6fc8c9 video: sa1100fb: use devm_request_irq()
df6b228 video: sa1100fb: use devm_ioremap_resource()
e43064c video: sa1100fb: use devm_clk_get()
ba1d36b video: sa1100fb: use devm_kzalloc()
...

Looks like I hadn't rebased that particular branch, so they were still showing up in my origin.. git log. Sorry about the false warning.

This is the problem of carrying close to 400 patches in separate branches, and git log origin.. not able to show the commits that the selection are based upon.

Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up