[PATCH 3/5] reuse the original port when reenable the phy in a port

[PATCH 3/5] reuse the original port when reenable the phy in a port

[jack wang]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

>From 24abd7f05d2293e03aacbdd9d620d6378ea5e2aa Mon Sep 17 00:00:00 2001
From: Tom Peng <tom_peng@usish.com>
Date: Wed, 1 Jul 2009 19:04:01 +0800
Subject: [PATCH 3/5] reuse the original port when reenable the phy in a port

Signed-off-by: Tom Peng <tom_peng@usish.com>
Signed-off-by: Jack Wang <jack_wang@usish.com>
Signed-off-by: Lindar Liu <lindar_liu@usish.com>
---
 drivers/scsi/libsas/sas_port.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index e6ac59c..5aad165 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -69,16 +69,27 @@ static void sas_form_port(struct asd_sas_phy *phy)
 			SAS_DPRINTK("phy%d matched wide port%d\n", phy->id,
 				    port->id);
 			break;
-		} else if (*(u64 *) port->sas_addr == 0 &&
port->num_phys==0) {
-			memcpy(port->sas_addr, phy->sas_addr,
SAS_ADDR_SIZE);
-			break;
 		}
 		spin_unlock(&port->phy_list_lock);
 	}
+	/* The phy does not match any port currently existed, create new one
*/
+	if (i == sas_ha->num_phys) {
+		for (i = 0; i < sas_ha->num_phys; i++) {
+			port = sas_ha->sas_port[i];
+			spin_lock(&port->phy_list_lock);
+			if (*(u64 *)port->sas_addr == 0
+				&& port->num_phys == 0) {
+				memcpy(port->sas_addr, phy->sas_addr,
+					SAS_ADDR_SIZE);
+				break;
+			}
+			spin_unlock(&port->phy_list_lock);
+		}
+	}
 
 	if (i >= sas_ha->num_phys) {
 		printk(KERN_NOTICE "%s: couldn't find a free port, bug?\n",
-		       __func__);
+			__func__);
 		spin_unlock_irqrestore(&sas_ha->phy_port_lock, flags);
 		return;
 	}
-- 



[-- Attachment #2: 0003-reuse-the-original-port-when-reenable-the-phy-in-a-p.patch --]
[-- Type: application/octet-stream, Size: 1679 bytes --]

>From 24abd7f05d2293e03aacbdd9d620d6378ea5e2aa Mon Sep 17 00:00:00 2001
From: Tom Peng <tom_peng@usish.com>
Date: Wed, 1 Jul 2009 19:04:01 +0800
Subject: [PATCH 3/5] reuse the original port when reenable the phy in a port

Signed-off-by: Tom Peng <tom_peng@usish.com>
Signed-off-by: Jack Wang <jack_wang@usish.com>
Signed-off-by: Lindar Liu <lindar_liu@usish.com>
---
 drivers/scsi/libsas/sas_port.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index e6ac59c..5aad165 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -69,16 +69,27 @@ static void sas_form_port(struct asd_sas_phy *phy)
 			SAS_DPRINTK("phy%d matched wide port%d\n", phy->id,
 				    port->id);
 			break;
-		} else if (*(u64 *) port->sas_addr == 0 && port->num_phys==0) {
-			memcpy(port->sas_addr, phy->sas_addr, SAS_ADDR_SIZE);
-			break;
 		}
 		spin_unlock(&port->phy_list_lock);
 	}
+	/* The phy does not match any port currently existed, create new one */
+	if (i == sas_ha->num_phys) {
+		for (i = 0; i < sas_ha->num_phys; i++) {
+			port = sas_ha->sas_port[i];
+			spin_lock(&port->phy_list_lock);
+			if (*(u64 *)port->sas_addr == 0
+				&& port->num_phys == 0) {
+				memcpy(port->sas_addr, phy->sas_addr,
+					SAS_ADDR_SIZE);
+				break;
+			}
+			spin_unlock(&port->phy_list_lock);
+		}
+	}
 
 	if (i >= sas_ha->num_phys) {
 		printk(KERN_NOTICE "%s: couldn't find a free port, bug?\n",
-		       __func__);
+			__func__);
 		spin_unlock_irqrestore(&sas_ha->phy_port_lock, flags);
 		return;
 	}
-- 

Re: [PATCH 3/5] reuse the original port when reenable the phy in a port

