Skip to contentSkip to conversation

mtd: maps/physmap: fix oops in suspend/resume/shutdown ops

PATCH In review Anton Vorontsov 2 participants 3 messages Feb 17, 2008 – Mar 14, 2008 Memory technology devices (MTD)

lore
Share & tools
lore

Anton Vorontsov

author
#0 lore raw

PATCH 1/1 mtd: maps/physmap: fix oops in suspend/resume/shutdown ops

- - - -

# reboot
...
[   42.351266] Flash device refused suspend due to active operation (state 0)
[   42.358195] Unable to handle kernel NULL pointer dereference at virtual address 00000078
[   42.360060] pgd = c7d9c000
[   42.362769] [00000078] *pgd=a7d8d031, *pte=00000000, *ppte=00000000
[   42.372902] Internal error: Oops: 17 [#1]
[   42.376911] Modules linked in:
[   42.379980] CPU: 0    Not tainted  (2.6.25-rc2-10642-ge8f2594-dirty #73)
[   42.380000] PC is at physmap_flash_shutdown+0x28/0x54
...
[   42.380000] Backtrace:
[   42.380000] [<c0130c1c>] (physmap_flash_shutdown+0x0/0x54) from [<c01207c0>] (platform_drv_shutdown+0x20/0x24)
[   42.380000]  r5:28121969 r4:c0229e08
[   42.380000] [<c01207a0>] (platform_drv_shutdown+0x0/0x24) from [<c011cd40>] (device_shutdown+0x60/0x88)
[   42.380000] [<c011cce0>] (device_shutdown+0x0/0x88) from [<c003e8a4>] (kernel_restart_prepare+0x2c/0x3c)
[   42.380000]  r4:00000000
[   42.380000] [<c003e878>] (kernel_restart_prepare+0x0/0x3c) from [<c003ea00>] (kernel_restart+0x14/0x48)
[   42.380000] [<c003e9ec>] (kernel_restart+0x0/0x48) from [<c003fdc0>] (sys_reboot+0xe8/0x1f8)
[   42.380000]  r4:01234567
[   42.380000] [<c003fcd8>] (sys_reboot+0x0/0x1f8) from [<c001aa00>] (ret_fast_syscall+0x0/0x2c)
[   42.380000]  r7:00000058 r6:00000004 r5:00000001 r4:00000000
[   42.380000] Code: 0a000009 e7953004 e1a00003 e1a0e00f (e593f078)
[   42.650051] ---[ end trace 6d6c26a0fc3141de ]---
Segmentation fault
INIT: no more processes left in this runlevel
- - - -

While looping for mtd[i]s, we should stop at the mtd[i] == NULL.

This patch also removes unnecessary "if (info)" checks:
suspend/resume/shutdown ops are executed only if probe() is succeeded, so info is guaranteed to be !NULL.

Signed-off-by Anton Vorontsov <[email protected]>
---
 drivers/mtd/maps/physmap.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)
drivers/mtd/maps/physmap.c +7 -8
index f00e04e..bc4649a 100644
@@ -202,9 +202,8 @@ static int physmap_flash_suspend(struct platform_device *dev, pm_message_t state
 	int ret = 0;
 	int i;
 
-	if (info)
-		for (i = 0; i < MAX_RESOURCES; i++)
-			ret |= info->mtd[i]->suspend(info->mtd[i]);
+	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
+		ret |= info->mtd[i]->suspend(info->mtd[i]);
 
 	return ret;
 }

@@ -214,9 +213,9 @@ static int physmap_flash_resume(struct platform_device *dev)
 	struct physmap_flash_info *info = platform_get_drvdata(dev);
 	int i;
 
-	if (info)
-		for (i = 0; i < MAX_RESOURCES; i++)
-			info->mtd[i]->resume(info->mtd[i]);
+	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
+		info->mtd[i]->resume(info->mtd[i]);
+
 	return 0;
 }
 

@@ -225,8 +224,8 @@ static void physmap_flash_shutdown(struct platform_device *dev)
 	struct physmap_flash_info *info = platform_get_drvdata(dev);
 	int i;
 
-	for (i = 0; i < MAX_RESOURCES; i++)
-		if (info && info->mtd[i]->suspend(info->mtd[i]) == 0)
+	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
+		if (info->mtd[i]->suspend(info->mtd[i]) == 0)
 			info->mtd[i]->resume(info->mtd[i]);
 }
 #else
