[PATCH] drop some attibutes from the FC transport class

[PATCH] drop some attibutes from the FC transport class

[Christoph Hellwig]

I think the hardware_version, firmware_version, rom_version and
driver_version don't belong into the FC transport class, there's
nothign specific to FC or even SCSI specific in them.

If you want these attributes talk to Greg about fiding a place in
the common driver model code for them.

p.s. I was surprised to find no Emulex copyright notices in the FC
transport implementation fields, I suspect you should add them.


--- 1.13/drivers/scsi/scsi_transport_fc.c	2005-01-12 17:08:53 +01:00
+++ edited/drivers/scsi/scsi_transport_fc.c	2005-01-19 18:17:03 +01:00
@@ -270,16 +270,8 @@ static int fc_setup_host_transport_attrs
 		sizeof(fc_host_symbolic_name(shost)));
 	fc_host_supported_speeds(shost) = FC_PORTSPEED_UNKNOWN;
 	fc_host_maxframe_size(shost) = -1;
-	memset(fc_host_hardware_version(shost), 0,
-		sizeof(fc_host_hardware_version(shost)));
-	memset(fc_host_firmware_version(shost), 0,
-		sizeof(fc_host_firmware_version(shost)));
 	memset(fc_host_serial_number(shost), 0,
 		sizeof(fc_host_serial_number(shost)));
-	memset(fc_host_opt_rom_version(shost), 0,
-		sizeof(fc_host_opt_rom_version(shost)));
-	memset(fc_host_driver_version(shost), 0,
-		sizeof(fc_host_driver_version(shost)));
 
 	fc_host_port_id(shost) = -1;
 	fc_host_port_type(shost) = FC_PORTTYPE_UNKNOWN;
