[PATCH 0/2] aic94xx / sas: ref count update

[PATCH 0/2] aic94xx / sas: ref count update

[Mike Anderson]

The following patches allow the aic94xx module to be unloaded and loaded.
All release functions in the sas transport and scsi core are being called
now. 

The patch to scsi_transport_sas most likely will need to be expanded as
there appears to be some extra puts in error cases, but this patch allows
the release functions to be called where they where not being called
before.

We have tested the patches on a single and multi-node x86 box and a ppc64
system.

We are currently investigating an issue on an expander based with drive
hotplug insertion.

-andmike
--
Michael Anderson
andmike@us.ibm.com

[PATCH 1/2] sas transport: ref count update

[Mike Anderson]

Fix puts so that release functions will be called.

Signed-off-by: Mike Anderson <andmike@us.ibm.com>

 drivers/scsi/scsi_transport_sas.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_transport_sas.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/scsi_transport_sas.c	2006-03-22 13:22:22.000000000 -0800
+++ aic94xx-sas-2.6-patched/drivers/scsi/scsi_transport_sas.c	2006-03-22 13:24:27.000000000 -0800
@@ -406,8 +406,6 @@ struct sas_phy *sas_phy_alloc(struct dev
 	if (!phy)
 		return NULL;
 
-	get_device(parent);
-
 	phy->number = number;
 
 	device_initialize(&phy->dev);
@@ -484,7 +482,7 @@ sas_phy_delete(struct sas_phy *phy)
 	transport_remove_device(dev);
 	device_del(dev);
 	transport_destroy_device(dev);
-	put_device(dev->parent);
+	put_device(dev);
 }
 EXPORT_SYMBOL(sas_phy_delete);
 
@@ -990,7 +988,7 @@ sas_rphy_delete(struct sas_rphy *rphy)
 
 	parent->rphy = NULL;
 
-	put_device(&parent->dev);
+	put_device(dev);
 }
 EXPORT_SYMBOL(sas_rphy_delete);
 

Re: [PATCH 1/2] sas transport: ref count update

[Mike Anderson]

Christoph,

Can I get your comment(s) on this change.

Mike Anderson <andmike@us.ibm.com> wrote:
> Fix puts so that release functions will be called.
> 
> Signed-off-by: Mike Anderson <andmike@us.ibm.com>
> 
>  drivers/scsi/scsi_transport_sas.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_transport_sas.c
> ===================================================================
> --- aic94xx-sas-2.6-patched.orig/drivers/scsi/scsi_transport_sas.c	2006-03-22 13:22:22.000000000 -0800
> +++ aic94xx-sas-2.6-patched/drivers/scsi/scsi_transport_sas.c	2006-03-22 13:24:27.000000000 -0800
> @@ -406,8 +406,6 @@ struct sas_phy *sas_phy_alloc(struct dev
>  	if (!phy)
>  		return NULL;
>  
> -	get_device(parent);
> -
>  	phy->number = number;
>  
>  	device_initialize(&phy->dev);
> @@ -484,7 +482,7 @@ sas_phy_delete(struct sas_phy *phy)
>  	transport_remove_device(dev);
>  	device_del(dev);
>  	transport_destroy_device(dev);
> -	put_device(dev->parent);
> +	put_device(dev);
>  }
>  EXPORT_SYMBOL(sas_phy_delete);
>  
> @@ -990,7 +988,7 @@ sas_rphy_delete(struct sas_rphy *rphy)
>  
>  	parent->rphy = NULL;
>  
> -	put_device(&parent->dev);
> +	put_device(dev);
>  }
>  EXPORT_SYMBOL(sas_rphy_delete);

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH 1/2] sas transport: ref count update

[James Bottomley]

On Fri, 2006-03-24 at 19:56 -0800, Mike Anderson wrote:
> Can I get your comment(s) on this change.

This looks correct as far as it goes, but it's not complete.
sas_phy_free() and sas_rphy_free() now do too many parent puts with this
change (actually, two too many in each case, which looks like a bug in
the original code).  There also looks to be two spurious puts in the
rphy (for expander and end device) allocation error paths (again, a bug
in the original code).

James

Re: [PATCH 1/2] sas transport: ref count update

[Mike Anderson]

