RE: 'rmmod button' doesn't remove /proc/acpi/button

RE: 'rmmod button' doesn't remove /proc/acpi/button

[Grover, Andrew]

> From: Cagle, John (ISS-Houston) [mailto:john.cagle-VXdhtT5mjnY@public.gmane.org] 
> I found a minor bug in button.c's acpi_button_exit() routine. 
>  It doesn't remove the top-level 'button' directory when 
> unloading the module, and if you insmod/rmmod multiple times, 
> you'll end up with lots of 'button' directories in /proc/acpi.

This actually reveals a bug in *all* of the client drivers. They all depend
upon a global variable with the name acpi_{battery,button,etc}_dir to see if
they need to make their class proc entry when adding their first device.
This has the benefit of only creating a "button" directory if there actually
is a button, but has the bad feature of not working for insmod...because the
global variables go away, and so we blindly add the proc entries again.

I think the best thing is to make the semantics "the proc dir is there if
the driver is loaded, even with no devices present" instead of "the proc dir
is there only if an actual instance of the device is present".

This allows us to move the creation of the dir to the driver init function,
instead of the device add function.

Does that sound right?

Regards -- Andy


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: RE: 'rmmod button' doesn't remove /proc/acpi/button

[Dominik Brodowski]

On Mon, Nov 11, 2002 at 12:01:35PM -0800, Grover, Andrew wrote:
> > From: Cagle, John (ISS-Houston) [mailto:john.cagle-VXdhtT5mjnY@public.gmane.org] 
> > I found a minor bug in button.c's acpi_button_exit() routine. 
> >  It doesn't remove the top-level 'button' directory when 
> > unloading the module, and if you insmod/rmmod multiple times, 
> > you'll end up with lots of 'button' directories in /proc/acpi.
> 
> This actually reveals a bug in *all* of the client drivers. They all depend
> upon a global variable with the name acpi_{battery,button,etc}_dir to see if
> they need to make their class proc entry when adding their first device.
> This has the benefit of only creating a "button" directory if there actually
> is a button, but has the bad feature of not working for insmod...because the
> global variables go away, and so we blindly add the proc entries again.
> 
> I think the best thing is to make the semantics "the proc dir is there if
> the driver is loaded, even with no devices present" instead of "the proc dir
> is there only if an actual instance of the device is present".
> 
> This allows us to move the creation of the dir to the driver init function,
> instead of the device add function.

Actually, I think this would confuse users even more than currently: If an
OSPM-module is loaded, but no entry is found in the DSDT (or anywhere), it
is still loaded; doesn't complain in dmesg (actually, doesn't add any output
to dmesg) and so causes some users to think "well, everything seems to be
working fine". Now, if the proc dir is added even if no such device is
added, the confusion enlargens.

Instead, I'd propose adding an acpi_{battery,button,etc}_dir_count, and when
this reaches zero in acpi_button_remove_fs(), for example, remove_proc_entry
the acpi_button_dir.

	Dominik 
who is very busy at the moment, else I'd write a patch :-)


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: RE: 'rmmod button' doesn't remove /proc/acpi/button

[Cagle, John (ISS-Houston)]

> From: Grover, Andrew [mailto:andrew.grover-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org] 
> > From: Cagle, John (ISS-Houston) [mailto:john.cagle-VXdhtT5mjnY@public.gmane.org]
> > I found a minor bug in button.c's acpi_button_exit() routine. 
> >  It doesn't remove the top-level 'button' directory when 
> > unloading the module, and if you insmod/rmmod multiple times, 
> > you'll end up with lots of 'button' directories in /proc/acpi.
> 
> This actually reveals a bug in *all* of the client drivers. 

Actually, all the other client drivers had code to remove
the class proc entry -- it was only missing from button.c's
unload logic.

> They all depend upon a global variable with the name 
> acpi_{battery,button,etc}_dir to see if they need to make 
> their class proc entry when adding their first device. This 
> has the benefit of only creating a "button" directory if 
> there actually is a button, but has the bad feature of not 
> working for insmod...because the global variables go away, 
> and so we blindly add the proc entries again.
> 
> I think the best thing is to make the semantics "the proc dir 
> is there if the driver is loaded, even with no devices 
> present" instead of "the proc dir is there only if an actual 
> instance of the device is present".
> 
> This allows us to move the creation of the dir to the driver 
> init function, instead of the device add function.
> 
> Does that sound right?

That does sound like a cleaner approach.  Let me try it out
and see what the patch looks like.

> 
> Regards -- Andy

Regards,
John
--------------------------------
John Cagle     john.cagle-VXdhtT5mjnY@public.gmane.org
Principal Member Technical Staff
   Industry Standard Servers
    Hewlett-Packard Company
    http://www.hp.com/linux
         281-514-9167


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: RE: 'rmmod button' doesn't remove /proc/acpi/button

[Grover, Andrew]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

> From: Cagle, John (ISS-Houston) [mailto:john.cagle-VXdhtT5mjnY@public.gmane.org] 
> Actually, all the other client drivers had code to remove
> the class proc entry -- it was only missing from button.c's 
> unload logic.

I think they're not quite right...

> > I think the best thing is to make the semantics "the proc dir
> > is there if the driver is loaded, even with no devices 
> > present" instead of "the proc dir is there only if an actual 
> > instance of the device is present".
> > This allows us to move the creation of the dir to the driver
> > init function, instead of the device add function.

> That does sound like a cleaner approach.  Let me try it out
> and see what the patch looks like.

I've generated the attached patch (against 2.4-acpi but should apply to
2.5). Does it fix the problem? If not, it should at least be a starting
point.

Regards -- Andy


[-- Attachment #2: proc_cng.diff.gz --]
[-- Type: application/octet-stream, Size: 1799 bytes --]

RE: RE: 'rmmod button' doesn't remove /proc/acpi/button

[Cagle, John (ISS-Houston)]

Andy,

Your patch seems OK for creating/removing the class directories at the
appropriate times.  I've run into some other issues, but I'll address
those in another thread.  In response to Dominik's email, I don't think
having empty class directories is confusing to the user, since there's
already precedence for having filesystem entries with nothing behind
them.  For instance, just because /dev/hdb exists, doesn't mean that you
have a 2nd IDE hard drive.

Thanks,
John

> -----Original Message-----
> From: Grover, Andrew [mailto:andrew.grover-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org] 
> Sent: Monday, November 11, 2002 2:48 PM
> To: Cagle, John (ISS-Houston)
> Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> Subject: RE: [ACPI] RE: 'rmmod button' doesn't remove 
> /proc/acpi/button
> 
> 
> > From: Cagle, John (ISS-Houston) [mailto:john.cagle-VXdhtT5mjnY@public.gmane.org]
> > Actually, all the other client drivers had code to remove
> > the class proc entry -- it was only missing from button.c's 
> > unload logic.
> 
> I think they're not quite right...
> 
> > > I think the best thing is to make the semantics "the proc dir is 
> > > there if the driver is loaded, even with no devices 
> present" instead 
> > > of "the proc dir is there only if an actual instance of 
> the device 
> > > is present". This allows us to move the creation of the 
> dir to the 
> > > driver init function, instead of the device add function.
> 
> > That does sound like a cleaner approach.  Let me try it out and see 
> > what the patch looks like.
> 
> I've generated the attached patch (against 2.4-acpi but 
> should apply to 2.5). Does it fix the problem? If not, it 
> should at least be a starting point.
> 
> Regards -- Andy


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf