compat: fix uevent_suppress on 2.6.29 or older kernels

PATCH In review Grazvydas Ignotas 5 participants 9 messages Apr 22, 2010 – Apr 23, 2010

Share & tools

Grazvydas Ignotas

author

2010-04-22 13:54 UTC #0 lore raw

PATCH 1/1 compat: fix uevent_suppress on 2.6.29 or older kernels

Missing uevent_suppress is causing two uevents instead of one, which is confusing udev and sometimes causing firmware load to fail due to race condition, so let's add it.

Signed-off-by Grazvydas Ignotas <[email protected]>

---
 compat/compat_firmware_class.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

compat/compat_firmware_class.c +5 -1

index ce937a3..54ee8b9 100644
@@ -432,6 +432,8 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
 	dev_set_drvdata(f_dev, fw_priv);
 #if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
 	dev_set_uevent_suppress(f_dev, 1);
+#else
+	f_dev->uevent_suppress = 1;
 #endif
 	retval = device_register(f_dev);
 	if (retval) {

@@ -479,9 +481,11 @@ static int fw_setup_device(struct firmware *fw, struct device **dev_p,
 		goto error_unreg;
 	}
 
-#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
 	if (uevent)
+#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
 		dev_set_uevent_suppress(f_dev, 0);
+#else
+		f_dev->uevent_suppress = 0;
 #endif
 	*dev_p = f_dev;
 	goto out;

Signature · 1 line

1.7.0.2

Walter Goldens

2010-04-22 15:17 UTC #1 lore raw

I reported this nearly a month ago here: http://comments.gmane.org/gmane.linux.kernel.wireless.general/48401

Had a chance to test your patch and everything's fine now. This bug has been existent since January. Great job!

Walter.

Quoted 56 lines · Grazvydas Ignotas

From: Grazvydas Ignotas <[email protected]>
Subject: [PATCH] compat: fix uevent_suppress on 2.6.29 or older kernels
To: "Luis R. Rodriguez" <[email protected]>
Cc: [email protected], "Grazvydas Ignotas" <[email protected]>
Date: Thursday, April 22, 2010, 9:54 AM
Missing uevent_suppress is causing
two uevents instead of one, which is
confusing udev and sometimes causing firmware load to fail
due to race
condition, so let's add it.
Signed-off-by Grazvydas Ignotas <[email protected]>
---
 compat/compat_firmware_class.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
compat/compat_firmware_class.c +2 -0
index ce937a3..54ee8b9 100644
@@ -432,6 +432,8 @@ static int fw_register_device(struct
 device **dev_p, const char *fw_name,
     dev_set_drvdata(f_dev, fw_priv);
 #if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
     dev_set_uevent_suppress(f_dev, 1);
+#else
+    f_dev->uevent_suppress = 1;
 #endif
     retval = device_register(f_dev);
if (retval) {
compat/compat_firmware_class.c +3 -1
@@ -479,9 +481,11 @@ static int fw_setup_device(struct
 firmware *fw, struct device **dev_p,
         goto error_unreg;
     }
 
-#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
     if (uevent)
+#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
        
 dev_set_uevent_suppress(f_dev, 0);
+#else
+       
 f_dev->uevent_suppress = 0;
#endif
*dev_p = f_dev;
goto out;
Signature · 7 lines
1.7.0.2

--
To unsubscribe from this list: send the line "unsubscribe
linux-wireless" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis R. Rodriguez

2010-04-22 16:34 UTC #2 lore raw

In reply to Grazvydas Ignotas

Quoted 14 lines · Grazvydas Ignotas

Missing uevent_suppress is causing two uevents instead of one, which is confusing udev and sometimes causing firmware load to fail due to race condition, so let's add it.
Signed-off-by Grazvydas Ignotas <[email protected]>
---
compat/compat_firmware_class.c | 6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
compat/compat_firmware_class.c +0 -0
index ce937a3..54ee8b9 100644

compat/compat_firmware_class.c static int fw_register_device(struct device **dev_p, const char *fw_name, · line 437

Quoted 6 lines

compat/compat_firmware_class.c +2 -0

@@ -432,6 +432,8 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
        dev_set_drvdata(f_dev, fw_priv);
  #if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
        dev_set_uevent_suppress(f_dev, 1);
+#else
+       f_dev->uevent_suppress = 1;
  #endif

How was this compiling for older kernels before then?

Luis

Pavel Roskin

2010-04-22 16:42 UTC #3 lore raw

Quoted 1 line · Luis R. Rodriguez

How was this compiling for older kernels before then?

Compiling was fine, but loading the firmware was failing sometimes. I'm glad somebody figured it out. Thank you, Grazvydas!

I would prefer to have replacement functions in compat-2.6.30.h rather than ifdefs in the code. We might want to resync compat_firmware with the kernel one day, and ifdefs would stand in the way.

Signature · 2 lines

Regards,
Pavel Roskin

Luis R. Rodriguez

2010-04-22 20:33 UTC #4 lore raw

Quoted 6 lines · Pavel Roskin

Quoted 1 line · Luis R. Rodriguez
How was this compiling for older kernels before then?
Compiling was fine, but loading the firmware was failing sometimes. I'm glad somebody figured it out. Thank you, Grazvydas!

OK Applied. I'm still puzzled, if it compiled, then that means dev_set_uevent_suppress() was being defined somehow for older kernels.

Quoted 3 lines

I would prefer to have replacement functions in compat-2.6.30.h rather than ifdefs in the code. We might want to resync compat_firmware with the kernel one day, and ifdefs would stand in the way.

Good point, I merged the patch for now given that that file does't change that often upstream.

Luis

Grazvydas Ignotas

author

2010-04-22 20:47 UTC #5 lore raw

Quoted 10 lines · Luis R. Rodriguez

Quoted 6 lines · Pavel Roskin
Quoted 1 line · Luis R. Rodriguez
How was this compiling for older kernels before then?
Compiling was fine, but loading the firmware was failing sometimes. I'm glad somebody figured it out. Thank you, Grazvydas!
OK Applied. I'm still puzzled, if it compiled, then that means dev_set_uevent_suppress() was being defined somehow for older kernels.

there was already ifdef, old code:

 #if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
       dev_set_uevent_suppress(f_dev, 1);

#endif

new code:

 #if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
       dev_set_uevent_suppress(f_dev, 1);
#else
       f_dev->uevent_suppress = 1;

#endif

Quoted 9 lines

Quoted 3 lines
I would prefer to have replacement functions in compat-2.6.30.h rather than ifdefs in the code. We might want to resync compat_firmware with the kernel one day, and ifdefs would stand in the way.
Good point, I merged the patch for now given that that file does't change that often upstream.
Luis

Johannes Berg

2010-04-23 05:57 UTC #6 lore raw

Quoted 15 lines · Grazvydas Ignotas

Quoted 10 lines · Luis R. Rodriguez
Quoted 6 lines · Pavel Roskin
Quoted 1 line · Luis R. Rodriguez
How was this compiling for older kernels before then?
Compiling was fine, but loading the firmware was failing sometimes. I'm glad somebody figured it out. Thank you, Grazvydas!
OK Applied. I'm still puzzled, if it compiled, then that means dev_set_uevent_suppress() was being defined somehow for older kernels.
there was already ifdef, old code:
 #if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
       dev_set_uevent_suppress(f_dev, 1);

Quoted 8 lines

#endif

new code:

 #if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30))
       dev_set_uevent_suppress(f_dev, 1);
#else
       f_dev->uevent_suppress = 1;

#endif

And the argument is that the new code should be

	dev_set_uevent_suppress(f_dev, 1);

and some header file should provide

static inline void dev_set_uevent_suppress(...)

{
	f_dev->uevent_suppress = 1;
}

which makes perfect sense. Although Luis merged it, a fix would be useful.

Luis, a patch changing firmware_class.c just entered the tree :P I made it.

johannes

Luis R. Rodriguez

2010-04-23 06:01 UTC #7 lore raw

Quoted 36 lines · Johannes Berg

Quoted 17 lines · Grazvydas Ignotas
Quoted 10 lines · Luis R. Rodriguez
Quoted 6 lines · Pavel Roskin
Quoted 1 line · Luis R. Rodriguez
How was this compiling for older kernels before then?
Compiling was fine, but loading the firmware was failing sometimes. I'm glad somebody figured it out. Thank you, Grazvydas!
OK Applied. I'm still puzzled, if it compiled, then that means dev_set_uevent_suppress() was being defined somehow for older kernels.
there was already ifdef, old code:
#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30)) dev_set_uevent_suppress(f_dev, 1); #endif
Quoted 6 lines
new code:
#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,30)) dev_set_uevent_suppress(f_dev, 1); #else
f_dev->uevent_suppress = 1;
#endif
And the argument is that the new code should be
dev_set_uevent_suppress(f_dev, 1);
and some header file should provide
static inline void dev_set_uevent_suppress(...)
{
f_dev->uevent_suppress = 1;
}

Quoted 5 lines

which makes perfect sense. Although Luis merged it, a fix would be useful.
Luis, a patch changing firmware_class.c just entered the tree :P I made it.

Damn you, had to be you :)

Luis

Pavel Roskin

2010-04-22 20:51 UTC #8 lore raw

In reply to Luis R. Rodriguez

Quoted 10 lines · Luis R. Rodriguez

Quoted 6 lines · Pavel Roskin
Quoted 1 line · Luis R. Rodriguez
How was this compiling for older kernels before then?
Compiling was fine, but loading the firmware was failing sometimes. I'm glad somebody figured it out. Thank you, Grazvydas!
OK Applied. I'm still puzzled, if it compiled, then that means dev_set_uevent_suppress() was being defined somehow for older kernels.

It's wasn't defined. The function just wasn't called for older kernels. "#if" was there; the patch added "#else".

It's a great example how simply-minded commenting out missing functions can lead to a hard problem, a race condition.

Signature · 2 lines

Regards,
Pavel Roskin