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(-)
diff --git a/compat/compat_firmware_class.c b/compat/compat_firmware_class.c
index ce937a3..54ee8b9 100644
--- a/compat/compat_firmware_class.c
+++ b/compat/compat_firmware_class.c
@@ -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;
--
1.7.0.2
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.
--- On Thu, 4/22/10, Grazvydas Ignotas <[email protected]> wrote:
> 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(-)
>
> diff --git a/compat/compat_firmware_class.c
> b/compat/compat_firmware_class.c
> index ce937a3..54ee8b9 100644
> --- a/compat/compat_firmware_class.c
> +++ b/compat/compat_firmware_class.c
> @@ -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;
> --
> 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
>
On Thu, 2010-04-22 at 23:47 +0300, Grazvydas Ignotas wrote:
> On Thu, Apr 22, 2010 at 11:33 PM, Luis R. Rodriguez <[email protected]> wrote:
> > On Thu, Apr 22, 2010 at 9:42 AM, Pavel Roskin <[email protected]> wrote:
> >> On Thu, 2010-04-22 at 09:33 -0700, Luis R. Rodriguez wrote:
> >>
> >>> 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
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
On Thu, Apr 22, 2010 at 6:54 AM, Grazvydas Ignotas <[email protected]> wrote:
> 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(-)
>
> diff --git a/compat/compat_firmware_class.c b/compat/compat_firmware_class.c
> index ce937a3..54ee8b9 100644
> --- a/compat/compat_firmware_class.c
> +++ b/compat/compat_firmware_class.c
> @@ -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
On Thu, 2010-04-22 at 13:33 -0700, Luis R. Rodriguez wrote:
> On Thu, Apr 22, 2010 at 9:42 AM, Pavel Roskin <[email protected]> wrote:
> > On Thu, 2010-04-22 at 09:33 -0700, Luis R. Rodriguez wrote:
> >
> >> 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.
--
Regards,
Pavel Roskin
On Thu, Apr 22, 2010 at 10:57 PM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2010-04-22 at 23:47 +0300, Grazvydas Ignotas wrote:
>> On Thu, Apr 22, 2010 at 11:33 PM, Luis R. Rodriguez <[email protected]> wrote:
>> > On Thu, Apr 22, 2010 at 9:42 AM, Pavel Roskin <[email protected]> wrote:
>> >> On Thu, 2010-04-22 at 09:33 -0700, Luis R. Rodriguez wrote:
>> >>
>> >>> 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
>
> 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.
Damn you, had to be you :)
Luis
On Thu, Apr 22, 2010 at 11:33 PM, Luis R. Rodriguez <[email protected]> wrote:
> On Thu, Apr 22, 2010 at 9:42 AM, Pavel Roskin <[email protected]> wrote:
>> On Thu, 2010-04-22 at 09:33 -0700, Luis R. Rodriguez wrote:
>>
>>> 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
>> 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
>
On Thu, Apr 22, 2010 at 9:42 AM, Pavel Roskin <[email protected]> wrote:
> On Thu, 2010-04-22 at 09:33 -0700, Luis R. Rodriguez wrote:
>
>> 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.
> 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
On Thu, 2010-04-22 at 09:33 -0700, Luis R. Rodriguez wrote:
> 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.
--
Regards,
Pavel Roskin