Signature · 1 line
1.5.3.7
author
#1 lore raw
Quoted 83 lines · Anton Vorontsov

- - - -

# reboot
...
[   42.351266] Flash device refused suspend due to active operation (state 0)
[   42.358195] Unable to handle kernel NULL pointer dereference at virtual address 00000078
[   42.360060] pgd = c7d9c000
[   42.362769] [00000078] *pgd=a7d8d031, *pte=00000000, *ppte=00000000
[   42.372902] Internal error: Oops: 17 [#1]
[   42.376911] Modules linked in:
[   42.379980] CPU: 0    Not tainted  (2.6.25-rc2-10642-ge8f2594-dirty #73)
[   42.380000] PC is at physmap_flash_shutdown+0x28/0x54
...
[   42.380000] Backtrace:
[   42.380000] [<c0130c1c>] (physmap_flash_shutdown+0x0/0x54) from [<c01207c0>] (platform_drv_shutdown+0x20/0x24)
[   42.380000]  r5:28121969 r4:c0229e08
[   42.380000] [<c01207a0>] (platform_drv_shutdown+0x0/0x24) from [<c011cd40>] (device_shutdown+0x60/0x88)
[   42.380000] [<c011cce0>] (device_shutdown+0x0/0x88) from [<c003e8a4>] (kernel_restart_prepare+0x2c/0x3c)
[   42.380000]  r4:00000000
[   42.380000] [<c003e878>] (kernel_restart_prepare+0x0/0x3c) from [<c003ea00>] (kernel_restart+0x14/0x48)
[   42.380000] [<c003e9ec>] (kernel_restart+0x0/0x48) from [<c003fdc0>] (sys_reboot+0xe8/0x1f8)
[   42.380000]  r4:01234567
[   42.380000] [<c003fcd8>] (sys_reboot+0x0/0x1f8) from [<c001aa00>] (ret_fast_syscall+0x0/0x2c)
[   42.380000]  r7:00000058 r6:00000004 r5:00000001 r4:00000000
[   42.380000] Code: 0a000009 e7953004 e1a00003 e1a0e00f (e593f078)
[   42.650051] ---[ end trace 6d6c26a0fc3141de ]---
Segmentation fault
INIT: no more processes left in this runlevel
- - - -

While looping for mtd[i]s, we should stop at the mtd[i] == NULL.

This patch also removes unnecessary "if (info)" checks:
suspend/resume/shutdown ops are executed only if probe() is succeeded, so info is guaranteed to be !NULL.

Signed-off-by Anton Vorontsov <[email protected]>
---
 drivers/mtd/maps/physmap.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)
drivers/mtd/maps/physmap.c +7 -8
index f00e04e..bc4649a 100644
@@ -202,9 +202,8 @@ static int physmap_flash_suspend(struct platform_device *dev, pm_message_t state
 	int ret = 0;
 	int i;
 
-	if (info)
-		for (i = 0; i < MAX_RESOURCES; i++)
-			ret |= info->mtd[i]->suspend(info->mtd[i]);
+	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
+		ret |= info->mtd[i]->suspend(info->mtd[i]);
 
 	return ret;
 }

@@ -214,9 +213,9 @@ static int physmap_flash_resume(struct platform_device *dev)
 	struct physmap_flash_info *info = platform_get_drvdata(dev);
 	int i;
 
-	if (info)
-		for (i = 0; i < MAX_RESOURCES; i++)
-			info->mtd[i]->resume(info->mtd[i]);
+	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
+		info->mtd[i]->resume(info->mtd[i]);
+
 	return 0;
 }
 

@@ -225,8 +224,8 @@ static void physmap_flash_shutdown(struct platform_device *dev)
 	struct physmap_flash_info *info = platform_get_drvdata(dev);
 	int i;
 
-	for (i = 0; i < MAX_RESOURCES; i++)
-		if (info && info->mtd[i]->suspend(info->mtd[i]) == 0)
+	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
+		if (info->mtd[i]->suspend(info->mtd[i]) == 0)
 			info->mtd[i]->resume(info->mtd[i]);
 }
 #else
Signature · 1 line
1.5.3.7

Patch seem to be lost. Would be great to get this applied before final 2.6.25...

Thanks,

Signature · 3 lines
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
Reviewed-by
#2 lore raw
Quoted 74 lines · Anton Vorontsov
Quoted 73 lines · Anton Vorontsov

- - - -

# reboot
...
[   42.351266] Flash device refused suspend due to active operation (state 0)
[   42.358195] Unable to handle kernel NULL pointer dereference at virtual address 00000078
[   42.360060] pgd = c7d9c000
[   42.362769] [00000078] *pgd=a7d8d031, *pte=00000000, *ppte=00000000
[   42.372902] Internal error: Oops: 17 [#1]
[   42.376911] Modules linked in:
[   42.379980] CPU: 0    Not tainted  (2.6.25-rc2-10642-ge8f2594-dirty #73)
[   42.380000] PC is at physmap_flash_shutdown+0x28/0x54
...
[   42.380000] Backtrace:
[   42.380000] [<c0130c1c>] (physmap_flash_shutdown+0x0/0x54) from [<c01207c0>] (platform_drv_shutdown+0x20/0x24)
[   42.380000]  r5:28121969 r4:c0229e08
[   42.380000] [<c01207a0>] (platform_drv_shutdown+0x0/0x24) from [<c011cd40>] (device_shutdown+0x60/0x88)
[   42.380000] [<c011cce0>] (device_shutdown+0x0/0x88) from [<c003e8a4>] (kernel_restart_prepare+0x2c/0x3c)
[   42.380000]  r4:00000000
[   42.380000] [<c003e878>] (kernel_restart_prepare+0x0/0x3c) from [<c003ea00>] (kernel_restart+0x14/0x48)
[   42.380000] [<c003e9ec>] (kernel_restart+0x0/0x48) from [<c003fdc0>] (sys_reboot+0xe8/0x1f8)
[   42.380000]  r4:01234567
[   42.380000] [<c003fcd8>] (sys_reboot+0x0/0x1f8) from [<c001aa00>] (ret_fast_syscall+0x0/0x2c)
[   42.380000]  r7:00000058 r6:00000004 r5:00000001 r4:00000000
[   42.380000] Code: 0a000009 e7953004 e1a00003 e1a0e00f (e593f078)
[   42.650051] ---[ end trace 6d6c26a0fc3141de ]---
Segmentation fault
INIT: no more processes left in this runlevel
- - - -

While looping for mtd[i]s, we should stop at the mtd[i] == NULL.

This patch also removes unnecessary "if (info)" checks:
suspend/resume/shutdown ops are executed only if probe() is succeeded, so info is guaranteed to be !NULL.

Signed-off-by Anton Vorontsov <[email protected]>
---
 drivers/mtd/maps/physmap.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)
drivers/mtd/maps/physmap.c +5 -6
index f00e04e..bc4649a 100644
@@ -202,9 +202,8 @@ static int physmap_flash_suspend(struct platform_device *dev, pm_message_t state
 	int ret = 0;
 	int i;
 
-	if (info)
-		for (i = 0; i < MAX_RESOURCES; i++)
-			ret |= info->mtd[i]->suspend(info->mtd[i]);
+	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
+		ret |= info->mtd[i]->suspend(info->mtd[i]);
 
 	return ret;
 }

@@ -214,9 +213,9 @@ static int physmap_flash_resume(struct platform_device *dev)
 	struct physmap_flash_info *info = platform_get_drvdata(dev);
 	int i;
 
-	if (info)
-		for (i = 0; i < MAX_RESOURCES; i++)
-			info->mtd[i]->resume(info->mtd[i]);
+	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
+		info->mtd[i]->resume(info->mtd[i]);
+
 	return 0;
 }
 

@@ -225,8 +224,8 @@ static void physmap_flash_shutdown(struct platform_device *dev)
 	struct physmap_flash_info *info = platform_get_drvdata(dev);
 	int i;
Quoted 13 lines
Quoted 10 lines
drivers/mtd/maps/physmap.c +2 -2
-	for (i = 0; i < MAX_RESOURCES; i++)
-		if (info && info->mtd[i]->suspend(info->mtd[i]) == 0)
+	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
+		if (info->mtd[i]->suspend(info->mtd[i]) == 0)
 			info->mtd[i]->resume(info->mtd[i]);
 }
 #else
Signature · 1 line
1.5.3.7

Patch seem to be lost. Would be great to get this applied before final 2.6.25...

Ooh! There has been another patch for this problem, but yours is much better.

Reviewed-by Joern Engel <[email protected]>

David, please apply this instead of Arnaud's or mine.

Jörn

Signature · 3 lines
There's nothing better for promoting creativity in a medium than
making an audience feel "Hmm ­ I could do better than that!"
-- Douglas Adams in a slashdot interview