[James Bottomley]

On Wed, 2009-07-01 at 20:37 +0800, jack wang wrote:
> >From 24abd7f05d2293e03aacbdd9d620d6378ea5e2aa Mon Sep 17 00:00:00 2001
> From: Tom Peng <tom_peng@usish.com>
> Date: Wed, 1 Jul 2009 19:04:01 +0800
> Subject: [PATCH 3/5] reuse the original port when reenable the phy in a port
> 
> Signed-off-by: Tom Peng <tom_peng@usish.com>
> Signed-off-by: Jack Wang <jack_wang@usish.com>
> Signed-off-by: Lindar Liu <lindar_liu@usish.com>
> ---
>  drivers/scsi/libsas/sas_port.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
> index e6ac59c..5aad165 100644
> --- a/drivers/scsi/libsas/sas_port.c
> +++ b/drivers/scsi/libsas/sas_port.c
> @@ -69,16 +69,27 @@ static void sas_form_port(struct asd_sas_phy *phy)
>  			SAS_DPRINTK("phy%d matched wide port%d\n", phy->id,
>  				    port->id);
>  			break;
> -		} else if (*(u64 *) port->sas_addr == 0 &&
> port->num_phys==0) {
> -			memcpy(port->sas_addr, phy->sas_addr,
> SAS_ADDR_SIZE);
> -			break;
>  		}
>  		spin_unlock(&port->phy_list_lock);
>  	}
> +	/* The phy does not match any port currently existed, create new one
> */
> +	if (i == sas_ha->num_phys) {
> +		for (i = 0; i < sas_ha->num_phys; i++) {
> +			port = sas_ha->sas_port[i];
> +			spin_lock(&port->phy_list_lock);
> +			if (*(u64 *)port->sas_addr == 0
> +				&& port->num_phys == 0) {
> +				memcpy(port->sas_addr, phy->sas_addr,
> +					SAS_ADDR_SIZE);
> +				break;
> +			}
> +			spin_unlock(&port->phy_list_lock);
> +		}
> +	}

I don't understand why you need this.  It seems identical to the else
leg of the if in the for loop above it, so it wouldn't seem to add
anything to the code.

James

Re: [PATCH 3/5] reuse the original port when reenable the phy in a port

[tom]

On Wed, 2009-07-01 at 20:37 +0800, jack wang wrote:
> >From 24abd7f05d2293e03aacbdd9d620d6378ea5e2aa Mon Sep 17 00:00:00 2001
> From: Tom Peng <tom_peng@usish.com>
> Date: Wed, 1 Jul 2009 19:04:01 +0800
> Subject: [PATCH 3/5] reuse the original port when reenable the phy in a
port
> 
> Signed-off-by: Tom Peng <tom_peng@usish.com>
> Signed-off-by: Jack Wang <jack_wang@usish.com>
> Signed-off-by: Lindar Liu <lindar_liu@usish.com>
> ---
>  drivers/scsi/libsas/sas_port.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_port.c
b/drivers/scsi/libsas/sas_port.c
> index e6ac59c..5aad165 100644
> --- a/drivers/scsi/libsas/sas_port.c
> +++ b/drivers/scsi/libsas/sas_port.c
> @@ -69,16 +69,27 @@ static void sas_form_port(struct asd_sas_phy *phy)
>  			SAS_DPRINTK("phy%d matched wide port%d\n", phy->id,
>  				    port->id);
>  			break;
> -		} else if (*(u64 *) port->sas_addr == 0 &&
> port->num_phys==0) {
> -			memcpy(port->sas_addr, phy->sas_addr,
> SAS_ADDR_SIZE);
> -			break;
>  		}
>  		spin_unlock(&port->phy_list_lock);
>  	}
> +	/* The phy does not match any port currently existed, create new one
> */
> +	if (i == sas_ha->num_phys) {
> +		for (i = 0; i < sas_ha->num_phys; i++) {
> +			port = sas_ha->sas_port[i];
> +			spin_lock(&port->phy_list_lock);
> +			if (*(u64 *)port->sas_addr == 0
> +				&& port->num_phys == 0) {
> +				memcpy(port->sas_addr, phy->sas_addr,
> +					SAS_ADDR_SIZE);
> +				break;
> +			}
> +			spin_unlock(&port->phy_list_lock);
> +		}
> +	}