@@ -546,11 +538,7 @@ fc_private_host_rd_attr_cast(node_name, 
 fc_private_host_rd_attr_cast(port_name, "0x%llx\n", 20, unsigned long long);
 fc_private_host_rd_attr(symbolic_name, "%s\n", (FC_SYMBOLIC_NAME_SIZE +1));
 fc_private_host_rd_attr(maxframe_size, "%u bytes\n", 20);
-fc_private_host_rd_attr(hardware_version, "%s\n", (FC_VERSION_STRING_SIZE +1));
-fc_private_host_rd_attr(firmware_version, "%s\n", (FC_VERSION_STRING_SIZE +1));
 fc_private_host_rd_attr(serial_number, "%s\n", (FC_SERIAL_NUMBER_SIZE +1));
-fc_private_host_rd_attr(opt_rom_version, "%s\n", (FC_VERSION_STRING_SIZE +1));
-fc_private_host_rd_attr(driver_version, "%s\n", (FC_VERSION_STRING_SIZE +1));
 
 
 /* Dynamic Host Attributes */
@@ -787,11 +775,7 @@ fc_attach_transport(struct fc_function_t
 	SETUP_HOST_ATTRIBUTE_RD(symbolic_name);
 	SETUP_HOST_ATTRIBUTE_RD(supported_speeds);
 	SETUP_HOST_ATTRIBUTE_RD(maxframe_size);
-	SETUP_HOST_ATTRIBUTE_RD(hardware_version);
-	SETUP_HOST_ATTRIBUTE_RD(firmware_version);
 	SETUP_HOST_ATTRIBUTE_RD(serial_number);
-	SETUP_HOST_ATTRIBUTE_RD(opt_rom_version);
-	SETUP_HOST_ATTRIBUTE_RD(driver_version);
 
 	SETUP_HOST_ATTRIBUTE_RD(port_id);
 	SETUP_HOST_ATTRIBUTE_RD(port_type);
--- 1.9/include/scsi/scsi_transport_fc.h	2005-01-12 17:08:53 +01:00
+++ edited/include/scsi/scsi_transport_fc.h	2005-01-19 18:17:24 +01:00
@@ -197,11 +197,7 @@ struct fc_host_attrs {
 	char symbolic_name[FC_SYMBOLIC_NAME_SIZE];
 	u32 supported_speeds;
 	u32 maxframe_size;
-	char hardware_version[FC_VERSION_STRING_SIZE];
-	char firmware_version[FC_VERSION_STRING_SIZE];
 	char serial_number[FC_SERIAL_NUMBER_SIZE];
-	char opt_rom_version[FC_VERSION_STRING_SIZE];
-	char driver_version[FC_VERSION_STRING_SIZE];
 
 	/* Dynamic Attributes */
 	u32 port_id;

Re: [PATCH] drop some attibutes from the FC transport class

[Greg KH]

On Wed, Jan 19, 2005 at 06:13:57PM +0100, Christoph Hellwig wrote:
> I think the hardware_version, firmware_version, rom_version and
> driver_version don't belong into the FC transport class, there's
> nothign specific to FC or even SCSI specific in them.

Then put them in the individual driver (not the driver_version one
though, that belongs as a MODULE_VERSION() paramater).

> If you want these attributes talk to Greg about fiding a place in
> the common driver model code for them.

No, they don't belong there either, sorry :)

thanks,

greg k-h

Re: [PATCH] drop some attibutes from the FC transport class

[Brian King]

Greg KH wrote:
> On Wed, Jan 19, 2005 at 06:13:57PM +0100, Christoph Hellwig wrote:
> 
>>I think the hardware_version, firmware_version, rom_version and
>>driver_version don't belong into the FC transport class, there's
>>nothign specific to FC or even SCSI specific in them.
> 
> 
> Then put them in the individual driver (not the driver_version one
> though, that belongs as a MODULE_VERSION() paramater).
> 
> 
>>If you want these attributes talk to Greg about fiding a place in
>>the common driver model code for them.
> 
> 
> No, they don't belong there either, sorry :)

How about at an attribute on the pci object? I currently have a driver 
private firmware version in the ipr driver as a scsi_host class 
attribute. Not sure how many other drivers will end up doing similar. 
Might be nice to have a common way to export this information to 
userspace. Or we can always wait for more users before we do something 
like this...

-Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH] drop some attibutes from the FC transport class

[Greg KH]

On Wed, Jan 19, 2005 at 12:38:31PM -0600, Brian King wrote:
> Greg KH wrote:
> >On Wed, Jan 19, 2005 at 06:13:57PM +0100, Christoph Hellwig wrote:
> >
> >>I think the hardware_version, firmware_version, rom_version and
> >>driver_version don't belong into the FC transport class, there's
> >>nothign specific to FC or even SCSI specific in them.
> >
> >
> >Then put them in the individual driver (not the driver_version one
> >though, that belongs as a MODULE_VERSION() paramater).
> >
> >
> >>If you want these attributes talk to Greg about fiding a place in
> >>the common driver model code for them.
> >
> >
> >No, they don't belong there either, sorry :)
> 
> How about at an attribute on the pci object?

It is a pci specific attribute?  If so, I'll add it to the pci core.
Otherwise you are free to add it yourself to the pci device in your pci
driver to that location.

thanks,

greg k-h

Re: [PATCH] drop some attibutes from the FC transport class

[Brian King]

Greg KH wrote:
> On Wed, Jan 19, 2005 at 12:38:31PM -0600, Brian King wrote:
> 
>>Greg KH wrote:
>>
>>>On Wed, Jan 19, 2005 at 06:13:57PM +0100, Christoph Hellwig wrote:
>>>
>>>
>>>>I think the hardware_version, firmware_version, rom_version and
>>>>driver_version don't belong into the FC transport class, there's
>>>>nothign specific to FC or even SCSI specific in them.
>>>
>>>
>>>Then put them in the individual driver (not the driver_version one
>>>though, that belongs as a MODULE_VERSION() paramater).
>>>
>>>
>>>
>>>>If you want these attributes talk to Greg about fiding a place in
>>>>the common driver model code for them.
>>>
>>>
>>>No, they don't belong there either, sorry :)
>>
>>How about at an attribute on the pci object?
> 
> 
> It is a pci specific attribute?  If so, I'll add it to the pci core.
> Otherwise you are free to add it yourself to the pci device in your pci
> driver to that location.

I won't argue that it is a pci specific attribute, since there are 
plenty of devices out there that have firmware that are not pci devices, 
such as scsi disks. This just seems like too common of an attribute to 
have each driver doing their own thing. The options as I see them are:

1. scsi_host class
2. pci core
3. driver unique



-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH] drop some attibutes from the FC transport class

[Mike Anderson]

Greg KH [greg@kroah.com] wrote:
> On Wed, Jan 19, 2005 at 06:13:57PM +0100, Christoph Hellwig wrote:
> > I think the hardware_version, firmware_version, rom_version and
> > driver_version don't belong into the FC transport class, there's
> > nothign specific to FC or even SCSI specific in them.
> 
> Then put them in the individual driver (not the driver_version one
> though, that belongs as a MODULE_VERSION() paramater).

Are there plans to export MODULE_VERSION info in /sys/module/*/ ? I
believe the intent was to try and use libsysfs to access these attributes
/ info.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] drop some attibutes from the FC transport class

[Greg KH]

On Wed, Jan 19, 2005 at 01:39:24PM -0800, Mike Anderson wrote:
> Greg KH [greg@kroah.com] wrote:
> > On Wed, Jan 19, 2005 at 06:13:57PM +0100, Christoph Hellwig wrote:
> > > I think the hardware_version, firmware_version, rom_version and
> > > driver_version don't belong into the FC transport class, there's
> > > nothign specific to FC or even SCSI specific in them.
> > 
> > Then put them in the individual driver (not the driver_version one
> > though, that belongs as a MODULE_VERSION() paramater).
> 
> Are there plans to export MODULE_VERSION info in /sys/module/*/ ? I
> believe the intent was to try and use libsysfs to access these attributes
> / info.

I had a patch to do that around here somewhere, but I think it was
rejected as people can get the same info by using 'modinfo' instead.
You know, the old, "use a userspace tool instead of wasting kernel
memory" argument :)

thanks,

greg k-h

Re: [PATCH] drop some attibutes from the FC transport class

[Brian King]

Greg KH wrote:
> On Wed, Jan 19, 2005 at 01:39:24PM -0800, Mike Anderson wrote:
> 
>>Greg KH [greg@kroah.com] wrote:
>>
>>>On Wed, Jan 19, 2005 at 06:13:57PM +0100, Christoph Hellwig wrote:
>>>
>>>>I think the hardware_version, firmware_version, rom_version and
>>>>driver_version don't belong into the FC transport class, there's
>>>>nothign specific to FC or even SCSI specific in them.
>>>
>>>Then put them in the individual driver (not the driver_version one
>>>though, that belongs as a MODULE_VERSION() paramater).
>>
>>Are there plans to export MODULE_VERSION info in /sys/module/*/ ? I
>>believe the intent was to try and use libsysfs to access these attributes
>>/ info.
> 
> 
> I had a patch to do that around here somewhere, but I think it was
> rejected as people can get the same info by using 'modinfo' instead.
> You know, the old, "use a userspace tool instead of wasting kernel
> memory" argument :)

That's too bad. modinfo will only tell you information about the driver 
that modprobe finds, which is not necessarily what is currently loaded 
in kernel memory.

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH] drop some attibutes from the FC transport class

[Christoph Hellwig]

On Wed, Jan 19, 2005 at 02:40:16PM -0800, Greg KH wrote:
> I had a patch to do that around here somewhere, but I think it was
> rejected as people can get the same info by using 'modinfo' instead.
> You know, the old, "use a userspace tool instead of wasting kernel
> memory" argument :)

How's that supposed to work for builtin drivers?

Re: [PATCH] drop some attibutes from the FC transport class

[Greg KH]

On Wed, Jan 19, 2005 at 11:03:50PM +0000, Christoph Hellwig wrote:
> On Wed, Jan 19, 2005 at 02:40:16PM -0800, Greg KH wrote:
> > I had a patch to do that around here somewhere, but I think it was
> > rejected as people can get the same info by using 'modinfo' instead.
> > You know, the old, "use a userspace tool instead of wasting kernel
> > memory" argument :)
> 
> How's that supposed to work for builtin drivers?

Look at /sys/modules these days.  It shows all builtin modules now :)

Oh wait.  I get your point.  Hm, I don't know.  Want me to dig up the
patch?  It also added such unneeded things as module author and license
just to be "complete".

thanks,

greg k-h

Re: [PATCH] drop some attibutes from the FC transport class

[Matt Domsch]

On Wed, Jan 19, 2005 at 03:08:55PM -0800, Greg KH wrote:
> On Wed, Jan 19, 2005 at 11:03:50PM +0000, Christoph Hellwig wrote:
> > On Wed, Jan 19, 2005 at 02:40:16PM -0800, Greg KH wrote:
> > > I had a patch to do that around here somewhere, but I think it was
> > > rejected as people can get the same info by using 'modinfo' instead.
> > > You know, the old, "use a userspace tool instead of wasting kernel
> > > memory" argument :)
> > 
> > How's that supposed to work for builtin drivers?
> 
> Oh wait.  I get your point.  Hm, I don't know.  Want me to dig up the
> patch?

Yes, DKMS would like to use that too.


-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [PATCH] drop some attibutes from the FC transport class

[Greg KH]

On Wed, Jan 19, 2005 at 05:15:59PM -0600, Matt Domsch wrote:
> On Wed, Jan 19, 2005 at 03:08:55PM -0800, Greg KH wrote:
> > On Wed, Jan 19, 2005 at 11:03:50PM +0000, Christoph Hellwig wrote:
> > > On Wed, Jan 19, 2005 at 02:40:16PM -0800, Greg KH wrote:
> > > > I had a patch to do that around here somewhere, but I think it was
> > > > rejected as people can get the same info by using 'modinfo' instead.
> > > > You know, the old, "use a userspace tool instead of wasting kernel
> > > > memory" argument :)
> > > 
> > > How's that supposed to work for builtin drivers?
> > 
> > Oh wait.  I get your point.  Hm, I don't know.  Want me to dig up the
> > patch?
> 
> Yes, DKMS would like to use that too.

Here it was against 2.6.9.  Odds are it doesn't apply anymore due to
some recent changes in this area.  If anyone wants to take it and fix it
up, please do so.

Note, this patch needs the evil strdup() function to work properly.
That's one of the main reasons I decided to not submit it to the main
kernel tree...

thanks,

greg k-h

------------
Module:  Add module author, version and license to the sysfs tree

Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

diff -Nru a/include/linux/module.h b/include/linux/module.h
--- a/include/linux/module.h	2004-10-20 10:16:28 -07:00
+++ b/include/linux/module.h	2004-10-20 10:16:28 -07:00
@@ -308,6 +308,12 @@
 	/* Fake kernel param for refcnt. */
 	struct kernel_param refcnt_param;
 #endif
+	struct kernel_param author_param;
+	struct kernel_param version_param;
+	struct kernel_param license_param;
+	char *author;
+	char *version;
+	char *license;
 
 #ifdef CONFIG_KALLSYMS
 	/* We keep the symbol and string tables for kallsyms. */
diff -Nru a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c	2004-10-20 10:16:28 -07:00
+++ b/kernel/module.c	2004-10-20 10:16:28 -07:00
@@ -1094,6 +1094,25 @@
 	.store = module_attr_store,
 };
 
