Messages in this thread | | | Date | Wed, 13 Jun 2007 22:34:07 +0530 | From | "Satyam Sharma" <> | Subject | Re: [RFC][PATCH -mm take5 5/7] switch function of netpoll |
| |
Hi Keiichi,
On 6/13/07, Keiichi KII <k-keiichi@bx.jp.nec.com> wrote: > From: Keiichi KII <k-keiichi@bx.jp.nec.com> > > This patch contains switch function of netpoll. > > If "enabled" attribute of certain port is '1', this port is used > and the configurations of this port are unable to change. > > If "enabled" attribute of certain port is '0', this port isn't used > and the configurations of this port are able to change. > > -+- /sys/class/misc/ > |-+- netconsole/ > |-+- port1/ > | |--- id [r--r--r--] id > | |--- enabled [rw-r--r--] 0: disable 1: enable, writable > | ... > |--- port2/ > ... > > Signed-off-by: Keiichi KII <k-keiichi@bx.jp.nec.com> > Signed-off-by: Takayoshi Kochi <t-kochi@bq.jp.nec.com> > --- > Index: mm/drivers/net/netconsole.c > =================================================================== > --- mm.orig/drivers/net/netconsole.c > +++ mm/drivers/net/netconsole.c > @@ -71,6 +71,7 @@ struct netconsole_target { > struct list_head list; > struct kobject obj; > int id; > + int enabled; > struct netpoll np; > };
I really think you need to document/comment the struct members. The newly introduced "enabled" for example, is serving a dual-purpose in your code here (like you mention in the Changelog). It's used not only to enable/disable a target, but also to ensure atomicity when changing the (multiple) configurable parameters of a given target ... the first purpose is self-evident from the name, but the second is non-obvious. A sysfs node is also a userspace interface, so some explicit mention of this is required.
Satyam - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |