[RFC] EDAC a new sysfs 'subsystem' under /sys/devices

[RFC] EDAC a new sysfs 'subsystem' under /sys/devices

[Doug Thompson]

EDAC is for detecting and logging memory ECC, PCI
Parity and (future) other hardware errors.

In my process of getting edac ready for kernel
submission:

I have been exploring mechanisms for placing EDAC
controls and attribute files into sysfs for a few days
now.

In the process, I "discovered" how sysfs subystems are
currently used. I have determined that EDAC itself
should be a subsystem, since more than one type of
EDAC device can be created.

I made a copy of drivers/base/sys.c to a new file,
drivers/base/edac.c (and corresponding .h file) and
refactored it to become a 'edacdev' subsystem.

I have mounted edac to: /sys/devices/edac because the
/sys/device/system subsystem is not exported (at the
moment. I suppose that could be changed). 

I now have:

/sys/devices/edac/mc/mc0/csrow0 kobjects working and
am adding attribute files to their respective
kobjects.

My RFC is:

Should I leave edac under /sys/devices OR refactor
drivers/base/sys.c and export the "system" subsystem
and mount edac under /sys/devices/system? 

Another RFC is:

Should EDAC really have a "new" subsystem in the
kernel?  (I believe it fits bets, but am looking for
alternatives)

It requires a new drivers/base/edac.c (and .h file +
plumbing).  I would like to see that, as it really
works nicely. The EDAC module currently in the -mm
tree will depend  on it, in order to implement the
requested sysfs features. (Other options are possible
I suppose)

The only problem I ran into was that I needed further
subdirectories under 'mc/mc0', and I had to resort to
using kobject_register() since the subsystem didn't
have methods for such creation. Yet that code now
works nicely.


I suppose I am looking for verification of the
direction I am progressing in.

Thanks for any input

doug t




"If you think Education is expensive, just try Ignorance"

"Don't tell people HOW to do things, tell them WHAT you
want and they will surprise you with their ingenuity."
                   Gen George Patton

Re: [RFC] EDAC a new sysfs 'subsystem' under /sys/devices

[Greg KH]

On Tue, Nov 22, 2005 at 03:57:44PM -0800, Doug Thompson wrote:
> EDAC is for detecting and logging memory ECC, PCI
> Parity and (future) other hardware errors.
> 
> In my process of getting edac ready for kernel
> submission:
> 
> I have been exploring mechanisms for placing EDAC
> controls and attribute files into sysfs for a few days
> now.
> 
> In the process, I "discovered" how sysfs subystems are
> currently used. I have determined that EDAC itself
> should be a subsystem, since more than one type of
> EDAC device can be created.
> 
> I made a copy of drivers/base/sys.c to a new file,
> drivers/base/edac.c (and corresponding .h file) and
> refactored it to become a 'edacdev' subsystem.
> 
> I have mounted edac to: /sys/devices/edac because the
> /sys/device/system subsystem is not exported (at the
> moment. I suppose that could be changed). 

Yes, that would be trivial to do :)

> I now have:
> 
> /sys/devices/edac/mc/mc0/csrow0 kobjects working and
> am adding attribute files to their respective
> kobjects.
> 
> My RFC is:
> 
> Should I leave edac under /sys/devices OR refactor
> drivers/base/sys.c and export the "system" subsystem
> and mount edac under /sys/devices/system? 

Please put it under /sys/devices/system.

Also, see the patch on lkml from Pat Mochel which might make your deep
sysfs trees much easier to create and use.

> Another RFC is:
> 
> Should EDAC really have a "new" subsystem in the
> kernel?  (I believe it fits bets, but am looking for
> alternatives)

Why not just a system device?

> It requires a new drivers/base/edac.c (and .h file +
> plumbing).  I would like to see that, as it really
> works nicely. The EDAC module currently in the -mm
> tree will depend  on it, in order to implement the
> requested sysfs features. (Other options are possible
> I suppose)

No, you should not be adding stuff to drivers/base.

> The only problem I ran into was that I needed further
> subdirectories under 'mc/mc0', and I had to resort to
> using kobject_register() since the subsystem didn't
> have methods for such creation. Yet that code now
> works nicely.

See the patch from Pat for how to do this easier.  But even without that
patch, creating new kobjects is one way to do it.

thanks,

greg k-h

Re: [RFC] EDAC a new sysfs 'subsystem' under /sys/devices

[Doug Thompson]

--- Greg KH <greg@kroah.com> wrote:

> On Tue, Nov 22, 2005 at 03:57:44PM -0800, Doug
> Thompson wrote:

> > I have mounted edac to: /sys/devices/edac because
> the
> > /sys/device/system subsystem is not exported
> 
> Yes, that would be trivial to do :)

I have changed to using /sys/devices/system for the
edac_subsystem.  I had to remove the 'static' for the
declaration of system_subsystem in drivers/base/sys.c,
and linked to it instead of the 'devices_subsys'
variable.

> 
> > I now have:
> > 
> > /sys/devices/system/edac/mc/mc0/csrow0 kobjects
working
>
> Please put it under /sys/devices/system.

With the mod:

/sys/devices/system/edac

subsystem is working and I can add:

mc/mc0/csrows0/...

to it.

> Also, see the patch on lkml from Pat Mochel which
> might make your deep
> sysfs trees much easier to create and use.
> 
> > Another RFC is:
> > 
> > Should EDAC really have a "new" subsystem in the
> > kernel?  (I believe it fits bets, but am looking
> for
> > alternatives)
> 
> Why not just a system device?

One reason is the naming convention used:

creating an 'edac' class followed by adding a edac
device creates the following name:

/sys/devices/system/edac/edac0/...
/sys/devices/system/edac/edac1/...

whereas I desire:

/system/devices/system/edac/mc/mc0/...

I would just register a class 'edac' and then all else
would be custom (or utilize the patch you have
mentioned)

Another reason is that EDAC is not necessarily just
for memory ECC checking only.  The EDAC module
currently has 1) ECC checking and 2) PCI parity
checking (not driver notification) on bus devices. For
the future, other EDAC types of devices can be added.

If I add an EDAC kobject 'subsystem' in the base, then
future EDAC type devices can be added and be decoupled
from the memory ECC EDAC module.

(This poses the question: if the current EDAC 
module's name is too generic just for the memory
reporter maybe it should should be edac_mem instead or
some such)

If the EDAC module "creates" the EDAC device under
/sys/devices/system, then that module will "own" that
name and future devices will need to be added to that
module. There currently is no "core" edac sysfs
subystem to provide the abstraction.

I guess the issue becomes:  Is the EDAC module JUST
for memory ECC and PCI parity reporting, in utilizing
sysfs? 

Should other future EDAC devices then also be added to
the EDAC module in order to utilize the sysfs
/sys/devices/system/edac device?

Or should they be modular in their own right and plug
into a sysfs EDAC subsystem?

I might be wrong, or need redirection, but I see this
as a clean model by using an abstraction of EDAC type
devices in sysfs.


> 
> > It requires a new drivers/base/edac.c (and .h file
> +
> > plumbing).

> No, you should not be adding stuff to drivers/base.

I understand. I did look at other options, but I saw
more coupling in the others. If there were a
de-coupled EDAC sysfs 'core or base class' (ie EDAC
subsystem), then the memory ECC reporter could plug in
cleanly. The PCI Parity report could plug in. Other
future, EDAC reporters could plug in as well.


> 
> > The only problem I ran into was that I needed
> further
> > subdirectories under 'mc/mc0', and I had to resort
> to
> > using kobject_register() since the subsystem
> didn't
> > have methods for such creation. Yet that code now
> > works nicely.
> 
> See the patch from Pat for how to do this easier. 
> But even without that
> patch, creating new kobjects is one way to do it.

Ok, I will take a look at the patch to see its options
are. I look forward to feedback.

> 
> thanks,
> 
> greg k-h
> 

my thanks

doug t


"If you think Education is expensive, just try Ignorance"

"Don't tell people HOW to do things, tell them WHAT you
want and they will surprise you with their ingenuity."
                   Gen George Patton