Skip to contentSkip to conversation

2.6.23-rc1: known regressions

PATCH In review Michal Piotrowski 4 participants 7 messages Jul 23, 2007

Subsystems (4)
Networking Network drivers Char and misc drivers LED
lore
Share & tools
lore

Michal Piotrowski

author
#0 lore raw

PATCH 1/2 2.6.23-rc1: known regressions

Hi all,

Here is a list of some known regressions in 2.6.23-rc1.

Feel free to add new regressions/remove fixed etc.
http://kernelnewbies.org/known_regressions

List of Aces

Name Regressions fixed since 21-Jun-2007

Andi Kleen                             4
Linus Torvalds                         4
Adrian Bunk                            3
Andrew Morton                          3
Jens Axboe                             3
Al Viro                                2
David Woodhouse                        2
Hugh Dickins                           2
Tejun Heo                              2

Unclassified

Subject : /usr/bin/ld: section .text [ffffffffff700500 -> ffffffffff7007e3] overlaps section .gnu.version_d [ffffffffff7004d8 -> ffffffffff70050f]
References : http://lkml.org/lkml/2007/7/22/239
Last known good : ?
Submitter : Andre Noll <[email protected]>

Caused-By       : ?
Handled-By      : Andi Kleen <[email protected]>
Status          : problem is being debugged

Subject : pcwd_init_module(): WARNING: at lib/kref.c:33 kref_get()
References : http://lkml.org/lkml/2007/7/22/94
Last known good : ?
Submitter : Ingo Molnar <[email protected]>

Caused-By       : ?
Handled-By      : ?
Status          : problem is being debugged

Subject : konqueror suddenly vanishing, "konqueror: Fatal IO error: client killed"
References : http://lkml.org/lkml/2007/7/22/86
Last known good : ?
Submitter : Markus <[email protected]>

Caused-By       : ?
Handled-By      : Ingo Molnar <[email protected]>
Status          : problem is being debugged

ACPI

Subject : drivers/misc/asus-laptop.c:*: error: 'struct led_classdev' has no member named 'class_dev'
References : http://lkml.org/lkml/2007/7/22/299
Last known good : ?
Submitter : Gabriel C <[email protected]>

Caused-By       : ?
Handled-By      : ?
Status          : unknown

Networking

Subject : New wake ups from sky2
References : http://lkml.org/lkml/2007/7/20/386
Last known good : ?
Submitter : Thomas Meyer <[email protected]>

Caused-By Stephen Hemminger <[email protected]>
                  commit eb35cf60e462491249166182e3e755d3d5d91a28
Handled-By Stephen Hemminger <[email protected]>

Status : unknown

Regards,
Michal

--
LOG
http://www.stardust.webpages.pl/log/

#1 lore raw
Quoted 9 lines · Michal Piotrowski

Subject : drivers/misc/asus-laptop.c:*: error: 'struct led_classdev' has no member named 'class_dev'
References : http://lkml.org/lkml/2007/7/22/299
Last known good : ?
Submitter : Gabriel C <[email protected]>

Caused-By       : ?
Handled-By      : ?
Status          : unknown

From 2a7e1148a9d3ee860dc2650c9a45288b120e250f Mon Sep 17 00:00:00 2001

From: Al Viro <[email protected]>
Date: Mon, 23 Jul 2007 06:20:22 -0400
Subject: [PATCH] Fix failure exits in asus-laptop

Fallout from f8a7c6fe14f556ca8eeddce258cb21392d0c3a2f. However, looking at it shows that checks done in ASUS_LED_UNREGISTER() can't trigger at all (we never get to asus_led_exit() if registration fails) and if that registration fails, we actually leak stuff. IOW, it's worse than just replacing class_dev with dev in there - the tests themselves had been papering over the lousy cleanup logics.

Signed-off-by Al Viro <[email protected]>
---
 drivers/misc/asus-laptop.c |   32 +++++++++++++++++++++-----------
 1 files changed, 21 insertions(+), 11 deletions(-)
