Re: [PATCH] ibmvscsi driver - sixth version

Jeff Garzik
2004-03-31T22:02:18Z

Dave Boutcher wrote:
> James,
>
> Here is the next version of the ibmvscsi lldd. I know you were waiting
> for the fix that correctly checks for errors on the dma_map_foo calls,
> which is included. The other functional changes are to implement
> device_reset, and to surface a bunch of adapter attributes through
> shost_attrs.
>
> This driver has been under test for the last couple of months, and is
> scheduled to ship in a couple of distributions shortly. There are a few
> bug fixes included in this patch as well, mostly in the area of abort
> processing and correctly handling edge conditions (freeing buffers in
> error paths etc.)
>
> Comments always welcomed. I would like to get this upstream if I can,
> since the linux distributors are complaining slightly that it is not.

Comments:

1) Would be nice to eliminate module options for commands-per-lun,
max-requests etc. and actually have the driver figure out the real
needs. One or two options could fall into the "performance tuning"
category, and stay, but the others are really dependent on the
underlying configuration and underlying limits.

2) why is one-descriptor a special case in map_sg_data()? You proceed
to do the same thing in a loop, right below that... AFAICS you can just
use the loop, and clamp the number of scatterlist elements to one.

3) in the following code...

+ if ((evt_struct->crq.format == VIOSRP_SRP_FORMAT) &&
+ (atomic_dec_if_positive(&hostdata->request_limit) < 0)) {
+ printk("ibmvscsi: Warning, request_limit exceeded\n");
+ unmap_cmd_data(&evt_struct->evt->srp.cmd, hostdata->dev);
+ ibmvscsi_free_event_struct(&hostdata->pool, evt_struct);
+ return SCSI_MLQUEUE_HOST_BUSY;
+ }

is the code prepared to deal with hostdata->request_limit being negative?

4) style: don't bother with unneeded brackets...

+ if (!evt_struct) {
+ return SCSI_MLQUEUE_HOST_BUSY;
+ }
+

5) two issues with your abort handler...

+ spin_unlock_irq(hostdata->host->host_lock);
+ wait_for_completion(&evt->comp);
+ spin_lock_irq(hostdata->host->host_lock);

5a) are there recursion or deadlock issues, when you release the lock
like this?

5b) IMO unconditional spin_{un}lock_irq() is unwise... the
->eh_abort_handler() saved the flags, so you might be restoring things
to an incorrect state.

Since you're already inside spin_lock_irqsave(), and #5a is not an
issue, just use spin_unlock() and spin_lock().

6) ditto ibmvscsi_eh_device_reset_handler

7) style: the names of many of the structures defined by this driver are
IN ALL CAPS AND SHOUTING :) Structs should not be in all caps.

8) purge_requests() locking looks bogus. It is safe to complete a SCSI
command inside the host lock.

9) ibmvscsi_do_host_config() continues, after a DMA mapping error

10) what the heck is this??? do some people not know they are running
Linux???

+static ssize_t show_host_os_type(struct class_device *class_dev, char *buf)
+{
+ struct Scsi_Host *shost = class_to_shost(class_dev);
+ struct ibmvscsi_host_data *hostdata =
+ (struct ibmvscsi_host_data *)shost->hostdata;
+ int len;
+
+ len = snprintf(buf, PAGE_SIZE, "%d\n", hostdata->madapter_info.os_type);
+ return len;
+}
+
+static struct class_device_attribute ibmvscsi_host_os_type = {
+ .attr = {
+ .name = "os_type",
+ .mode = S_IRUGO,
+ },
+ .show = show_host_os_type,
+};
+

11) I'm not sure ".emulated = 1" is correct, since you are really
talking SCSI. But this is arguable, as is the value of 'emulated' flag
itself...

12) in ibmvscsi_probe(), you want to use TASK_UNINTERRUPTIBLE here:

+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(5);

13) in the code pasted in #12, you should pass a value calculated using
the 'HZ' constant.

14) why are you faking a PCI bus? The following is very, very wrong:

+static struct pci_dev iseries_vscsi_dev = {
+ .dev.bus = &pci_bus_type,
+ .dev.bus_id = "vscsi",
+ .dev.release = noop_release

Did I mention "very" wrong? :)