James Bottomley <James.Bottomley@SteelEye.com> wrote:
> On Fri, 2006-03-24 at 19:56 -0800, Mike Anderson wrote:
> > Can I get your comment(s) on this change.
> 
> This looks correct as far as it goes, but it's not complete.
> sas_phy_free() and sas_rphy_free() now do too many parent puts with this
> change (actually, two too many in each case, which looks like a bug in
> the original code).  There also looks to be two spurious puts in the
> rphy (for expander and end device) allocation error paths (again, a bug
> in the original code).

ok, here is an updated patch that covers these cases also. I moved to
using the releases functions in the *_free() cases also as it seems like a
good idea to use the release functions if you can. I have compiled and
tested it on a aic based system, but it does not call the free functions.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Fix puts so that release functions will be called.

Signed-off-by: Mike Anderson <andmike@us.ibm.com>

 drivers/scsi/scsi_transport_sas.c |   30 ++++++------------------------
 1 files changed, 6 insertions(+), 24 deletions(-)

Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_transport_sas.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/scsi_transport_sas.c	2006-03-24 13:58:17.000000000 -0800
+++ aic94xx-sas-2.6-patched/drivers/scsi/scsi_transport_sas.c	2006-03-27 09:23:43.000000000 -0800
@@ -406,8 +406,6 @@ struct sas_phy *sas_phy_alloc(struct dev
 	if (!phy)
 		return NULL;
 
-	get_device(parent);
-
 	phy->number = number;
 
 	device_initialize(&phy->dev);
@@ -459,10 +457,7 @@ EXPORT_SYMBOL(sas_phy_add);
 void sas_phy_free(struct sas_phy *phy)
 {
 	transport_destroy_device(&phy->dev);
-	put_device(phy->dev.parent);
-	put_device(phy->dev.parent);
-	put_device(phy->dev.parent);
-	kfree(phy);
+	put_device(&phy->dev);
 }
 EXPORT_SYMBOL(sas_phy_free);
 
@@ -484,7 +479,7 @@ sas_phy_delete(struct sas_phy *phy)
 	transport_remove_device(dev);
 	device_del(dev);
 	transport_destroy_device(dev);
-	put_device(dev->parent);
+	put_device(dev);
 }
 EXPORT_SYMBOL(sas_phy_delete);
 
