aic94xx / sas: ref count update

PATCH In review Mike Anderson 3 participants 7 messages Mar 22, 2006 – Mar 28, 2006 SCSI

Share & tools

Mike Anderson

author

2006-03-22 21:41 UTC #0 lore raw

cover aic94xx / sas: ref count update

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
[email protected]

Mike Anderson

author

2006-03-22 21:42 UTC #1 lore raw

PATCH 1/2 sas transport: ref count update

Fix puts so that release functions will be called.

Signed-off-by Mike Anderson <[email protected]>

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

drivers/scsi/scsi_transport_sas.c +2 -4

Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_transport_sas.c
===================================================================
@@ -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);

Mike Anderson

author

2006-03-25 04:03 UTC #2 lore raw

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

Christoph,

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

Quoted 30 lines · Mike Anderson

Fix puts so that release functions will be called.
Signed-off-by Mike Anderson <[email protected]>
 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
===================================================================
drivers/scsi/scsi_transport_sas.c +1 -3
@@ -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);
 

drivers/scsi/scsi_transport_sas.c sas_rphy_delete(struct sas_rphy *rphy) · line 993

Quoted 7 lines

drivers/scsi/scsi_transport_sas.c +1 -1

@@ -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
[email protected]

James Bottomley

2006-03-25 19:43 UTC #3 lore raw

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

Quoted 1 line · Mike Anderson

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

Mike Anderson

author

2006-03-27 17:50 UTC #4 lore raw

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

Quoted 9 lines · James Bottomley

Quoted 1 line · Mike Anderson
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
[email protected]

Fix puts so that release functions will be called.

Signed-off-by Mike Anderson <[email protected]>

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

drivers/scsi/scsi_transport_sas.c +6 ~~-24~~

Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_transport_sas.c
===================================================================
@@ -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);

Christoph Hellwig

2006-03-28 15:08 UTC #5 lore raw

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

Quoted 11 lines · Mike Anderson

Quoted 9 lines · James Bottomley
Quoted 1 line · Mike Anderson
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).

Quoted 4 lines

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.

Mike Anderson

author

2006-03-22 21:44 UTC #6 lore raw

PATCH 2/2 aic94xx: ref count update

In reply to Mike Anderson

Allow aic94xx driver to unloaded and load.

Signed-off-by Mike Anderson <[email protected]>

Signed-off-by Alexis Bruemmer <[email protected]>

 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(-)

include/scsi/sas/sas_discover.h +0 -3

Index: aic94xx-sas-2.6-patched/include/scsi/sas/sas_discover.h
===================================================================
@@ -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)

drivers/scsi/sas/sas_scsi_host.c +1 -0

Index: aic94xx-sas-2.6-patched/drivers/scsi/sas/sas_scsi_host.c
===================================================================
@@ -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;

drivers/scsi/sas/sas_init.c +0 -2

Index: aic94xx-sas-2.6-patched/drivers/scsi/sas/sas_init.c
===================================================================
@@ -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);
 	}

drivers/scsi/sas/sas_discover.c +2 -0

Index: aic94xx-sas-2.6-patched/drivers/scsi/sas/sas_discover.c
===================================================================
@@ -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 ---------- */