Re: [PATCH] async scsi scanning, version 12

Andrew Morton
2006-09-28T20:32:49Z

On Thu, 28 Sep 2006 12:24:38 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> Add ability to scan scsi busses asynchronously
>
> ...

> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -140,6 +140,8 @@ obj-$(CONFIG_CHR_DEV_SCH) += ch.o
> # This goes last, so that "real" scsi devices probe earlier
> obj-$(CONFIG_SCSI_DEBUG) += scsi_debug.o
>
> +obj-$(CONFIG_SCSI) += scsi_wait_scan.o
> +
> scsi_mod-y += scsi.o hosts.o scsi_ioctl.o constants.o \
> scsicam.o scsi_error.o scsi_lib.o \
> scsi_scan.o scsi_sysfs.o \
>
> ...

I think that's supposed to go into scsi_mod-y, at line 146.

>
> +static char scsi_scan_type[6] = "sync";

That wasted a byte.

> +
> +module_param_string(scan, scsi_scan_type, sizeof(scsi_scan_type), S_IRUGO);
> +MODULE_PARM_DESC(scan, "sync, async or none");
> +
> /*
> * max_scsi_report_luns: the maximum number of LUNS that will be
> * returned from the REPORT LUNS command. 8 times this value must
> @@ -108,6 +115,53 @@ MODULE_PARM_DESC(inq_timeout,
> "Timeout (in seconds) waiting for devices to answer INQUIRY."
> " Default is 5. Some non-compliant devices need more.");
>
> +static spinlock_t async_scan_lock = SPIN_LOCK_UNLOCKED;

DEFINE_SPINLOCK()

> +static LIST_HEAD(scanning_hosts);
> +
> +struct async_scan_data {
> + struct list_head list;
> + struct Scsi_Host *shost;
> + struct completion prev_finished;
> +};
> +
> +int scsi_complete_async_scans(void)
> +{
> + struct async_scan_data *data;
> +
> + do {
> + if (list_empty(&scanning_hosts))
> + return 0;
> + data = kmalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + msleep(1);
> + } while (!data);

ick. Immortal allocation loops are poor form. If there's really no
alternative, please use __GFP_NOFAIL, so people can easily grep for your
sins.

> + data->shost = NULL;
> + init_completion(&data->prev_finished);
> +
> + spin_lock(&async_scan_lock);
> + if (list_empty(&scanning_hosts))
> + goto done;
> + list_add_tail(&data->list, &scanning_hosts);
> + spin_unlock(&async_scan_lock);

"If the list is not empty, stick something on it".

What an unusual thing to do. I'm sure it makes sense, but some comments
explaining what's going on would be nice.

> + printk(KERN_INFO "scsi: waiting for bus probes to complete ...\n");
> + wait_for_completion(&data->prev_finished);
> +
> + spin_lock(&async_scan_lock);
> + list_del(&data->list);
> + done:
> + spin_unlock(&async_scan_lock);
> +
> + kfree(data);
> + return 0;
> +}
> +
> +#ifdef MODULE
> +/* Only exported for the benefit of scsi_wait_scan */
> +EXPORT_SYMBOL_GPL(scsi_complete_async_scans);
> +#endif

Is that actually needed? AFACIT this .o file will just get linked against
the one which contains scsi_wait_scan() anyway.