drivers/misc/asus-laptop.c +21 -11
index f753060..6b89854 100644
@@ -1067,19 +1067,16 @@ static void asus_backlight_exit(void)
 }
 
 #define  ASUS_LED_UNREGISTER(object)				\
-	if(object##_led.class_dev				\
-	   && !IS_ERR(object##_led.class_dev))			\
-		led_classdev_unregister(&object##_led)
+	led_classdev_unregister(&object##_led)
 
 static void asus_led_exit(void)
 {
+	destroy_workqueue(led_workqueue);
 	ASUS_LED_UNREGISTER(mled);
 	ASUS_LED_UNREGISTER(tled);
 	ASUS_LED_UNREGISTER(pled);
 	ASUS_LED_UNREGISTER(rled);
 	ASUS_LED_UNREGISTER(gled);
-
-	destroy_workqueue(led_workqueue);
 }
 
 static void __exit asus_laptop_exit(void)

@@ -1135,29 +1132,42 @@ static int asus_led_init(struct device *dev)
 
 	rv = ASUS_LED_REGISTER(mled, dev);
 	if (rv)
-		return rv;
+		goto out;
 
 	rv = ASUS_LED_REGISTER(tled, dev);
 	if (rv)
-		return rv;
+		goto out1;
 
 	rv = ASUS_LED_REGISTER(rled, dev);
 	if (rv)
-		return rv;
+		goto out2;
 
 	rv = ASUS_LED_REGISTER(pled, dev);
 	if (rv)
-		return rv;
+		goto out3;
 
 	rv = ASUS_LED_REGISTER(gled, dev);
 	if (rv)
-		return rv;
+		goto out4;
 
 	led_workqueue = create_singlethread_workqueue("led_workqueue");
 	if (!led_workqueue)
-		return -ENOMEM;
+		goto out5;
 
 	return 0;
+out5:
+	rv = -ENOMEM;
+	ASUS_LED_UNREGISTER(gled);
+out4:
+	ASUS_LED_UNREGISTER(pled);
+out3:
+	ASUS_LED_UNREGISTER(rled);
+out2:
+	ASUS_LED_UNREGISTER(tled);
+out1:
+	ASUS_LED_UNREGISTER(mled);
+out:
+	return rv;
 }
 
 static int __init asus_laptop_init(void)
Signature · 1 line
1.5.3.GIT
#2 lore raw
In reply to Michal Piotrowski
Quoted 9 lines · Michal Piotrowski

ACPI

Subject : drivers/misc/asus-laptop.c:*: error: 'struct led_classdev' has no member named 'class_dev'
References : http://lkml.org/lkml/2007/7/22/299
Last known good : ?
Submitter : Gabriel C <[email protected]>

Caused-By       : ?
Handled-By      : ?
Status          : unknown

Here's a trivial patch for this one.
------------------------------------------------------------------------
asus-laptop: Sync with changes to led class

Driver was broken by commit f8a7c6fe14f556ca8eeddce258cb21392d0c3a2f
leds: Convert from struct class_device to struct device

Convert the LEDs class from struct class_device to struct device since class_device is scheduled for removal.

Use (struct led_classdev).dev instead of (struct led_classdev).class_dev

drivers/misc/asus-laptop.c +2 -2
diff -r 3f21143ca44c drivers/misc/asus-laptop.c
@@ -1067,8 +1067,8 @@ static void asus_backlight_exit(void)
 }
 
 #define  ASUS_LED_UNREGISTER(object)				\
-	if(object##_led.class_dev				\
-	   && !IS_ERR(object##_led.class_dev))			\
+	if(object##_led.dev					\
+	   && !IS_ERR(object##_led.dev))			\
 		led_classdev_unregister(&object##_led)
 
 static void asus_led_exit(void)
#3 lore raw
Quoted 11 lines · Trent Piepho

Here's a trivial patch for this one.
------------------------------------------------------------------------
asus-laptop: Sync with changes to led class

Driver was broken by commit f8a7c6fe14f556ca8eeddce258cb21392d0c3a2f
leds: Convert from struct class_device to struct device

Convert the LEDs class from struct class_device to struct device since class_device is scheduled for removal.

Use (struct led_classdev).dev instead of (struct led_classdev).class_dev

It doesn't fix the real bug in there - if you look carefully at the code, you'll see that we don't get to these checks if allocation fails halfway through (and we leak in that case) *and* these checks are not needed at all if failure happens elsewhere.

#4 lore raw
Quoted 12 lines · Al Viro
Quoted 10 lines · Trent Piepho

Here's a trivial patch for this one.
------------------------------------------------------------------------
asus-laptop: Sync with changes to led class

Driver was broken by commit f8a7c6fe14f556ca8eeddce258cb21392d0c3a2f
leds: Convert from struct class_device to struct device

Convert the LEDs class from struct class_device to struct device since class_device is scheduled for removal.

Quoted 6 lines
Quoted 1 line

Use (struct led_classdev).dev instead of (struct led_classdev).class_dev

It doesn't fix the real bug in there - if you look carefully at the code, you'll see that we don't get to these checks if allocation fails halfway through (and we leak in that case) *and* these checks are not needed at all if failure happens elsewhere.

leds: Allow led_classdev_unregister() to ignore unregistered drivers

Allow led_classdev_unregister() to return for devices that were already unregistered, never registered or failed to register to simplify drivers.

Fix error leftover from LED class struct device conversion (after the above, drivers don't need to look at the device structure).

Fix the leak where asus-led registration fails half way through.

Signed-off-by Richard Purdie <[email protected]>
drivers/leds/led-class.c +9 -2
index 4211293..68d26b8 100644
@@ -95,8 +95,10 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 
 	led_cdev->dev = device_create(leds_class, parent, 0, "%s",
 					    led_cdev->name);
-	if (unlikely(IS_ERR(led_cdev->dev)))
-		return PTR_ERR(led_cdev->dev);
+	if (unlikely(IS_ERR(led_cdev->dev))) {
+		rc = PTR_ERR(led_cdev->dev);
+		goto create_err;
+	}
 
 	dev_set_drvdata(led_cdev->dev, led_cdev);
 

@@ -132,6 +134,8 @@ err_out_led_list:
 #endif
 err_out:
 	device_unregister(led_cdev->dev);
+create_err:
+	led_cdev->dev = NULL;
 	return rc;
 }
 EXPORT_SYMBOL_GPL(led_classdev_register);

@@ -144,6 +148,9 @@ EXPORT_SYMBOL_GPL(led_classdev_register);
  */
 void led_classdev_unregister(struct led_classdev *led_cdev)
 {
+	if (!led_cdev->dev)
+		return;
+
 	device_remove_file(led_cdev->dev, &dev_attr_brightness);
 #ifdef CONFIG_LEDS_TRIGGERS
 	device_remove_file(led_cdev->dev, &dev_attr_trigger);
drivers/misc/asus-laptop.c +6 -12
index f753060..2bc4f6e 100644
@@ -1066,18 +1066,13 @@ static void asus_backlight_exit(void)
 		backlight_device_unregister(asus_backlight_device);
 }
 
-#define  ASUS_LED_UNREGISTER(object)				\
-	if(object##_led.class_dev				\
-	   && !IS_ERR(object##_led.class_dev))			\
-		led_classdev_unregister(&object##_led)
-
 static void asus_led_exit(void)
 {
-	ASUS_LED_UNREGISTER(mled);
-	ASUS_LED_UNREGISTER(tled);
-	ASUS_LED_UNREGISTER(pled);
-	ASUS_LED_UNREGISTER(rled);
-	ASUS_LED_UNREGISTER(gled);
+	led_classdev_unregister(&mled_led);
+	led_classdev_unregister(&tled_led);
+	led_classdev_unregister(&pled_led);
+	led_classdev_unregister(&rled_led);
+	led_classdev_unregister(&gled_led);
 
 	destroy_workqueue(led_workqueue);
 }

@@ -1226,9 +1221,8 @@ static int __init asus_laptop_init(void)
 	platform_driver_unregister(&asuspf_driver);
 
       fail_platform_driver:
-	asus_led_exit();
-
       fail_led:
+	asus_led_exit();
 	asus_backlight_exit();
 
       fail_backlight:
#5 lore raw
Quoted 1 line · Richard Purdie

Fix the leak where asus-led registration fails half way through.

Frankly, I don't think that any post-factum checks are needed. We should just bail out at the point of failure. See the patch posted earlier... BTW, your variant does destroy_workqueue() when no create_singlethreaded_workqueue() had been called. FWIW, here's the patch I'm talking about:

drivers/misc/asus-laptop.c +21 -11
index f753060..6b89854 100644
@@ -1067,19 +1067,16 @@ static void asus_backlight_exit(void)
 }
 
 #define  ASUS_LED_UNREGISTER(object)				\
-	if(object##_led.class_dev				\
-	   && !IS_ERR(object##_led.class_dev))			\
-		led_classdev_unregister(&object##_led)
+	led_classdev_unregister(&object##_led)
 
 static void asus_led_exit(void)
 {
+	destroy_workqueue(led_workqueue);
 	ASUS_LED_UNREGISTER(mled);
 	ASUS_LED_UNREGISTER(tled);
 	ASUS_LED_UNREGISTER(pled);
 	ASUS_LED_UNREGISTER(rled);
 	ASUS_LED_UNREGISTER(gled);
-
-	destroy_workqueue(led_workqueue);
 }
 
 static void __exit asus_laptop_exit(void)

@@ -1135,29 +1132,42 @@ static int asus_led_init(struct device *dev)
 
 	rv = ASUS_LED_REGISTER(mled, dev);
 	if (rv)
-		return rv;
+		goto out;
 
 	rv = ASUS_LED_REGISTER(tled, dev);
 	if (rv)
-		return rv;
+		goto out1;
 
 	rv = ASUS_LED_REGISTER(rled, dev);
 	if (rv)
-		return rv;
+		goto out2;
 
 	rv = ASUS_LED_REGISTER(pled, dev);
 	if (rv)
-		return rv;
+		goto out3;
 
 	rv = ASUS_LED_REGISTER(gled, dev);
 	if (rv)
-		return rv;
+		goto out4;
 
 	led_workqueue = create_singlethread_workqueue("led_workqueue");
 	if (!led_workqueue)
-		return -ENOMEM;
+		goto out5;
 
 	return 0;
+out5:
+	rv = -ENOMEM;
+	ASUS_LED_UNREGISTER(gled);
+out4:
+	ASUS_LED_UNREGISTER(pled);
+out3:
+	ASUS_LED_UNREGISTER(rled);
+out2:
+	ASUS_LED_UNREGISTER(tled);
+out1:
+	ASUS_LED_UNREGISTER(mled);
+out:
+	return rv;
 }
 
 static int __init asus_laptop_init(void)
Signature · 1 line
1.5.3.GIT
#6 lore raw
Quoted 8 lines · Al Viro
Quoted 1 line · Richard Purdie

Fix the leak where asus-led registration fails half way through.

Frankly, I don't think that any post-factum checks are needed. We should just bail out at the point of failure. See the patch posted earlier... BTW, your variant does destroy_workqueue() when no create_singlethreaded_workqueue() had been called. FWIW, here's the patch I'm talking about:

I saw that after I'd posted mine. I don't mind which we use...

I'm pleased the IS_ERR(object##_led.class_dev) is gone as that makes nasty assumptions about what the LED class does internally...

Cheers,

Richard