+#define MOD_SYSFS(field)	\
+static int field##_get_fn(char *buffer, struct kernel_param *kp)		\
+{										\
+	struct module *mod = container_of(kp, struct module, field##_param);	\
+	return sprintf(buffer, "%s", mod->field);				\
+}										\
+static int sysfs_##field##_setup(struct module *mod)				\
+{										\
+	if (!mod->field)							\
+		return 0;							\
+	mod->field##_param.name = __stringify(field);				\
+	mod->field##_param.perm = 0444;						\
+	mod->field##_param.get = field##_get_fn;				\
+	return add_attribute(mod, &mod->field##_param);				\
+}
+MOD_SYSFS(author);
+MOD_SYSFS(version);
+MOD_SYSFS(license);
+
 static void module_kobj_release(struct kobject *kobj)
 {
 	kfree(container_of(kobj, struct module_kobject, kobj));
@@ -1114,7 +1133,7 @@
 
 	/* We overallocate: not every param is in sysfs, and maybe no refcnt */
 	mod->mkobj = kmalloc(sizeof(*mod->mkobj)
-			     + sizeof(mod->mkobj->attr[0]) * (num_params+1),
+			     + sizeof(mod->mkobj->attr[0]) * (num_params+4),
 			     GFP_KERNEL);
 	if (!mod->mkobj)
 		return -ENOMEM;
@@ -1140,6 +1159,15 @@
 	err = sysfs_unload_setup(mod);
 	if (err)
 		goto out_unreg;
+	err = sysfs_author_setup(mod);
+	if (err)
+		goto out_unreg;
+	err = sysfs_version_setup(mod);
+	if (err)
+		goto out_unreg;
+	err = sysfs_license_setup(mod);
+	if (err)
+		goto out_unreg;
 	return 0;
 
 out_unreg:
@@ -1160,6 +1188,9 @@
 		sysfs_remove_file(&mod->mkobj->kobj,&mod->mkobj->attr[i].attr);
 	/* Calls module_kobj_release */
 	kobject_unregister(&mod->mkobj->kobj);
+	kfree(mod->author);
+	kfree(mod->license);
+	kfree(mod->version);
 }
 
 /* Free a module, remove from lists, etc (must hold module mutex). */
@@ -1346,11 +1377,25 @@
 		|| strcmp(license, "Dual MPL/GPL") == 0);
 }
 
