Comments:Hmmm...well, I could collapse the two together, since commands_per_lun
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.The SRP spec has two different buffer formats: SRP_DIRECT_BUFFER for a
3) in the following code...Yes....that is one of the things that would drive you down that path.
+ 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...Fair enough.
+ if (!evt_struct) {
+ return SCSI_MLQUEUE_HOST_BUSY;
+ }
+
OK, two issues. There are a bunch of SCSI LLDDs that use this same
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.Ya, copied from less sane operating systems. I'll fix.
8) purge_requests() locking looks bogus. It is safe to complete a SCSI command inside the host lock.I'll fix.
9) ibmvscsi_do_host_config() continues, after a DMA mapping errorOops.
10) what the heck is this??? do some people not know they are running Linux???This whole driver has to do with adapter sharing....and this answers
+static ssize_t show_host_os_type(struct class_device *class_dev, char *buf)
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...Agreed...I'll walk either side of the street on that one.
12) in ibmvscsi_probe(), you want to use TASK_UNINTERRUPTIBLE here:Hmmm...above code copied from the qlogic driver...and it looked reasonable
+ 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:Because for iseries it is implemented in the pci code. While it may
+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? :)