Re: [PATCH] ibmvscsi driver - sixth version

From: Jeff Garzik
Date: Wed Mar 31 2004 - 17:06:06 EST

Dave Boutcher wrote:

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.


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);
+ }

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

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

+ if (!evt_struct) {
+ }

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? :)

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at