+static char *strdup(const char *str)
+{
+	char *s;
+
+	if (!str)
+		return NULL;
+	s = kmalloc(strlen(str)+1, GFP_KERNEL);
+	if (!s)
+		return NULL;
+	strcpy(s, str);
+	return s;
+}
+
 static void set_license(struct module *mod, const char *license)
 {
 	if (!license)
 		license = "unspecified";
 
+	mod->license = strdup(license);
 	mod->license_gplok = license_is_gpl_compatible(license);
 	if (!mod->license_gplok && !(tainted & TAINT_PROPRIETARY_MODULE)) {
 		printk(KERN_WARNING "%s: module license '%s' taints kernel.\n",
@@ -1696,6 +1741,9 @@
 
 	/* Set up license info based on the info section */
 	set_license(mod, get_modinfo(sechdrs, infoindex, "license"));
+
+	mod->author = strdup(get_modinfo(sechdrs, infoindex, "author"));
+	mod->version = strdup(get_modinfo(sechdrs, infoindex, "version"));
 
 	/* Fix up syms, so that st_value is a pointer to location. */
 	err = simplify_symbols(sechdrs, symindex, strtab, versindex, pcpuindex,

Re: [PATCH] drop some attibutes from the FC transport class

[Matt Domsch]

On Wed, Jan 19, 2005 at 03:42:19PM -0800, Greg KH wrote:
> Here it was against 2.6.9.  Odds are it doesn't apply anymore due to
> some recent changes in this area.  If anyone wants to take it and fix it
> up, please do so.
> 
> Note, this patch needs the evil strdup() function to work properly.
> That's one of the main reasons I decided to not submit it to the main
> kernel tree...

How would you feel if instead, we just didn't drop the .modinfo
section?  That would eliminate the need to copy the strings, and then
the field##_get_function would just use the string from the .modinfo
section directly?

Thanks,
Matt

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [PATCH] drop some attibutes from the FC transport class

[Greg KH]

On Wed, Jan 19, 2005 at 11:12:00PM -0600, Matt Domsch wrote:
> On Wed, Jan 19, 2005 at 03:42:19PM -0800, Greg KH wrote:
> > Here it was against 2.6.9.  Odds are it doesn't apply anymore due to
> > some recent changes in this area.  If anyone wants to take it and fix it
> > up, please do so.
> > 
> > Note, this patch needs the evil strdup() function to work properly.
> > That's one of the main reasons I decided to not submit it to the main
> > kernel tree...
> 
> How would you feel if instead, we just didn't drop the .modinfo
> section?  That would eliminate the need to copy the strings, and then
> the field##_get_function would just use the string from the .modinfo
> section directly?

That makes sense to me, but you might want to ask Rusty about it, as
it's his code.

thanks,

greg k-h

[PATCH 2.6.11-rc2] modules: add version and srcversion to sysfs

[Matt Domsch]

Module:  Add module version and srcversion to the sysfs tree

There are two ways one could go with this:
1) strdup() the interesting fields from modinfo section before it is discarded.
That's what this patch does, and what Greg's original patch did too,
despite his reservations about using strdup().
2) don't drop the modinfo section on load, and use those strings directly.

The problem with 2) is that the modinfo section can be quite large,
compared to the few bytes that the "interesting" fields consume.  And
it would have to be kmalloc'd and copied from the vmalloc area before
that area is dropped.

So, I did #1.

It's trivial to add new fields now.  For my purposes, version and
srcversion are all I need, so that's all I'm exposing via sysfs.
Others may be added later as needed.

As the idea originated from GregKH, I leave his Signed-off-by: intact,
though the implementation is nearly completely new.  Compiled and run
on x86_64.

Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Signed-off-by: Matt Domsch <Matt_Domsch@dell.com>

Rusty, thoughts?

Thanks,
Matt

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

===== include/linux/module.h 1.92 vs edited =====
--- 1.92/include/linux/module.h	2005-01-10 13:28:15 -06:00
+++ edited/include/linux/module.h	2005-01-25 22:25:15 -06:00
@@ -51,6 +51,9 @@ struct module_attribute {
         ssize_t (*show)(struct module_attribute *, struct module *, char *);
         ssize_t (*store)(struct module_attribute *, struct module *,
 			 const char *, size_t count);
+	void (*setup)(struct module *, const char *);
+	int (*test)(struct module *);
+	void (*free)(struct module *);
 };
 
 struct module_kobject
@@ -249,6 +252,8 @@ struct module
 	/* Sysfs stuff. */
 	struct module_kobject mkobj;
 	struct module_param_attrs *param_attrs;
+	const char *version;
+	const char *srcversion;
 
 	/* Exported symbols */
 	const struct kernel_symbol *syms;
===== kernel/module.c 1.132 vs edited =====
--- 1.132/kernel/module.c	2005-01-11 18:42:57 -06:00
+++ edited/kernel/module.c	2005-01-25 23:42:17 -06:00
@@ -663,6 +663,57 @@ static struct module_attribute refcnt = 
 	.show = show_refcnt,
 };
 