I don't understand why you need this.  It seems identical to the else
leg of the if in the for loop above it, so it wouldn't seem to add
anything to the code.

James


--

Hi James,
Just think one situation: a HBA has two wide ports, port 0 (phy0, phy1,
phy2, phy3) attached to expander A, port1 (phy4, phy5, phy6, phy7) attached
to expander B, supposed the cable of port0 was plugged out, port1 still
here. After a while, for some reason phy4(or phy5 phy6 phy7) linked down
then linked up, the code will added this phy to form a new port, it is
port0, rather than added this phy to port1.

I have tested this bug, use SMP utility phy_control to disable phy4
(expander B)and then enabled it, it formed new port: port0.

To fix this, we can detect all of the ports existed and if no any port
matched, we form new one.

Best Regards

****************************************************************************
******************

Tom Peng     Software Engineer

Network & Storage Division, R & D Center

Universal Scientific Industrial (Shanghai) Co., Ltd

****************************************************************************
********************

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 3/5] reuse the original port when reenable the phy in a port

[James Bottomley]

On Thu, 2009-07-09 at 10:12 +0800, tom wrote:
> On Wed, 2009-07-01 at 20:37 +0800, jack wang wrote:
> > >From 24abd7f05d2293e03aacbdd9d620d6378ea5e2aa Mon Sep 17 00:00:00 2001
> > From: Tom Peng <tom_peng@usish.com>
> > Date: Wed, 1 Jul 2009 19:04:01 +0800
> > Subject: [PATCH 3/5] reuse the original port when reenable the phy in a
> port
> > 
> > Signed-off-by: Tom Peng <tom_peng@usish.com>
> > Signed-off-by: Jack Wang <jack_wang@usish.com>
> > Signed-off-by: Lindar Liu <lindar_liu@usish.com>
> > ---
> >  drivers/scsi/libsas/sas_port.c |   19 +++++++++++++++----
> >  1 files changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/scsi/libsas/sas_port.c
> b/drivers/scsi/libsas/sas_port.c
> > index e6ac59c..5aad165 100644
> > --- a/drivers/scsi/libsas/sas_port.c
> > +++ b/drivers/scsi/libsas/sas_port.c
> > @@ -69,16 +69,27 @@ static void sas_form_port(struct asd_sas_phy *phy)
> >  			SAS_DPRINTK("phy%d matched wide port%d\n", phy->id,
> >  				    port->id);
> >  			break;
> > -		} else if (*(u64 *) port->sas_addr == 0 &&
> > port->num_phys==0) {
> > -			memcpy(port->sas_addr, phy->sas_addr,
> > SAS_ADDR_SIZE);
> > -			break;
> >  		}
> >  		spin_unlock(&port->phy_list_lock);
> >  	}
> > +	/* The phy does not match any port currently existed, create new one
> > */
> > +	if (i == sas_ha->num_phys) {
> > +		for (i = 0; i < sas_ha->num_phys; i++) {
> > +			port = sas_ha->sas_port[i];
> > +			spin_lock(&port->phy_list_lock);
> > +			if (*(u64 *)port->sas_addr == 0
> > +				&& port->num_phys == 0) {
> > +				memcpy(port->sas_addr, phy->sas_addr,
> > +					SAS_ADDR_SIZE);
> > +				break;
> > +			}
> > +			spin_unlock(&port->phy_list_lock);
> > +		}
> > +	}
> 
> I don't understand why you need this.  It seems identical to the else
> leg of the if in the for loop above it, so it wouldn't seem to add
> anything to the code.
> 
> James
> 
> 
> --
> 
> Hi James,
> Just think one situation: a HBA has two wide ports, port 0 (phy0, phy1,
> phy2, phy3) attached to expander A, port1 (phy4, phy5, phy6, phy7) attached
> to expander B, supposed the cable of port0 was plugged out, port1 still
> here. After a while, for some reason phy4(or phy5 phy6 phy7) linked down
> then linked up, the code will added this phy to form a new port, it is
> port0, rather than added this phy to port1.
> 
> I have tested this bug, use SMP utility phy_control to disable phy4
> (expander B)and then enabled it, it formed new port: port0.
> 
> To fix this, we can detect all of the ports existed and if no any port
> matched, we form new one.

OK, get it now, sorry ... missed the else clause removal in the patch.

James