[PATCH] scsi_transport_sas: remove local_attached flag

[PATCH] scsi_transport_sas: remove local_attached flag

[James Bottomley]

This flag denotes local attachment of the phy.  There are two problems
with it:

1) It's actually redundant ... you can get the same information simply
by seeing whether a host is the phys parent
2) we condition a lot of phy parameters on it on the false assumption
that we can only control local phys.  I'm wiring up phy resets in the
aic94xx now, and it will be able to reset non-local phys as well.

I fixed 2) by moving the local check into the reset and stats function
of the mptsas, since that seems to be the only HBA that can't
(currently) control non-local phys.

James

Index: BUILD-2.6/drivers/scsi/scsi_transport_sas.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/scsi_transport_sas.c	2006-08-25 13:22:46.000000000 -0500
+++ BUILD-2.6/drivers/scsi/scsi_transport_sas.c	2006-08-25 13:29:40.000000000 -0500
@@ -266,9 +266,6 @@
 	struct sas_internal *i = to_sas_internal(shost->transportt);	\
 	int error;							\
 									\
-	if (!phy->local_attached)					\
-		return -EINVAL;						\
-									\
 	error = i->f->get_linkerrors ? i->f->get_linkerrors(phy) : 0;	\
 	if (error)							\
 		return error;						\
@@ -299,9 +296,6 @@
 	struct sas_internal *i = to_sas_internal(shost->transportt);
 	int error;
 
-	if (!phy->local_attached)
-		return -EINVAL;
-
 	error = i->f->phy_reset(phy, hard_reset);
 	if (error)
 		return error;
@@ -849,7 +843,7 @@
 	 * Only devices behind an expander are supported, because the
 	 * enclosure identifier is a SMP feature.
 	 */
-	if (phy->local_attached)
+	if (scsi_is_sas_phy_local(phy))
 		return -EINVAL;
 
 	error = i->f->get_enclosure_identifier(rphy, &identifier);
@@ -870,7 +864,7 @@
 	struct sas_internal *i = to_sas_internal(shost->transportt);
 	int val;
 
-	if (phy->local_attached)
+	if (scsi_is_sas_phy_local(phy))
 		return -EINVAL;
 
 	val = i->f->get_bay_identifier(rphy);
Index: BUILD-2.6/include/scsi/scsi_transport_sas.h
===================================================================
--- BUILD-2.6.orig/include/scsi/scsi_transport_sas.h	2006-08-25 13:23:08.000000000 -0500
+++ BUILD-2.6/include/scsi/scsi_transport_sas.h	2006-08-25 13:29:59.000000000 -0500
@@ -57,9 +57,6 @@
 	enum sas_linkrate	maximum_linkrate_hw;
 	enum sas_linkrate	maximum_linkrate;
 
-	/* internal state */
-	unsigned int		local_attached : 1;
-
 	/* link error statistics */
 	u32			invalid_dword_count;
 	u32			running_disparity_error_count;
@@ -196,4 +193,6 @@
 		rphy->identify.device_type == SAS_EDGE_EXPANDER_DEVICE;
 }
 
+#define scsi_is_sas_phy_local(phy)	scsi_is_host_device((phy)->dev.parent)
+
 #endif /* SCSI_TRANSPORT_SAS_H */
Index: BUILD-2.6/drivers/message/fusion/mptsas.c
===================================================================
--- BUILD-2.6.orig/drivers/message/fusion/mptsas.c	2006-08-25 13:30:28.000000000 -0500
+++ BUILD-2.6/drivers/message/fusion/mptsas.c	2006-08-25 13:35:50.000000000 -0500
@@ -852,6 +852,10 @@
 	dma_addr_t dma_handle;
 	int error;
 
+	/* FIXME: only have link errors on local phys */
+	if (!scsi_is_sas_phy_local(phy))
+		return -EINVAL;
+
 	hdr.PageVersion = MPI_SASPHY1_PAGEVERSION;
 	hdr.ExtPageLength = 0;
 	hdr.PageNumber = 1 /* page number 1*/;