+static char *strdup(const char *str)
+{
+	char *s;
+
+	if (!str)
+		return NULL;
+	s = kmalloc(strlen(str)+1, GFP_KERNEL);
+	if (!s)
+		return NULL;
+	strcpy(s, str);
+	return s;
+}
+
+#define MODINFO_ATTR(field)	\
+static void setup_modinfo_##field(struct module *mod, const char *s)  \
+{                                                                     \
+	mod->field = strdup(s);                                       \
+}                                                                     \
+static ssize_t show_modinfo_##field(struct module_attribute *mattr,   \
+	                struct module *mod, char *buffer)             \
+{                                                                     \
+	return sprintf(buffer, "%s\n", mod->field);                   \
+}                                                                     \
+static int modinfo_##field##_exists(struct module *mod)               \
+{                                                                     \
+	return mod->field != NULL;                                    \
+}                                                                     \
+static void free_modinfo_##field(struct module *mod)                  \
+{                                                                     \
+        kfree(mod->field);                                            \
+        mod->field = NULL;                                            \
+}                                                                     \
+static struct module_attribute modinfo_##field = {                    \
+	.attr = { .name = __stringify(field), .mode = 0444,           \
+		  .owner = THIS_MODULE },                             \
+	.show = show_modinfo_##field,                                 \
+	.setup = setup_modinfo_##field,                               \
+	.test = modinfo_##field##_exists,                             \
+	.free = free_modinfo_##field,                                 \
+};
+
+
+MODINFO_ATTR(version);
+MODINFO_ATTR(srcversion);
+
+static struct module_attribute *modinfo_attrs[] = {
+	&modinfo_version,
+	&modinfo_srcversion,
+	NULL,
+};
+
 #else /* !CONFIG_MODULE_UNLOAD */
 static void print_unload_info(struct seq_file *m, struct module *mod)
 {
@@ -1031,6 +1082,29 @@ static void module_remove_refcnt_attr(st
 }
 #endif
 
+static int module_add_modinfo_attrs(struct module *mod)
+{
+	struct module_attribute * attr;
+	int error = 0;
+	int i;
+
+	for (i = 0; (attr = modinfo_attrs[i]) && !error; i++) {
+		if (!attr->test ||
+		    (attr->test && attr->test(mod)))
+			error = sysfs_create_file(&mod->mkobj.kobj,&attr->attr);
+	}
+}
+
+static void module_remove_modinfo_attrs(struct module *mod)
+{
+	struct module_attribute * attr;
+	int i;
+
+	for (i = 0; (attr = modinfo_attrs[i]); i++) {
+		sysfs_remove_file(&mod->mkobj.kobj,&attr->attr);
+		attr->free(mod);
+	}
+}
 
 static int mod_sysfs_setup(struct module *mod,
 			   struct kernel_param *kparam,
@@ -1056,6 +1130,10 @@ static int mod_sysfs_setup(struct module
 	if (err)
 		goto out_unreg;
 
+	err = module_add_modinfo_attrs(mod);
+	if (err)
+		goto out_unreg;
+
 	return 0;
 
 out_unreg:
@@ -1066,6 +1144,7 @@ out:
 
 static void mod_kobject_remove(struct module *mod)
 {
+	module_remove_modinfo_attrs(mod);
 	module_remove_refcnt_attr(mod);
 	module_param_sysfs_remove(mod);
 
@@ -1303,6 +1382,21 @@ static char *get_modinfo(Elf_Shdr *sechd
 	return NULL;
 }
 
+static void setup_modinfo(struct module *mod, Elf_Shdr *sechdrs,
+			  unsigned int infoindex)
+{
+	struct module_attribute * attr;
+	int i;
+
+	for (i = 0; (attr = modinfo_attrs[i]); i++) {
+		if (attr->setup)
+			attr->setup(mod,
+				    get_modinfo(sechdrs,
+						infoindex,
+						attr->attr.name));
+	}
+}
+
 #ifdef CONFIG_KALLSYMS
 int is_exported(const char *name, const struct module *mod)
 {
@@ -1606,6 +1700,9 @@ static struct module *load_module(void _
 
 	/* Set up license info based on the info section */
 	set_license(mod, get_modinfo(sechdrs, infoindex, "license"));
+
+	/* Set up MODINFO_ATTR fields */
+	setup_modinfo(mod, sechdrs, infoindex);
 
 	/* Fix up syms, so that st_value is a pointer to location. */
 	err = simplify_symbols(sechdrs, symindex, strtab, versindex, pcpuindex,

Re: [PATCH 2.6.11-rc2] modules: add version and srcversion to sysfs

[Andreas Gruenbacher]

Hello,

On Wednesday 26 January 2005 07:05, Matt Domsch wrote:
> Module:  Add module version and srcversion to the sysfs tree

why do you need this?

Thanks,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

Re: [PATCH 2.6.11-rc2] modules: add version and srcversion to sysfs

[Matt Domsch]

On Wed, Jan 26, 2005 at 10:22:29AM +0100, Andreas Gruenbacher wrote:
> On Wednesday 26 January 2005 07:05, Matt Domsch wrote:
> > Module:  Add module version and srcversion to the sysfs tree
> 
> why do you need this?

a) Tools like DKMS, which deal with changing out individual kernel
modules without replacing the whole kernel, can behave smarter if they
can tell the version of a given module.  The autoinstaller feature,
for example, which determines if your system has a "good" version of a
driver (i.e. if the one provided by DKMS has a newer verson than that
provided by the kernel package installed), and to automatically
compile and install a newer version if DKMS has it but your kernel
doesn't yet have that version.

b) Because tools like DKMS can switch out modules, you can't count on
'modinfo foo.ko', which looks at
/lib/modules/${kernelver}/... actually matching what is loaded into
the kernel already.  Hence asking sysfs for this.

c) as the unbind-driver-from-device work takes shape, it will be
possible to rebind a driver that's built-in (no .ko to modinfo for the
version) to a newly loaded module.  sysfs will have the
currently-built-in version info, for comparison.

d) tech support scripts can then easily grab the version info for
what's running presently - a question I get often.

Thanks,
Matt


-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [PATCH 2.6.11-rc2] modules: add version and srcversion to sysfs

[Andreas Gruenbacher]

On Wed, 2005-01-26 at 15:09, Matt Domsch wrote:
> On Wed, Jan 26, 2005 at 10:22:29AM +0100, Andreas Gruenbacher wrote:
> > On Wednesday 26 January 2005 07:05, Matt Domsch wrote:
> > > Module:  Add module version and srcversion to the sysfs tree
> > 
> > why do you need this?
> 
> a) Tools like DKMS, which deal with changing out individual kernel
> modules without replacing the whole kernel, can behave smarter if they
> can tell the version of a given module.

They can look at the modules in /lib/modules/$(uname -r).

> The autoinstaller feature,
> for example, which determines if your system has a "good" version of a
> driver (i.e. if the one provided by DKMS has a newer verson than that
> provided by the kernel package installed), and to automatically
> compile and install a newer version if DKMS has it but your kernel
> doesn't yet have that version.

I find the autoinstaller feature quite scary.

> b) Because tools like DKMS can switch out modules, you can't count on
> 'modinfo foo.ko', which looks at
> /lib/modules/${kernelver}/... actually matching what is loaded into
> the kernel already.  Hence asking sysfs for this.