@@ -800,7 +795,6 @@ struct sas_rphy *sas_end_device_alloc(st
 
 	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
 	if (!rdev) {
-		put_device(&parent->dev);
 		return NULL;
 	}
 
@@ -836,7 +830,6 @@ struct sas_rphy *sas_expander_alloc(stru
 
 	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
 	if (!rdev) {
-		put_device(&parent->dev);
 		return NULL;
 	}
 
@@ -910,6 +903,7 @@ EXPORT_SYMBOL(sas_rphy_add);
  */
 void sas_rphy_free(struct sas_rphy *rphy)
 {
+	struct device *dev = &rphy->dev;
 	struct Scsi_Host *shost = dev_to_shost(rphy->dev.parent->parent);
 	struct sas_host_attrs *sas_host = to_sas_host_attrs(shost);
 
@@ -917,21 +911,9 @@ void sas_rphy_free(struct sas_rphy *rphy
 	list_del(&rphy->list);
 	mutex_unlock(&sas_host->lock);
 
-	transport_destroy_device(&rphy->dev);
-	put_device(rphy->dev.parent);
-	put_device(rphy->dev.parent);
-	put_device(rphy->dev.parent);
-	if (rphy->identify.device_type == SAS_END_DEVICE) {
-		struct sas_end_device *edev = rphy_to_end_device(rphy);
-
-		kfree(edev);
-	} else {
-		/* must be expander */
-		struct sas_expander_device *edev =
-			rphy_to_expander_device(rphy);
+	transport_destroy_device(dev);
 
-		kfree(edev);
-	}
+	put_device(dev);
 }
 EXPORT_SYMBOL(sas_rphy_free);
 
@@ -971,7 +953,7 @@ sas_rphy_delete(struct sas_rphy *rphy)
 
 	parent->rphy = NULL;
 
-	put_device(&parent->dev);
+	put_device(dev);
 }
 EXPORT_SYMBOL(sas_rphy_delete);
 

Re: [PATCH 1/2] sas transport: ref count update

[Christoph Hellwig]

On Mon, Mar 27, 2006 at 09:37:28AM -0800, Mike Anderson wrote:
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> > On Fri, 2006-03-24 at 19:56 -0800, Mike Anderson wrote:
> > > Can I get your comment(s) on this change.
> > 
> > This looks correct as far as it goes, but it's not complete.
> > sas_phy_free() and sas_rphy_free() now do too many parent puts with this
> > change (actually, two too many in each case, which looks like a bug in
> > the original code).  There also looks to be two spurious puts in the
> > rphy (for expander and end device) allocation error paths (again, a bug
> > in the original code).
> 
> ok, here is an updated patch that covers these cases also. I moved to
> using the releases functions in the *_free() cases also as it seems like a
> good idea to use the release functions if you can. I have compiled and
> tested it on a aic based system, but it does not call the free functions.

the _free cases only existed because I didn't want to go through all
the driver model ->release stuff initially.  now that you do that the
_free functions can be removed.

[PATCH 2/2] aic94xx: ref count update

[Mike Anderson]

Allow aic94xx driver to unloaded and load.

Signed-off-by: Mike Anderson <andmike@us.ibm.com>
Signed-off-by: Alexis Bruemmer <alexisb@us.ibm.com>

 drivers/scsi/sas/sas_discover.c  |    2 ++
 drivers/scsi/sas/sas_init.c      |    2 --
 drivers/scsi/sas/sas_scsi_host.c |    1 +
 include/scsi/sas/sas_discover.h  |    3 ---
 4 files changed, 3 insertions(+), 5 deletions(-)

Index: aic94xx-sas-2.6-patched/include/scsi/sas/sas_discover.h
===================================================================
--- aic94xx-sas-2.6-patched.orig/include/scsi/sas/sas_discover.h	2006-03-22 13:22:22.000000000 -0800
+++ aic94xx-sas-2.6-patched/include/scsi/sas/sas_discover.h	2006-03-22 13:24:39.000000000 -0800
@@ -128,8 +128,6 @@ static inline int sas_notify_lldd_dev_fo
 	int res = 0;
 	struct sas_ha_struct *sas_ha = dev->port->ha;
 
-	if (!try_module_get(sas_ha->lldd_module))
-		return -ENOMEM;
 	if (sas_ha->lldd_dev_found) {
 		res = sas_ha->lldd_dev_found(dev);
 		if (res) {
@@ -146,7 +144,6 @@ static inline void sas_notify_lldd_dev_g
 {
 	if (dev->port->ha->lldd_dev_gone)
 		dev->port->ha->lldd_dev_gone(dev);
-	module_put(dev->port->ha->lldd_module);
 }
 
 static inline void sas_init_dev(struct domain_device *dev)
Index: aic94xx-sas-2.6-patched/drivers/scsi/sas/sas_scsi_host.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/sas/sas_scsi_host.c	2006-03-22 13:22:22.000000000 -0800
+++ aic94xx-sas-2.6-patched/drivers/scsi/sas/sas_scsi_host.c	2006-03-22 13:24:39.000000000 -0800
@@ -693,6 +693,7 @@ out_err:
 
 void sas_unregister_scsi_host(struct sas_ha_struct *sas_ha)
 {
+	sas_remove_host(sas_ha->core.shost);
 	scsi_remove_host(sas_ha->core.shost);
 	scsi_host_put(sas_ha->core.shost);
 	sas_ha->core.shost = NULL;
Index: aic94xx-sas-2.6-patched/drivers/scsi/sas/sas_init.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/sas/sas_init.c	2006-03-22 13:22:22.000000000 -0800
+++ aic94xx-sas-2.6-patched/drivers/scsi/sas/sas_init.c	2006-03-22 13:24:39.000000000 -0800
@@ -108,8 +108,6 @@ Undo:
 
 int sas_unregister_ha(struct sas_ha_struct *sas_ha)
 {
-	sas_unregister_devices(sas_ha);
-
 	if (sas_ha->lldd_max_execute_num > 1) {
 		sas_shutdown_queue(sas_ha);
 	}
Index: aic94xx-sas-2.6-patched/drivers/scsi/sas/sas_discover.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/sas/sas_discover.c	2006-03-22 13:22:22.000000000 -0800
+++ aic94xx-sas-2.6-patched/drivers/scsi/sas/sas_discover.c	2006-03-22 13:24:39.000000000 -0800
@@ -591,6 +591,8 @@ static void sas_unregister_domain_device
 
 	list_for_each_entry_reverse_safe(dev,n,&port->dev_list,dev_list_node)
 		sas_unregister_dev(dev);
+
+	complete(&port->port_gone_completion);
 }
 
 /* ---------- Discovery and Revalidation ---------- */