[PATCH 6/8] Fix spi initialisation failure

[PATCH 6/8] Fix spi initialisation failure

[Hannes Reinecke]

With the target rework we cannot set the attribute of the
sysfs files as the sysfs object is not registered yet. So
just modify the attribute itself.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_transport_spi.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
index bc12b5d..4f9c98c 100644
--- a/drivers/scsi/scsi_transport_spi.c
+++ b/drivers/scsi/scsi_transport_spi.c
@@ -1473,13 +1473,13 @@ static int spi_target_configure(struct transport_container *tc,
 		 * to ignore, sysfs also does a WARN_ON and dumps a trace,
 		 * which is bad, so temporarily, skip attributes that are
 		 * already visible (the revalidate one) */
-		if (j && attr != &dev_attr_revalidate.attr)
+		if (j && attr != &dev_attr_revalidate.attr) {
+			if ((j & 1))
+				attr->mode |= S_IWUSR;
+
 			rc = sysfs_add_file_to_group(kobj, attr,
 						target_attribute_group.name);
-		/* and make the attribute writeable if we have a set
-		 * function */
-		if ((j & 1))
-			rc = sysfs_chmod_file(kobj, attr, attr->mode | S_IWUSR);
+		}
 	}
 
 	return 0;
-- 
1.5.2.4

Re: [PATCH 6/8] Fix spi initialisation failure

[James Bottomley]

On Tue, 2008-03-18 at 14:32 +0100, Hannes Reinecke wrote:
> With the target rework we cannot set the attribute of the
> sysfs files as the sysfs object is not registered yet. So
> just modify the attribute itself.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_transport_spi.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
> index bc12b5d..4f9c98c 100644
> --- a/drivers/scsi/scsi_transport_spi.c
> +++ b/drivers/scsi/scsi_transport_spi.c
> @@ -1473,13 +1473,13 @@ static int spi_target_configure(struct transport_container *tc,
>  		 * to ignore, sysfs also does a WARN_ON and dumps a trace,
>  		 * which is bad, so temporarily, skip attributes that are
>  		 * already visible (the revalidate one) */
> -		if (j && attr != &dev_attr_revalidate.attr)
> +		if (j && attr != &dev_attr_revalidate.attr) {
> +			if ((j & 1))
> +				attr->mode |= S_IWUSR;
> +
>  			rc = sysfs_add_file_to_group(kobj, attr,
>  						target_attribute_group.name);
> -		/* and make the attribute writeable if we have a set
> -		 * function */
> -		if ((j & 1))
> -			rc = sysfs_chmod_file(kobj, attr, attr->mode | S_IWUSR);
> +		}
>  	}

We can't do it like this, I'm afraid.  The attribute passed in is global
to the entire transport class.  You'd basically make every following
revalidate attribute writeable.

My initial reaction is that the target should obviously be visible when
we call configure, the same way the device is.  Could this be fixed just
by moving the configure event?

James

Re: [PATCH 6/8] Fix spi initialisation failure

[James Bottomley]

On Tue, 2008-03-18 at 16:38 +0100, Hannes Reinecke wrote:
> James Bottomley wrote:
> > On Tue, 2008-03-18 at 14:32 +0100, Hannes Reinecke wrote:
> >> With the target rework we cannot set the attribute of the
> >> sysfs files as the sysfs object is not registered yet. So
> >> just modify the attribute itself.
> >>
> >> Signed-off-by: Hannes Reinecke <hare@suse.de>
> >> ---
> >>  drivers/scsi/scsi_transport_spi.c |   10 +++++-----
> >>  1 files changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
> >> index bc12b5d..4f9c98c 100644
> >> --- a/drivers/scsi/scsi_transport_spi.c
> >> +++ b/drivers/scsi/scsi_transport_spi.c
> >> @@ -1473,13 +1473,13 @@ static int spi_target_configure(struct transport_container *tc,
> >>  		 * to ignore, sysfs also does a WARN_ON and dumps a trace,
> >>  		 * which is bad, so temporarily, skip attributes that are
> >>  		 * already visible (the revalidate one) */
> >> -		if (j && attr != &dev_attr_revalidate.attr)
> >> +		if (j && attr != &dev_attr_revalidate.attr) {
> >> +			if ((j & 1))
> >> +				attr->mode |= S_IWUSR;
> >> +
> >>  			rc = sysfs_add_file_to_group(kobj, attr,
> >>  						target_attribute_group.name);
> >> -		/* and make the attribute writeable if we have a set
> >> -		 * function */
> >> -		if ((j & 1))
> >> -			rc = sysfs_chmod_file(kobj, attr, attr->mode | S_IWUSR);
> >> +		}
> >>  	}
> > 
> > We can't do it like this, I'm afraid.  The attribute passed in is global
> > to the entire transport class.  You'd basically make every following
> > revalidate attribute writeable.
> > 
> > My initial reaction is that the target should obviously be visible when
> > we call configure, the same way the device is.  Could this be fixed just
> > by moving the configure event?
> > 
> Well, sort of.
> Frankly I didn't quite get what you're trying to achieve with
> transport_configure().

It's adjusting the writeability of the attributes.  If the device driver
can't support setting them, then they remain read only ... if it can,
then we make them writeable.

I suppose really this should be a function of is_visible() ... I can
look into patching that up.

> Documentation states that 
>  * The idea of configure is simply to provide a point within the setup
>  * process to allow the transport class to extract information from a
>  * device after it has been setup.  This is used in SCSI because we
>  * have to have a setup device to begin using the HBA, but after we
>  * send the initial inquiry, we use configure to extract the device
>  * parameters.  The device need not have been added to be configured.
> 
> So inferring from that I'd assumed I'd have to call configure before
> ->slave_configure(). If you tell me it's okay we can easily move it
> into scsi_sysfs_add_sdev() and we wouldn't have to worry about this.

James

Re: [PATCH 6/8] Fix spi initialisation failure

[James Bottomley]

On Tue, 2008-03-18 at 14:32 +0100, Hannes Reinecke wrote:
> With the target rework we cannot set the attribute of the
> sysfs files as the sysfs object is not registered yet. So
> just modify the attribute itself.

Actually, this isn't the problem.  The problem is that the target is
added too early ... before we've had time to gather the necessary data
from the devices, so is_visible() is returning 0 (i.e. invisible) for a
lot of the files.  Then when we try to chmod a nonexistent file, sysfs
goes boom.

I think this can be fixed by making the sysfs API more elastic ... I'll
send patches shortly.

James