[openib-general] SRP: [PATCH] Releasing the scsi_host when unloading

[Ishai Rabinovitz]

On Wed, May 17, 2006 at 02:55:57AM +0300, Roland Dreier wrote:
>  > +			/*
>  > +			 * We need 2 scsi_host_put becuase there are two get:
>  > +			 *  in scsi_host_alloc and in scsi_add_host
>  > +			 */
>  > +			scsi_host_put(target->scsi_host);
>  >  			scsi_host_put(target->scsi_host);
> 
> Hmm, this doesn't seem right to me.  If I try this, then I get a crash
> because the scsi_host is already gone after the first put.  I verified
> that the reference count is 1 before these puts, and with the
> unmodified module I don't see anything left in /sys/class/scsi_host
> after unloading the module.
> 
> What kernel are you seeing problems with?  I'm testing with an
> up-to-date git kernel, although I doubt it makes a difference (did
> SCSI reference counting change recently??).
> 
> I do think there are some extra scsi_host_put() calls in
> srp_remove_work() -- I think the double scsi_host_put() dates back to
> a version (which I may never even have checked in) where there was a
> scsi_host_get() to avoid the scsi_host going away between the
> schedule_work() and srp_remove_work() actually running.
> 
> So the patch below seems correct to me.
> 
> What do you think?
> 
> --- linux-kernel/infiniband/ulp/srp/ib_srp.c	(revision 7245)
> +++ linux-kernel/infiniband/ulp/srp/ib_srp.c	(working copy)
> @@ -353,7 +356,6 @@ static void srp_remove_work(void *target
>  	spin_lock_irq(target->scsi_host->host_lock);
>  	if (target->state != SRP_TARGET_DEAD) {
>  		spin_unlock_irq(target->scsi_host->host_lock);
> -		scsi_host_put(target->scsi_host);
>  		return;
>  	}
>  	target->state = SRP_TARGET_REMOVED;
> @@ -367,8 +369,6 @@ static void srp_remove_work(void *target
>  	ib_destroy_cm_id(target->cm_id);
>  	srp_free_target_ib(target);
>  	scsi_host_put(target->scsi_host);
> -	/* And another put to really free the target port... */
> -	scsi_host_put(target->scsi_host);
>  }
>  
>  static int srp_connect_target(struct srp_target_port *target)
> 
> 

Roland, 

As I told you before, your patch looks correct.
Are you going to apply it?

-- 
Ishai Rabinovitz