DKMS doesn't manage loading modules, does it? If it does, then at least
it shouldn't; that's even more scary than the autoinstaller. From the
point of view of the kernel, the modules relevant for the running kernel
are those below /lib/modules/$(uname -r)/. If DKMS replaces things
there, it'd better keep proper track of what it did.

I never want to see DKMS try to remove a module from the running kernel
or insmod a new one.

> c) as the unbind-driver-from-device work takes shape, it will be
> possible to rebind a driver that's built-in (no .ko to modinfo for the
> version) to a newly loaded module.  sysfs will have the
> currently-built-in version info, for comparison.
>
> d) tech support scripts can then easily grab the version info for
> what's running presently - a question I get often.

That's something you can do entirely in userspace by looking at the *.ko
files.

Regards,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX GMBH

Re: [PATCH 2.6.11-rc2] modules: add version and srcversion to sysfs

[Matt Domsch]

On Wed, Jan 26, 2005 at 05:38:51PM +0100, Andreas Gruenbacher wrote:
> > The autoinstaller feature,
> > for example, which determines if your system has a "good" version of a
> > driver (i.e. if the one provided by DKMS has a newer verson than that
> > provided by the kernel package installed), and to automatically
> > compile and install a newer version if DKMS has it but your kernel
> > doesn't yet have that version.
> 
> I find the autoinstaller feature quite scary.

