[PATCH] mptsas: eliminate ghost devices

[PATCH] mptsas: eliminate ghost devices

[James Bottomley]

One of the current problems the mptsas driver has is that of "ghost"
devices (these are devices the firmware reports as existing, but what
they actually represent are the parents of a lower device), so for
example in my dual expander configuration, three expanders actually show
up, two for the real expanders but a third is created because the
firmware reports that the lower expander also has another expander
connected (which is simply the port going back to the upper expander).

The attached patch eliminates all these ghosts by not allocating any
devices for them if the SAS address is the SAS address of the parent.

James

Index: BUILD-2.6/drivers/message/fusion/mptsas.c
===================================================================
--- BUILD-2.6.orig/drivers/message/fusion/mptsas.c
+++ BUILD-2.6/drivers/message/fusion/mptsas.c
@@ -1635,8 +1635,10 @@ static int mptsas_probe_one_phy(struct d
 	if (!mptsas_get_rphy(phy_info) && port && !port->rphy) {
 
 		struct sas_rphy *rphy;
+		struct device *parent;
 		struct sas_identify identify;
 
+		parent = dev->parent->parent;
 		/*
 		 * Let the hotplug_work thread handle processing
 		 * the adding/removing of devices that occur
@@ -1647,6 +1649,27 @@ static int mptsas_probe_one_phy(struct d
 				goto out;
 
 		mptsas_parse_device_info(&identify, &phy_info->attached);
+		if (scsi_is_host_device(parent)) {
+			struct mptsas_portinfo *port_info;
+			int i;
+
+			mutex_lock(&ioc->sas_topology_mutex);
+			port_info = mptsas_find_portinfo_by_handle(ioc,
+								   ioc->handle);
+			mutex_unlock(&ioc->sas_topology_mutex);
+
+			for (i = 0; i < port_info->num_phys; i++)
+				if (port_info->phy_info[i].identify.sas_address ==
+				    identify.sas_address)
+					goto out;
+
+		} else if (scsi_is_sas_rphy(parent)) {
+			struct sas_rphy *parent_rphy = dev_to_rphy(parent);
+			if (identify.sas_address ==
+			    parent_rphy->identify.sas_address)
+				goto out;
+		}
+
 		switch (identify.device_type) {
 		case SAS_END_DEVICE:
 			rphy = sas_end_device_alloc(port);
@@ -1698,6 +1721,7 @@ mptsas_probe_hba_phys(MPT_ADAPTER *ioc)
 		goto out_free_port_info;
 
 	mutex_lock(&ioc->sas_topology_mutex);
+	ioc->handle = hba->handle;
 	port_info = mptsas_find_portinfo_by_handle(ioc, hba->handle);
 	if (!port_info) {
 		port_info = hba;
Index: BUILD-2.6/drivers/message/fusion/mptbase.h
===================================================================
--- BUILD-2.6.orig/drivers/message/fusion/mptbase.h
+++ BUILD-2.6/drivers/message/fusion/mptbase.h
@@ -632,6 +632,7 @@ typedef struct _MPT_ADAPTER
 	struct mutex		 sas_discovery_mutex;
 	u8			 sas_discovery_runtime;
 	u8			 sas_discovery_ignore_events;
+	u16			 handle;
 	int			 sas_index; /* index refrencing */
 	MPT_SAS_MGMT		 sas_mgmt;
 	int			 num_ports;

[PATCH] mptsas: eliminate ghost devices

[Eric Moore]

On Friday, June 30, 2006 11:54 AM, James Bottomley wrote:
> The attached patch eliminates all these ghosts by not allocating any
> devices for them if the SAS address is the SAS address of the parent.

In older version of sas transport, it wouldn't work unless the ghost
remote phys were reported. I'm glad to see this restriction go away.

> @@ -1647,6 +1649,27 @@ static int mptsas_probe_one_phy(struct d
> 				goto out;
> 
> 		mptsas_parse_device_info(&identify, &phy_info->attached);
>+		if (scsi_is_host_device(parent)) {
>+			struct mptsas_portinfo *port_info;
>+			int i;
>+
>+			mutex_lock(&ioc->sas_topology_mutex);
>+			port_info = mptsas_find_portinfo_by_handle(ioc,
>+
>ioc->handle);
>+			mutex_unlock(&ioc->sas_topology_mutex);
>+
>+			for (i = 0; i < port_info->num_phys; i++)
>+				if
>(port_info->phy_info[i].identify.sas_address ==
>+				    identify.sas_address)
>+					goto out;
>+

This is code segment is not required.  This can be done much simplier.
All hba phys that are connected to an expander will have device type
equal to END_DEVICE and the target_port_protocols set to zero.  Here
is my suggested patch, to apply over previous patch making
expander port_ids zero based.

Signed-off-by: Eric Moore <Eric.Moore@lsil.com>


diff -uarpN b/drivers/message/fusion/mptsas.c a/drivers/message/fusion/mptsas.c
--- b/drivers/message/fusion/mptsas.c	2006-06-29 17:38:43.000000000 -0600
+++ a/drivers/message/fusion/mptsas.c	2006-06-30 14:57:37.000000000 -0600
@@ -1635,8 +1635,10 @@ static int mptsas_probe_one_phy(struct d
 	if (!mptsas_get_rphy(phy_info) && port && !port->rphy) {
 
 		struct sas_rphy *rphy;
+		struct device *parent;
 		struct sas_identify identify;
 
+		parent = dev->parent->parent;
 		/*
 		 * Let the hotplug_work thread handle processing
 		 * the adding/removing of devices that occur
@@ -1647,6 +1649,17 @@ static int mptsas_probe_one_phy(struct d
 				goto out;
 
 		mptsas_parse_device_info(&identify, &phy_info->attached);
+		if (scsi_is_host_device(parent)) {
+			if (!identify.target_port_protocols &&
+				(identify.device_type == SAS_END_DEVICE))
+				goto out;
+		} else if (scsi_is_sas_rphy(parent)) {
+			struct sas_rphy *parent_rphy = dev_to_rphy(parent);
+			if (identify.sas_address ==
+			    parent_rphy->identify.sas_address)
+				goto out;
+		}
+
 		switch (identify.device_type) {
 		case SAS_END_DEVICE:
 			rphy = sas_end_device_alloc(port);

Re: [PATCH] mptsas: eliminate ghost devices

[James Bottomley]

On Fri, 2006-06-30 at 15:07 -0600, Eric Moore wrote:
> +               if (scsi_is_host_device(parent)) {
> +                       if (!identify.target_port_protocols &&
> +                               (identify.device_type ==
> SAS_END_DEVICE))
> +                               goto out;

Won't this happen for any initiator, rather than just the parent (i.e.
in a multi-initiator environment we won't see anything even for remote
initiators)?

James

RE: [PATCH] mptsas: eliminate ghost devices

[Moore, Eric]

On Friday, June 30, 2006 3:25 PM, James Bottomley wrote: 
> 
> On Fri, 2006-06-30 at 15:07 -0600, Eric Moore wrote:
> > +               if (scsi_is_host_device(parent)) {
> > +                       if (!identify.target_port_protocols &&
> > +                               (identify.device_type ==
> > SAS_END_DEVICE))
> > +                               goto out;
> 
> Won't this happen for any initiator, rather than just the parent (i.e.
> in a multi-initiator environment we won't see anything even for remote
> initiators)?
> 

I believe my code is correct.  The scsi_is_host_device(parent) condition
will only be entered from an expander one level down from the host hba,
and this case
will remove for all rphys that would of been reported pointing back to
the 
host hba.  So in the case you have two initiators connected to the same
expander,
I think it would be expected that all ghost rphys need not be reported
for all host
host hba's connected to the same expander, right?  

If the remote initiator is located beyond the 1st leve expander, or
direct
connected to each other, this condition will not entered.

Eric

RE: [PATCH] mptsas: eliminate ghost devices

[James Bottomley]

On Fri, 2006-06-30 at 16:38 -0600, Moore, Eric wrote:
> I believe my code is correct.  The scsi_is_host_device(parent) condition
> will only be entered from an expander one level down from the host hba,
> and this case
> will remove for all rphys that would of been reported pointing back to
> the 
> host hba.  So in the case you have two initiators connected to the same
> expander,
> I think it would be expected that all ghost rphys need not be reported
> for all host
> host hba's connected to the same expander, right?  

Yes, but we want an end device for a remote HBA, which your condition
would also eliminate.  It shows useful information (like sas address and
parameters).

> If the remote initiator is located beyond the 1st leve expander, or
> direct
> connected to each other, this condition will not entered.

Right ... here we'll see the end device, so if we go with your patch
we'd see an end device for a remote HBA connected to a second level
expander but not for a first level one.  Whatever happens, we need to be
consistent ...

James

RE: [PATCH] mptsas: eliminate ghost devices

[Moore, Eric]

On Friday, June 30, 2006 4:59 PM, James Bottomley wrote: 

> 
> Yes, but we want an end device for a remote HBA, which your condition
> would also eliminate.  It shows useful information (like sas 
> address and
> parameters).

Yes good point. My thinking was the data is there on the host system
having
the remote HBA.  

> 
> > If the remote initiator is located beyond the 1st leve expander, or
> > direct
> > connected to each other, this condition will not entered.
> 
> Right ... here we'll see the end device, so if we go with your patch
> we'd see an end device for a remote HBA connected to a second level
> expander but not for a first level one.  Whatever happens, we 
> need to be
> consistent ...
> 

Agreed. Your doing referencing checking with host sas address, so 
the only rphys removed are those pointing back to the host, and not
other initiators.  So other initiators can exist on the 1st level
expanders,
as well as 2nd, 3rd, and so one.  Okay your patch is fine. Apply.

Eric