@@ -924,6 +928,10 @@
 	unsigned long timeleft;
 	int error = -ERESTARTSYS;
 
+	/* FIXME: fusion doesn't allow non-local phy reset */
+	if (!scsi_is_sas_phy_local(phy))
+		return -EINVAL;
+
 	/* not implemented for expanders */
 	if (phy->identify.target_port_protocols & SAS_PROTOCOL_SMP)
 		return -ENXIO;
@@ -1570,9 +1578,6 @@
 
 	if (!phy_info->phy) {
 
-		if (local)
-			phy->local_attached = 1;
-
 		error = sas_phy_add(phy);
 		if (error) {
 			sas_phy_free(phy);

Re: [PATCH] scsi_transport_sas: remove local_attached flag

[Christoph Hellwig]

On Fri, Aug 25, 2006 at 01:48:18PM -0500, James Bottomley wrote:
> This flag denotes local attachment of the phy.  There are two problems
> with it:
> 
> 1) It's actually redundant ... you can get the same information simply
> by seeing whether a host is the phys parent
> 2) we condition a lot of phy parameters on it on the false assumption
> that we can only control local phys.  I'm wiring up phy resets in the
> aic94xx now, and it will be able to reset non-local phys as well.
> 
> I fixed 2) by moving the local check into the reset and stats function
> of the mptsas, since that seems to be the only HBA that can't
> (currently) control non-local phys.

You can of course do the reset and stats using the normal smp code from
libsas for fusion aswell.  I just added the flag as a quick hack because
I was waiting for the libsas merge to implement this instead of adding
a mptsas-specific implementation.

Re: [PATCH] scsi_transport_sas: remove local_attached flag

[James Bottomley]

On Mon, 2006-08-28 at 10:30 +0100, Christoph Hellwig wrote:
> You can of course do the reset and stats using the normal smp code from
> libsas for fusion aswell.  I just added the flag as a quick hack because
> I was waiting for the libsas merge to implement this instead of adding
> a mptsas-specific implementation.

Well ... we need an SMP input for that, which mptsas doesn't provide.
If it did, we could move SMP execution out of libsas into the transport
class proper and use it for the non-local expander stuff.

James

Re: [PATCH] scsi_transport_sas: remove local_attached flag

[Christoph Hellwig]

On Mon, Aug 28, 2006 at 09:01:36AM -0500, James Bottomley wrote:
> On Mon, 2006-08-28 at 10:30 +0100, Christoph Hellwig wrote:
> > You can of course do the reset and stats using the normal smp code from
> > libsas for fusion aswell.  I just added the flag as a quick hack because
> > I was waiting for the libsas merge to implement this instead of adding
> > a mptsas-specific implementation.
> 
> Well ... we need an SMP input for that, which mptsas doesn't provide.
> If it did, we could move SMP execution out of libsas into the transport
> class proper and use it for the non-local expander stuff.

I had something like that for the experimental SG_IO SMP passthrough
patch.  I'll see if I can do something useful once I get some time
allocated for it.

Re: [PATCH] scsi_transport_sas: remove local_attached flag

[Luben Tuikov]

--- James Bottomley <James.Bottomley@SteelEye.com> wrote:

> This flag denotes local attachment of the phy.  There are two problems
> with it:
> 
> 1) It's actually redundant ... you can get the same information simply
> by seeing whether a host is the phys parent
> 2) we condition a lot of phy parameters on it on the false assumption
> that we can only control local phys.  I'm wiring up phy resets in the
> aic94xx now, and it will be able to reset non-local phys as well.

Of course, the actual SAS Stack, doesn't have this "local_attached".

