[PATCH] scsi_transport_fc: Initialize sysfs attributes with sysfs_attr_init

[PATCH] scsi_transport_fc: Initialize sysfs attributes with sysfs_attr_init

[Christof Schmitt]

With the new lockdep tracking in sysfs, sysfs_attr_init has to be used
for initializing all non-static sysfs attributes. Otherwise, lockdep
will warn about the missing initialization with:

Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae350 not in .data!
Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae388 not in .data!
Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3c0 not in .data!
Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3f8 not in .data!

Add the calls to sysfs_attr_init for the attributes in struct
fc_internal.

Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
---
 drivers/scsi/scsi_transport_fc.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

--- a/drivers/scsi/scsi_transport_fc.c	2010-03-15 16:39:54.000000000 +0100
+++ b/drivers/scsi/scsi_transport_fc.c	2010-03-15 16:59:08.000000000 +0100
@@ -777,6 +777,7 @@ static FC_DEVICE_ATTR(rport, title, S_IR
 	i->private_rport_attrs[count].attr.mode = S_IRUGO;		\
 	i->private_rport_attrs[count].store = NULL;			\
 	i->rport_attrs[count] = &i->private_rport_attrs[count];		\
+	sysfs_attr_init(&i->rport_attrs[count]->attr);			\
 	if (i->f->show_rport_##field)					\
 		count++
 
@@ -785,6 +786,7 @@ static FC_DEVICE_ATTR(rport, title, S_IR
 	i->private_rport_attrs[count].attr.mode = S_IRUGO;		\
 	i->private_rport_attrs[count].store = NULL;			\
 	i->rport_attrs[count] = &i->private_rport_attrs[count];		\
+	sysfs_attr_init(&i->rport_attrs[count]->attr);			\
 	count++
 
 #define SETUP_RPORT_ATTRIBUTE_RW(field)					\
@@ -794,6 +796,7 @@ static FC_DEVICE_ATTR(rport, title, S_IR
 		i->private_rport_attrs[count].store = NULL;		\
 	}								\
 	i->rport_attrs[count] = &i->private_rport_attrs[count];		\
+	sysfs_attr_init(&i->rport_attrs[count]->attr);			\
 	if (i->f->show_rport_##field)					\
 		count++
 
@@ -801,6 +804,7 @@ static FC_DEVICE_ATTR(rport, title, S_IR
 {									\
 	i->private_rport_attrs[count] = device_attr_rport_##field; \
 	i->rport_attrs[count] = &i->private_rport_attrs[count];		\
+	sysfs_attr_init(&i->rport_attrs[count]->attr);			\
 	count++;							\
 }
 
@@ -994,6 +998,7 @@ static FC_DEVICE_ATTR(starget, field, S_
 	i->private_starget_attrs[count].attr.mode = S_IRUGO;		\
 	i->private_starget_attrs[count].store = NULL;			\
 	i->starget_attrs[count] = &i->private_starget_attrs[count];	\
+	sysfs_attr_init(&i->starget_attrs[count]->attr);		\
 	if (i->f->show_starget_##field)					\
 		count++
 
@@ -1004,6 +1009,7 @@ static FC_DEVICE_ATTR(starget, field, S_
 		i->private_starget_attrs[count].store = NULL;		\
 	}								\
 	i->starget_attrs[count] = &i->private_starget_attrs[count];	\
+	sysfs_attr_init(&i->starget_attrs[count]->attr);		\
 	if (i->f->show_starget_##field)					\
 		count++
 
@@ -1157,6 +1163,7 @@ static FC_DEVICE_ATTR(vport, title, S_IR
 	i->private_vport_attrs[count].attr.mode = S_IRUGO;		\
 	i->private_vport_attrs[count].store = NULL;			\
 	i->vport_attrs[count] = &i->private_vport_attrs[count];		\
+	sysfs_attr_init(i->vport_attrs[count].attr);			\
 	if (i->f->get_##field)						\
 		count++
 	/* NOTE: Above MACRO differs: checks function not show bit */
@@ -1166,11 +1173,13 @@ static FC_DEVICE_ATTR(vport, title, S_IR
 	i->private_vport_attrs[count].attr.mode = S_IRUGO;		\
 	i->private_vport_attrs[count].store = NULL;			\
 	i->vport_attrs[count] = &i->private_vport_attrs[count];		\
+	sysfs_attr_init(&i->vport_attrs[count]->attr);			\
 	count++
 
 #define SETUP_VPORT_ATTRIBUTE_WR(field)					\
 	i->private_vport_attrs[count] = device_attr_vport_##field; \
 	i->vport_attrs[count] = &i->private_vport_attrs[count];		\
+	sysfs_attr_init(&i->vport_attrs[count]->attr);			\
 	if (i->f->field)						\
 		count++
 	/* NOTE: Above MACRO differs: checks function */
@@ -1182,6 +1191,7 @@ static FC_DEVICE_ATTR(vport, title, S_IR
 		i->private_vport_attrs[count].store = NULL;		\
 	}								\
 	i->vport_attrs[count] = &i->private_vport_attrs[count];		\
+	sysfs_attr_init(&i->vport_attrs[count]->attr);			\
 	count++
 	/* NOTE: Above MACRO differs: does not check show bit */
 
@@ -1189,6 +1199,7 @@ static FC_DEVICE_ATTR(vport, title, S_IR
 {									\
 	i->private_vport_attrs[count] = device_attr_vport_##field; \
 	i->vport_attrs[count] = &i->private_vport_attrs[count];		\
+	sysfs_attr_init(&i->vport_attrs[count]->attr);			\
 	count++;							\
 }
 
@@ -1366,6 +1377,7 @@ static FC_DEVICE_ATTR(host, title, S_IRU
 	i->private_host_attrs[count].attr.mode = S_IRUGO;		\
 	i->private_host_attrs[count].store = NULL;			\
 	i->host_attrs[count] = &i->private_host_attrs[count];		\
+	sysfs_attr_init(&i->host_attrs[count]->attr);			\
 	if (i->f->show_host_##field)					\
 		count++
 
@@ -1374,6 +1386,7 @@ static FC_DEVICE_ATTR(host, title, S_IRU
 	i->private_host_attrs[count].attr.mode = S_IRUGO;		\
 	i->private_host_attrs[count].store = NULL;			\
 	i->host_attrs[count] = &i->private_host_attrs[count];		\
+	sysfs_attr_init(&i->host_attrs[count]->attr);			\
 	count++
 
 #define SETUP_HOST_ATTRIBUTE_RW(field)					\
@@ -1383,6 +1396,7 @@ static FC_DEVICE_ATTR(host, title, S_IRU
 		i->private_host_attrs[count].store = NULL;		\
 	}								\
 	i->host_attrs[count] = &i->private_host_attrs[count];		\
+	sysfs_attr_init(&i->host_attrs[count]->attr);			\
 	if (i->f->show_host_##field)					\
 		count++
 
@@ -1411,12 +1425,14 @@ static FC_DEVICE_ATTR(host, field, S_IRU
 	i->private_host_attrs[count].attr.mode = S_IRUGO;		\
 	i->private_host_attrs[count].store = NULL;			\
 	i->host_attrs[count] = &i->private_host_attrs[count];		\
+	sysfs_attr_init(&i->host_attrs[count]->attr);			\
 	count++
 
 #define SETUP_PRIVATE_HOST_ATTRIBUTE_RW(field)			\
 {									\
 	i->private_host_attrs[count] = device_attr_host_##field;	\
 	i->host_attrs[count] = &i->private_host_attrs[count];		\
+	sysfs_attr_init(&i->host_attrs[count]->attr);			\
 	count++;							\
 }
 

Re: [PATCH] scsi_transport_fc: Initialize sysfs attributes with sysfs_attr_init

[Mike Christie]

On 03/16/2010 05:14 AM, Christof Schmitt wrote:
> With the new lockdep tracking in sysfs, sysfs_attr_init has to be used
> for initializing all non-static sysfs attributes. Otherwise, lockdep
> will warn about the missing initialization with:
>
> Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae350 not in .data!
> Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae388 not in .data!
> Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3c0 not in .data!
> Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3f8 not in .data!
>

I think iscsi needs this too, but I am not see this error message. I 
just tried iscsi and fc/lpfc with linus's tree and did not see those 
errors for either. What .config settings do I need for this? I have:

CONFIG_LOCKDEP=y
CONFIG_DEBUG_LOCKDEP=y

are there other lockdep settings?

Re: [PATCH] scsi_transport_fc: Initialize sysfs attributes with sysfs_attr_init

[James Bottomley]

On Tue, 2010-03-16 at 15:48 -0500, Mike Christie wrote:
> On 03/16/2010 05:14 AM, Christof Schmitt wrote:
> > With the new lockdep tracking in sysfs, sysfs_attr_init has to be used
> > for initializing all non-static sysfs attributes. Otherwise, lockdep
> > will warn about the missing initialization with:
> >
> > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae350 not in .data!
> > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae388 not in .data!
> > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3c0 not in .data!
> > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3f8 not in .data!
> >
> 
> I think iscsi needs this too, but I am not see this error message. I 
> just tried iscsi and fc/lpfc with linus's tree and did not see those 
> errors for either. What .config settings do I need for this? I have:

It's a lot worse than that ... every transport class plus some of the
core attributes will need this.

> CONFIG_LOCKDEP=y
> CONFIG_DEBUG_LOCKDEP=y
> 
> are there other lockdep settings?

I also don't see a problem in my systems, so I'd appreciate an answer to
this too.

James

Re: [PATCH] scsi_transport_fc: Initialize sysfs attributes with sysfs_attr_init

[Christof Schmitt]

On Tue, Mar 16, 2010 at 01:49:04PM -0700, James Bottomley wrote:
> On Tue, 2010-03-16 at 15:48 -0500, Mike Christie wrote:
> > On 03/16/2010 05:14 AM, Christof Schmitt wrote:
> > > With the new lockdep tracking in sysfs, sysfs_attr_init has to be used
> > > for initializing all non-static sysfs attributes. Otherwise, lockdep
> > > will warn about the missing initialization with:
> > >
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae350 not in .data!
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae388 not in .data!
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3c0 not in .data!
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3f8 not in .data!
> > >
> > 
> > I think iscsi needs this too, but I am not see this error message. I 
> > just tried iscsi and fc/lpfc with linus's tree and did not see those 
> > errors for either. What .config settings do I need for this? I have:
> 
> It's a lot worse than that ... every transport class plus some of the
> core attributes will need this.
> 
> > CONFIG_LOCKDEP=y
> > CONFIG_DEBUG_LOCKDEP=y
> > 
> > are there other lockdep settings?
> 
> I also don't see a problem in my systems, so I'd appreciate an answer to
> this too.

include/linux/sysfs.h uses CONFIG_DEBUG_LOCK_ALLOC in some places,
e.g.
#ifdef CONFIG_DEBUG_LOCK_ALLOC
#define sysfs_attr_init(attr)				\
do {							\
	static struct lock_class_key __key;		\
							\
	(attr)->key = &__key;				\
} while(0)
#else
#define sysfs_attr_init(attr) do {} while(0)
#endif

So, enabling CONFIG_DEBUG_LOCK_ALLOC should show this problem. I
always use CONFIG_PROVE_LOCKING to enable the lock dependency
checker.

Christof

Re: [PATCH] scsi_transport_fc: Initialize sysfs attributes with sysfs_attr_init

[James Bottomley]

On Tue, 2010-03-16 at 13:49 -0700, James Bottomley wrote:
> On Tue, 2010-03-16 at 15:48 -0500, Mike Christie wrote:
> > On 03/16/2010 05:14 AM, Christof Schmitt wrote:
> > > With the new lockdep tracking in sysfs, sysfs_attr_init has to be used
> > > for initializing all non-static sysfs attributes. Otherwise, lockdep
> > > will warn about the missing initialization with:
> > >
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae350 not in .data!
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae388 not in .data!
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3c0 not in .data!
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3f8 not in .data!
> > >
> > 
> > I think iscsi needs this too, but I am not see this error message. I 
> > just tried iscsi and fc/lpfc with linus's tree and did not see those 
> > errors for either. What .config settings do I need for this? I have:
> 
> It's a lot worse than that ... every transport class plus some of the
> core attributes will need this.

Actually, it looks like we can fix all the transport classes at one go
in the attribute container code rather than doing this per-attribute.

Can someone who sees the problem check this out?

Thanks,

James

---

diff --git a/drivers/base/attribute_container.c b/drivers/base/attribute_container.c
index b9cda05..8fc200b 100644
--- a/drivers/base/attribute_container.c
+++ b/drivers/base/attribute_container.c
@@ -328,6 +328,7 @@ attribute_container_add_attrs(struct device *classdev)
 		return sysfs_create_group(&classdev->kobj, cont->grp);
 
 	for (i = 0; attrs[i]; i++) {
+		sysfs_attr_init(&attrs[i]->attr);
 		error = device_create_file(classdev, attrs[i]);
 		if (error)
 			return error;

Re: [PATCH] scsi_transport_fc: Initialize sysfs attributes with sysfs_attr_init

[Christof Schmitt]

On Sat, Mar 20, 2010 at 12:44:12PM -0500, James Bottomley wrote:
> On Tue, 2010-03-16 at 13:49 -0700, James Bottomley wrote:
> > On Tue, 2010-03-16 at 15:48 -0500, Mike Christie wrote:
> > > On 03/16/2010 05:14 AM, Christof Schmitt wrote:
> > > > With the new lockdep tracking in sysfs, sysfs_attr_init has to be used
> > > > for initializing all non-static sysfs attributes. Otherwise, lockdep
> > > > will warn about the missing initialization with:
> > > >
> > > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae350 not in .data!
> > > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae388 not in .data!
> > > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3c0 not in .data!
> > > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3f8 not in .data!
> > > >
> > > 
> > > I think iscsi needs this too, but I am not see this error message. I 
> > > just tried iscsi and fc/lpfc with linus's tree and did not see those 
> > > errors for either. What .config settings do I need for this? I have:
> > 
> > It's a lot worse than that ... every transport class plus some of the
> > core attributes will need this.
> 
> Actually, it looks like we can fix all the transport classes at one go
> in the attribute container code rather than doing this per-attribute.
> 
> Can someone who sees the problem check this out?
> 
> Thanks,
> 
> James
> 
> ---
> 
> diff --git a/drivers/base/attribute_container.c b/drivers/base/attribute_container.c
> index b9cda05..8fc200b 100644
> --- a/drivers/base/attribute_container.c
> +++ b/drivers/base/attribute_container.c
> @@ -328,6 +328,7 @@ attribute_container_add_attrs(struct device *classdev)
>  		return sysfs_create_group(&classdev->kobj, cont->grp);
> 
>  	for (i = 0; attrs[i]; i++) {
> +		sysfs_attr_init(&attrs[i]->attr);
>  		error = device_create_file(classdev, attrs[i]);
>  		if (error)
>  			return error;
> 

I just tested it with the zfcp driver and this approach works for me.
However, this would put all transport attributes in the same class for
the lock dependency tracker. For the fc_host and fc_remote_port
attributes exported for zfcp, this is no problem. I don't know if this
would be a problem for drivers using the vport_create and vport_delete
attributes.

Christof

RE: [PATCH] scsi_transport_fc: Initialize sysfs attributes with sysfs_attr_init

[Alex.Iannicelli]

This fix seems to do the trick. I tested it with the lpfc driver and I didn't see any more warnings.

Alex

-----Original Message-----
From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of James Bottomley
Sent: Saturday, March 20, 2010 1:44 PM
To: Mike Christie
Cc: Christof Schmitt; linux-scsi@vger.kernel.org
Subject: Re: [PATCH] scsi_transport_fc: Initialize sysfs attributes with sysfs_attr_init

On Tue, 2010-03-16 at 13:49 -0700, James Bottomley wrote:
> On Tue, 2010-03-16 at 15:48 -0500, Mike Christie wrote:
> > On 03/16/2010 05:14 AM, Christof Schmitt wrote:
> > > With the new lockdep tracking in sysfs, sysfs_attr_init has to be used
> > > for initializing all non-static sysfs attributes. Otherwise, lockdep
> > > will warn about the missing initialization with:
> > >
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae350 not in .data!
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae388 not in .data!
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3c0 not in .data!
> > > Mar 15 09:19:49 t6345029 kernel: BUG: key 000000002f5ae3f8 not in .data!
> > >
> > 
> > I think iscsi needs this too, but I am not see this error message. I 
> > just tried iscsi and fc/lpfc with linus's tree and did not see those 
> > errors for either. What .config settings do I need for this? I have:
> 
> It's a lot worse than that ... every transport class plus some of the
> core attributes will need this.

Actually, it looks like we can fix all the transport classes at one go
in the attribute container code rather than doing this per-attribute.

Can someone who sees the problem check this out?

Thanks,

James

---

diff --git a/drivers/base/attribute_container.c b/drivers/base/attribute_container.c
index b9cda05..8fc200b 100644
--- a/drivers/base/attribute_container.c
+++ b/drivers/base/attribute_container.c
@@ -328,6 +328,7 @@ attribute_container_add_attrs(struct device *classdev)
 		return sysfs_create_group(&classdev->kobj, cont->grp);
 
 	for (i = 0; attrs[i]; i++) {
+		sysfs_attr_init(&attrs[i]->attr);
 		error = device_create_file(classdev, attrs[i]);
 		if (error)
 			return error;


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html