The autoinstaller itself is mechanism.  When it's invoked is a policy
decision.  I described the policies that Dell employs with it in my
OLS paper last summer.  For non-distribution-provided drivers
(ppp_mppe, ALSA in some cases, 3rd party video), the autoinstaller is
enabled, and it rebuilds and installs those drivers at new kernel boot
time.

For distribution-provided drivers, we default the autoinstaller
off.  But, this has bitten us.  RHEL3 Update 2 and Update 3 both
shipped with an aacraid (boot storage) driver that crashed the kernel
on insmod.  As fate would have it, Dell provided an updated driver
source package in DKMS format to solve this.  An intelligent
autoinstaller could have looked at the version fields and determined
that what was in the "newest" Update kernel was in fact older than
what DKMS had available to it, and used that instead.  Which is why
we've been pushing for MODULE_VERSION() fields in all the drivers, and
why the srcversion field can exist for all drivers now.

 
> > b) Because tools like DKMS can switch out modules, you can't count on
> > 'modinfo foo.ko', which looks at
> > /lib/modules/${kernelver}/... actually matching what is loaded into
> > the kernel already.  Hence asking sysfs for this.
> 
> DKMS doesn't manage loading modules, does it? If it does, then at least
> it shouldn't; that's even more scary than the autoinstaller. From the
> point of view of the kernel, the modules relevant for the running kernel
> are those below /lib/modules/$(uname -r)/. If DKMS replaces things
> there, it'd better keep proper track of what it did.

It does keep track, quite well.
 
> I never want to see DKMS try to remove a module from the running kernel
> or insmod a new one.

Ahh, but that's a policy decision to be made.  I don't have a
production example of needing to do this yet, so it doesn't.  But I
wouldn't rule out the future need for such.  (i.e. wanting to
automate upgrading a NIC driver without rebooting the server).


> > c) as the unbind-driver-from-device work takes shape, it will be
> > possible to rebind a driver that's built-in (no .ko to modinfo for the
> > version) to a newly loaded module.  sysfs will have the
> > currently-built-in version info, for comparison.

As it happens, right now built-in modules don't have modinfo sections,
so this piece doesn't work.  There's no way to ask the kernel for the
version of built-in modules then, it doesn't know.  But with unbind
happening, that will be useful information to have, I'll have to look
into building parts of that section in.  Thanks for pointing this
deficiency out.

Thanks,
Matt

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [PATCH 2.6.11-rc2] modules: add version and srcversion to sysfs

[Bill Davidsen]

Andreas Gruenbacher wrote:
> On Wed, 2005-01-26 at 15:09, Matt Domsch wrote:
> 
>>On Wed, Jan 26, 2005 at 10:22:29AM +0100, Andreas Gruenbacher wrote:
>>
>>>On Wednesday 26 January 2005 07:05, Matt Domsch wrote:
>>>
>>>>Module:  Add module version and srcversion to the sysfs tree
>>>
>>>why do you need this?
>>
>>a) Tools like DKMS, which deal with changing out individual kernel
>>modules without replacing the whole kernel, can behave smarter if they
>>can tell the version of a given module.
> 
> 
> They can look at the modules in /lib/modules/$(uname -r).