> 
> I fixed 2) by moving the local check into the reset and stats function
> of the mptsas, since that seems to be the only HBA that can't
> (currently) control non-local phys.
> 
> James
> 
> Index: BUILD-2.6/drivers/scsi/scsi_transport_sas.c
> ===================================================================
> --- BUILD-2.6.orig/drivers/scsi/scsi_transport_sas.c	2006-08-25 13:22:46.000000000 -0500
> +++ BUILD-2.6/drivers/scsi/scsi_transport_sas.c	2006-08-25 13:29:40.000000000 -0500
> @@ -266,9 +266,6 @@
>  	struct sas_internal *i = to_sas_internal(shost->transportt);	\
>  	int error;							\
>  									\
> -	if (!phy->local_attached)					\
> -		return -EINVAL;						\
> -									\
>  	error = i->f->get_linkerrors ? i->f->get_linkerrors(phy) : 0;	\
>  	if (error)							\
>  		return error;						\
> @@ -299,9 +296,6 @@
>  	struct sas_internal *i = to_sas_internal(shost->transportt);
>  	int error;
>  
> -	if (!phy->local_attached)
> -		return -EINVAL;
> -
>  	error = i->f->phy_reset(phy, hard_reset);
>  	if (error)
>  		return error;
> @@ -849,7 +843,7 @@
>  	 * Only devices behind an expander are supported, because the
>  	 * enclosure identifier is a SMP feature.
>  	 */
> -	if (phy->local_attached)
> +	if (scsi_is_sas_phy_local(phy))
>  		return -EINVAL;
>  
>  	error = i->f->get_enclosure_identifier(rphy, &identifier);
> @@ -870,7 +864,7 @@
>  	struct sas_internal *i = to_sas_internal(shost->transportt);
>  	int val;
>  
> -	if (phy->local_attached)
> +	if (scsi_is_sas_phy_local(phy))
>  		return -EINVAL;
>  
>  	val = i->f->get_bay_identifier(rphy);
> Index: BUILD-2.6/include/scsi/scsi_transport_sas.h
> ===================================================================
> --- BUILD-2.6.orig/include/scsi/scsi_transport_sas.h	2006-08-25 13:23:08.000000000 -0500
> +++ BUILD-2.6/include/scsi/scsi_transport_sas.h	2006-08-25 13:29:59.000000000 -0500
> @@ -57,9 +57,6 @@
>  	enum sas_linkrate	maximum_linkrate_hw;
>  	enum sas_linkrate	maximum_linkrate;
>  
> -	/* internal state */
> -	unsigned int		local_attached : 1;
> -
>  	/* link error statistics */
>  	u32			invalid_dword_count;
>  	u32			running_disparity_error_count;
> @@ -196,4 +193,6 @@
>  		rphy->identify.device_type == SAS_EDGE_EXPANDER_DEVICE;
>  }
>  
> +#define scsi_is_sas_phy_local(phy)	scsi_is_host_device((phy)->dev.parent)
> +
>  #endif /* SCSI_TRANSPORT_SAS_H */
> Index: BUILD-2.6/drivers/message/fusion/mptsas.c
> ===================================================================
> --- BUILD-2.6.orig/drivers/message/fusion/mptsas.c	2006-08-25 13:30:28.000000000 -0500
> +++ BUILD-2.6/drivers/message/fusion/mptsas.c	2006-08-25 13:35:50.000000000 -0500
> @@ -852,6 +852,10 @@
>  	dma_addr_t dma_handle;
>  	int error;
>  
> +	/* FIXME: only have link errors on local phys */
> +	if (!scsi_is_sas_phy_local(phy))
> +		return -EINVAL;
> +
>  	hdr.PageVersion = MPI_SASPHY1_PAGEVERSION;
>  	hdr.ExtPageLength = 0;
>  	hdr.PageNumber = 1 /* page number 1*/;
> @@ -924,6 +928,10 @@
>  	unsigned long timeleft;
>  	int error = -ERESTARTSYS;
>  
> +	/* FIXME: fusion doesn't allow non-local phy reset */
> +	if (!scsi_is_sas_phy_local(phy))
> +		return -EINVAL;
> +
>  	/* not implemented for expanders */
>  	if (phy->identify.target_port_protocols & SAS_PROTOCOL_SMP)
>  		return -ENXIO;
> @@ -1570,9 +1578,6 @@
>  
>  	if (!phy_info->phy) {
>  
> -		if (local)
> -			phy->local_attached = 1;
> -
>  		error = sas_phy_add(phy);
>  		if (error) {
>  			sas_phy_free(phy);
> 
> 
> -
> 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
> 

RE: [PATCH] scsi_transport_sas: remove local_attached flag

[Moore, Eric]

On Friday, August 25, 2006 12:48 PM, James Bottomley wrote: 

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
> Sent: Friday, August 25, 2006 12:48 PM
> To: linux-scsi
> Cc: Moore, Eric
> Subject: [PATCH] scsi_transport_sas: remove local_attached flag
> 
> This flag denotes local attachment of the phy.  There are two problems
> with it:
> 
> 1) It's actually redundant ... you can get the same information simply
> by seeing whether a host is the phys parent
> 2) we condition a lot of phy parameters on it on the false assumption
> that we can only control local phys.  I'm wiring up phy resets in the
> aic94xx now, and it will be able to reset non-local phys as well.
> 
> I fixed 2) by moving the local check into the reset and stats function
> of the mptsas, since that seems to be the only HBA that can't
> (currently) control non-local phys.
> 

