Hello,
I've been toying with hostap_pci and found a bug that is triggered when
you unload/reload the module.
If you don't have PRISM2_NO_PROCFS_DEBUG defined (which is the default,
as per hostap_config.h), the driver creates a "registers" file in /proc
used to dump PRISM registers (function prism2_init_local_data):
#ifndef PRISM2_NO_PROCFS_DEBUG
create_proc_read_entry("registers", 0, local->proc,
prism2_registers_proc_read, local);
#endif /* PRISM2_NO_PROCFS_DEBUG */
Note that create_proc_read_entry is being called with local->proc which
isn't defined at the time: "registers" ends up being created as
/proc/registers.
When hostap_pci is done initializing the hardware, it calls
hostap_hw_ready which in turns creates all the files in /proc through
hostap_init_proc (hostap_proc.c):
local->proc = NULL;
if (hostap_proc == NULL) {
printk(KERN_WARNING "%s: hostap proc directory not created\n",
local->dev->name);
return;
}
local->proc = proc_mkdir(local->ddev->name, hostap_proc);
So far so good, well sort of... When you unload the module,
prism2_free_local_data does this:
#ifndef PRISM2_NO_PROCFS_DEBUG
if (local->proc != NULL)
remove_proc_entry("registers", local->proc);
#endif /* PRISM2_NO_PROCFS_DEBUG */
hostap_remove_proc(local);
Problem is local->proc has now been overwritten by hostap_init_proc
meaning "registers" isn't removed. When you reload the module you get
an error telling you "registers" already exists (and you then get
duplicate "registers" files in /proc).
So in a nutshell "registers" should be created after calling
hostap_init_proc (this way it'll be created where it belongs -
/proc/net/hostap/%s) making it removable at module unload.
Please apply the patch below which does just that: it creates "register"
in hostap_hw_ready right after creating all the proc entries.
Best,
Mathieu
Signed-off-by: Mathieu Chouquet-Stringer <[email protected]>
diff --git a/drivers/net/wireless/hostap/hostap_hw.c b/drivers/net/wireless/hostap/hostap_hw.c
index 7be68db..454fcb4 100644
--- a/drivers/net/wireless/hostap/hostap_hw.c
+++ b/drivers/net/wireless/hostap/hostap_hw.c
@@ -3276,11 +3276,6 @@ while (0)
}
printk(KERN_INFO "%s: Registered netdevice %s\n", dev_info, dev->name);
-#ifndef PRISM2_NO_PROCFS_DEBUG
- create_proc_read_entry("registers", 0, local->proc,
- prism2_registers_proc_read, local);
-#endif /* PRISM2_NO_PROCFS_DEBUG */
-
hostap_init_data(local);
return dev;
@@ -3307,6 +3302,10 @@ static int hostap_hw_ready(struct net_device *dev)
netif_carrier_off(local->ddev);
}
hostap_init_proc(local);
+#ifndef PRISM2_NO_PROCFS_DEBUG
+ create_proc_read_entry("registers", 0, local->proc,
+ prism2_registers_proc_read, local);
+#endif /* PRISM2_NO_PROCFS_DEBUG */
hostap_init_ap_proc(local);
return 0;
}
--
Mathieu Chouquet-Stringer [email protected]
The sun itself sees not till heaven clears.
-- William Shakespeare --
On Mon, May 12, 2008 at 07:43:46AM -0400, Pavel Roskin wrote:
> On Tue, 2008-05-06 at 17:23 +0200, Mathieu Chouquet-Stringer wrote:
>
> > Problem is local->proc has now been overwritten by hostap_init_proc
> > meaning "registers" isn't removed. When you reload the module you get
> > an error telling you "registers" already exists (and you then get
> > duplicate "registers" files in /proc).
>
> I've seen it many times. Thanks for tracking it down! I've tested the
> patch with PCI and PCMCIA devices, and everything appears to be
> correct. /proc/registers is not created, "registers" is created
> under /proc/net/hostap/wlan0/, no more warnings.
>
> > Signed-off-by: Mathieu Chouquet-Stringer <[email protected]>
>
> Acked-off-by: Pavel Roskin <[email protected]>
>
> If a more concise description needs to be written, I can do it.
If you don't mind, I would appreciate a more concise description.
I appreciate the full description of the bug, but I think it is a
bit much for the changelog as-is.
Thanks!
John
--
John W. Linville
[email protected]
On Tue, 13 May 2008, "John W. Linville" <[email protected]> wrote:
> On Mon, May 12, 2008 at 07:43:46AM -0400, Pavel Roskin wrote:
>> On Tue, 2008-05-06 at 17:23 +0200, Mathieu Chouquet-Stringer wrote:
>> > Signed-off-by: Mathieu Chouquet-Stringer <[email protected]>
>>
>> Acked-off-by: Pavel Roskin <[email protected]>
>>
>> If a more concise description needs to be written, I can do it.
>
> If you don't mind, I would appreciate a more concise description.
> I appreciate the full description of the bug, but I think it is a
> bit much for the changelog as-is.
What about something like this:
"registers" was wrongly created under /proc which was generating warning
during module removal and warning of duplicates objects in /proc after
module reloading. The file now lies in /proc/net/hostap, where it was
meant to be.
--
Mathieu Chouquet-Stringer [email protected]
The sun itself sees not till heaven clears.
-- William Shakespeare --
On Fri, May 16, 2008 at 09:22:25AM -0400, Pavel Roskin wrote:
> Sorry, I should have set you a copy of the patch with my description.
> The patch is in linux-wireless already.
> Thanks again for fixing that bug.
No problem at all, thanks for taking the time to write this up!!!
--
Mathieu Chouquet-Stringer [email protected]
The sun itself sees not till heaven clears.
-- William Shakespeare --
On Tue, 2008-05-06 at 17:23 +0200, Mathieu Chouquet-Stringer wrote:
> Problem is local->proc has now been overwritten by hostap_init_proc
> meaning "registers" isn't removed. When you reload the module you get
> an error telling you "registers" already exists (and you then get
> duplicate "registers" files in /proc).
I've seen it many times. Thanks for tracking it down! I've tested the
patch with PCI and PCMCIA devices, and everything appears to be
correct. /proc/registers is not created, "registers" is created
under /proc/net/hostap/wlan0/, no more warnings.
> Signed-off-by: Mathieu Chouquet-Stringer <[email protected]>
Acked-off-by: Pavel Roskin <[email protected]>
If a more concise description needs to be written, I can do it.
--
Regards,
Pavel Roskin