Re: [PATCH] scsi host/scsi device ref count cleanup 3/4

[Christoph Hellwig <hch@infradead.org>]

On Tue, Jul 08, 2003 at 03:27:07PM -0700, Mike Anderson wrote:
> -andmike
> --
> Michael Anderson
> andmike@us.ibm.com
> 
> DESC
> This is a cleanup patch for scsi_host removal.
> 	- Addition of a host_flags member to the scsi_host strucuture to
> 	  contain "state" of the host instance.
> 	- Added SHOST_ADD, SHOST_DEL, SHOST_CANCEL, SHOST_RECOVERY states
> 	  for a scsi host

Hmm, I'd call this shost_state.


> +	spin_lock_irqsave(shost->host_lock, flags);
> +	__set_bit(SHOST_CANCEL, &shost->host_flags);
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +	device_for_each_child(&shost->host_gendev, &recovery,
> +			      scsi_device_cancel);
> +	wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY,
> +						&shost->host_flags)));

Using the non-atomic and the atomic bitops on one variable doesn't
get you atomicity.  Always use the atomic ones if you need them sometimes.

> +/**
> + * scsi_remove_host - remove a scsi host
> + * @shost:	a pointer to a scsi host to remove
> + **/
> +void scsi_remove_host(struct Scsi_Host *shost)

A, cool I already had a patch pending to make this a void retval.

> -void scsi_host_get(struct Scsi_Host *shost)
> +struct Scsi_Host * scsi_host_get(struct Scsi_Host *shost)

struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)

>  {
> -	get_device(&shost->host_gendev);
> -	class_device_get(&shost->class_dev);
> +	struct Scsi_Host *res_shost = shost;
> +
> +	if (res_shost) {

I don't think we should allow for a NULL argument here.  Also
can't we just return NULL in the error case?  That would
simplify the code to:

struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
{
	if (test_bit(SHOST_DEL, &shost->host_flags) ||
	    !get_device(&res_shost->host_gendev))
		return NULL;
	return shost;
}

>  void scsi_host_put(struct Scsi_Host *shost)
>  {
> -	class_device_put(&shost->class_dev);

Can you explain why we don't need that anymore?  /me wants to learn a bit
more about deep LDM magic..

> -void scsi_set_device_offline(struct scsi_device *sdev)
> +int scsi_device_cancel(struct device *dev, void *data)
>  {
>  	struct scsi_cmnd *scmd;
>  	LIST_HEAD(active_list);
>  	struct list_head *lh, *lh_sf;
>  	unsigned long flags;
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +	unsigned int recovery = *(unsigned int *)data;

I think the prototype should be

int scsi_device_cancel(struct scsi_device *sdev, int recovery)

and there should be a small wrapper for the LDM iterator.

> +shost_rd_attr(host_flags, "%lx\n");

Shouldn't these better be printed in ascii instead of the binary
presentation that might aswell change form one kernelrelease to
another?

> +/*
> + * shost flags
> + */
> +#define SHOST_ADD	1
> +#define SHOST_DEL	2
> +#define SHOST_CANCEL	3
> +#define SHOST_RECOVERY	4

Make this an enum?

>  	/* Dissociate from the USB device */
>  	dissociate_dev(us);
>  
> -	/* Begin the SCSI host removal sequence */
> -	if (scsi_remove_host(us->host)) {
> -		US_DEBUGP("-- SCSI refused to remove the host\n");
> -		BUG();
> -		return;
> -	}
> +	scsi_remove_host(us->host);

Also in usb-storage the loop over my_devices to set them offline
just before scsi_remove_host should be nuked now, shouldn't it?