This makes sense to remove this flag.

Perhaps this is time to decide on how to handle this for expanders.
At the time this was being developed in transport via Christophs help, 
we couldn't decide if this should be done by userspace, smp_passthru, or

fusion internal implementation.  Currently you can get link errors or 
phy/link reset via Doug Gilberts smputils. For instance you can get link

errors using smp_rep_phy_err_log, and send link/phy reset via
smp_phy_control.

If the tools are fine with you. Then nothing needs to be done.  I could 
add support in fusion for sending internal smp passthru, or we can
export 
a smp portal for you.  The smp portal should still be in the future
plans 
for the bsg driver.

Eric

RE: [PATCH] scsi_transport_sas: remove local_attached flag

[James Bottomley]

On Tue, 2006-08-29 at 09:24 -0600, Moore, Eric wrote:
> If the tools are fine with you. Then nothing needs to be done.  I
> could 
> add support in fusion for sending internal smp passthru, or we can
> export 
> a smp portal for you.  The smp portal should still be in the future
> plans 
> for the bsg driver.

I think the object should be to have all the SAS cards display the same
thing, so support for internal SMP passthrough looks to be the way to
implement this.  Also, if the SAS transport class does this, then we can
hook that up to BSG instead of having to have the drivers do it
individually.

I understood there was some architectural objection to doing SMP
passthrough from the LSI architects ... does that still exist?

James

RE: [PATCH] scsi_transport_sas: remove local_attached flag

[Moore, Eric]

On Tuesday, August 29, 2006 9:35 AM, James Bottomley wrote: 

> I think the object should be to have all the SAS cards 
> display the same
> thing, so support for internal SMP passthrough looks to be the way to
> implement this.  Also, if the SAS transport class does this, 
> then we can
> hook that up to BSG instead of having to have the drivers do it
> individually.

Ok, I will look into passing internal smp request so link_errors
and link/phy reset can work for expander phys.

It maybe a couple weeks as sles10 activities have been keeping
rather busy of late.

> 
> I understood there was some architectural objection to doing SMP
> passthrough from the LSI architects ... does that still exist?
> 

I think their main complaint was they didn't want transport layer doing
discovery. Something that would be redundant as fusion fw already
handles
that, and that would introduce a performance hit with all the primitives
bouncing around. What we currently do is reading config pages to
determine
what was setup by firmware, which is fine with them.  I believe you
already 
assured me that the transport layer wouldn't take over discovery if we 
exported a portal.  My idea of the portal was for Doug Gilberts smputils
to use generic interface, instead of the current mptctl interface.  To
what
extent would transport layer use the portal besides exporting it to
userspace?

Eric