[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

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

[James.Smart]

True. The only thing that made them FC-ish is that they were
mandated by HBAAPI.  The only other desire was to place all
hbaapi-related data in a single place so users/applications don't
have to trapse through the multiple directories in the class and
device trees.

I'll ping Greg on adding them to the common driver model code.

As for the copyright notices - I'll add them. Sorry, my lack of
procedure on kernel additions.  Just trying to be a good citizen.

-- James


> 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.