I think he means he wants to know what's in memory, not on the disk.

[ snip ]
> 
>>c) as the unbind-driver-from-device work takes shape, it will be
>>possible to rebind a driver that's built-in (no .ko to modinfo for the
>>version) to a newly loaded module.  sysfs will have the
>>currently-built-in version info, for comparison.
>>
>>d) tech support scripts can then easily grab the version info for
>>what's running presently - a question I get often.
> 
> 
> That's something you can do entirely in userspace by looking at the *.ko
> files.

How do you find which *.ko file was used to load the module in memory? I 
think you are talking about two things here.

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: [PATCH 2.6.11-rc2] modules: add version and srcversion to sysfs

[Paulo Marques]

Matt Domsch wrote:
> [...]
>  
> +static char *strdup(const char *str)
> +{
> +	char *s;
> +
> +	if (!str)
> +		return NULL;
> +	s = kmalloc(strlen(str)+1, GFP_KERNEL);
> +	if (!s)
> +		return NULL;
> +	strcpy(s, str);
> +	return s;
> +}
> +

There is already this code in sound/core/memory.c:

char *snd_kmalloc_strdup(const char *string, int flags)
{
	size_t len;
	char *ptr;

	if (!string)
		return NULL;
	len = strlen(string) + 1;
	ptr = _snd_kmalloc(len, flags);
	if (ptr)
		memcpy(ptr, string, len);
	return ptr;
}

and grep'ing the includes for "strdup" gives this:

./linux/netdevice.h:extern char *net_sysctl_strdup(const char *s);
./linux/parser.h:char *match_strdup(substring_t *);
./sound/core.h:char *snd_kmalloc_strdup(const char *string, int flags);

Actually, I've just grep'ed the entire tree and there are about 7 
similar implementations all over the place:

./arch/um/kernel/process_kern.c:char *uml_strdup(char *string)
./drivers/parport/probe.c:static char *strdup(char *str)
./drivers/md/dm-ioctl.c:static inline char *kstrdup(const char *str)
./net/core/sysctl_net_core.c:char *net_sysctl_strdup(const char *s)
./net/sunrpc/svcauth_unix.c:static char *strdup(char *s)
./sound/core/memory.c:char *snd_kmalloc_strdup(const char *string, int 
flags)
./lib/parser.c:char *match_strdup(substring_t *s)

So maybe we should turn this into a library function and modify the 
callers, so that we have only one implementation. The implementation 
from sound/core seems better for a library function, because of the 
flags argument (and it seems a little more eficient too).

-- 
Paulo Marques - www.grupopie.com

"A journey of a thousand miles begins with a single step."
Lao-tzu, The Way of Lao-tzu

Re: [PATCH 2.6.11-rc2] modules: add version and srcversion to sysfs

[Rusty Russell]

On Wed, 2005-01-26 at 14:32 +0000, Paulo Marques wrote:
> Matt Domsch wrote:
> > [...]
> >  
> > +static char *strdup(const char *str)
...
> Actually, I've just grep'ed the entire tree and there are about 7 
> similar implementations all over the place:

Wow, I'd never noticed.  Linus, please apply 8)

Rusty.
Name: kstrdup
Author: Neil Brown, Rusty Russell and Robert Love
Status: Trivial

Everyone loves reimplementing strdup.  Give them a kstrdup.

Index: linux-2.6.11-rc2-bk4-Misc/include/linux/string.h
===================================================================
--- linux-2.6.11-rc2-bk4-Misc.orig/include/linux/string.h	2004-05-10 15:13:54.000000000 +1000
+++ linux-2.6.11-rc2-bk4-Misc/include/linux/string.h	2005-01-27 13:08:30.042035568 +1100
@@ -88,6 +88,8 @@
 extern void * memchr(const void *,int,__kernel_size_t);
 #endif
 
+extern char *kstrdup(const char *s, int gfp);
+
 #ifdef __cplusplus
 }
 #endif
Index: linux-2.6.11-rc2-bk4-Misc/lib/string.c
===================================================================
--- linux-2.6.11-rc2-bk4-Misc.orig/lib/string.c	2005-01-27 11:26:15.000000000 +1100
+++ linux-2.6.11-rc2-bk4-Misc/lib/string.c	2005-01-27 13:08:30.080029792 +1100
@@ -23,6 +23,7 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 
 #ifndef __HAVE_ARCH_STRNICMP
 /**
@@ -599,3 +600,19 @@
 }
 EXPORT_SYMBOL(memchr);
 #endif
+
+/*
+ * kstrdup - allocate space for and copy an existing string
+ *
+ * @s: the string to duplicate
+ * @gfp: the GFP mask used in the kmalloc() call when allocating memory
+ */
+char *kstrdup(const char *s, int gfp)
+{
+	char *buf = kmalloc(strlen(s)+1, gfp);
+	if (buf)
+		strcpy(buf, s);
+	return buf;
+}
+
+EXPORT_SYMBOL(kstrdup);

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman