ibmvscsi driver - third version

This is the third version of the SCSI driver to support the adapter implemented by the IBM Power5 firmware. I have incorporated all comments from the previous submissions. Probably the most significant functional change is that all locking is done with the host_lock now.

If anyone would prefer to review the code in a prettier c2html format, you can see it at
http://www-users.cs.umn.edu/~boutcher/ibmvscsi/

And there is a tarball at ftp://ftp.cs.umn.edu/dept/users/boutcher/ibmvscsi-feb22b.tar.gz

Thanks,

Dave Boutcher

diff -uNr --exclude=SCCS --exclude=*.o --exclude=*.cmd linux-2.5/drivers/scsi/ibmvscsi/Makefile ppc64-2.5new/drivers/scsi/ibmvscsi/Makefile
@@ -0,0 +1,7 @@
+obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsic.o
+
+ibmvscsic-y			+= ibmvscsi.o
+ibmvscsic-$(CONFIG_PPC_ISERIES)	+= iseries_vscsi.o 
+ibmvscsic-$(CONFIG_PPC_PSERIES)	+= rpa_vscsi.o 
+ 
+

@@ -725,6 +725,15 @@
 	  To compile this driver as a module, choose M here: the
 	  module will be called ips.
 
+config SCSI_IBMVSCSI
+	tristate "IBM Virtual SCSI support"
+	depends on PPC_PSERIES || PPC_ISERIES
+	help
+	  This is the IBM Virtual SCSI Client
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ibmvscsic.
+
 config SCSI_INITIO
 	tristate "Initio 9100U(W) support"
 	depends on PCI && SCSI && BROKEN

@@ -114,6 +114,7 @@
 obj-$(CONFIG_SCSI_SATA_PROMISE)	+= libata.o sata_promise.o
 obj-$(CONFIG_SCSI_SATA_SIL)	+= libata.o sata_sil.o
 obj-$(CONFIG_SCSI_SATA_VIA)	+= libata.o sata_via.o
+obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsi/
 
 obj-$(CONFIG_ARM)		+= arm/
 

This one still has the issue with error returns from queuecommand.

Here are the rules for queuecommand again:

- either your return 0, then you must finish the command through it's ->done callback
- or you return SCSI_MLQUEUE_HOST_BUSY/SCSI_MLQUEUE_DEVICE_BUSY and must _not_ call ->done.

There's also a fair share of style issues left. What about simply running it through scripts/Lindent?

Christoph, thanks for catching the queuecommand bug...I should have fixed that last time around.

Here is what I expect to be the final patch. All of the files have been nicely Lindent-ed. The only functional change is:
===== ibmvscsi.c 1.8 vs edited =====

@@ -625,8 +625,6 @@
 	if (!evt_struct) {
 		printk(KERN_ERR
 		       "ibmvscsi: can't convert struct scsi_cmnd to event\n");
-		cmd->result = DID_ERROR << 16;
-		done(cmd);
 		return SCSI_MLQUEUE_HOST_BUSY;
 	}
 

James, can you please add this to the tree?

Thanks,

Dave B

diff -uNr --exclude=SCCS --exclude=*.o --exclude=*.cmd linux-2.5/drivers/scsi/ibmvscsi/Makefile ppc64-2.5new/drivers/scsi/ibmvscsi/Makefile
@@ -0,0 +1,7 @@
+obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsic.o
+
+ibmvscsic-y			+= ibmvscsi.o
+ibmvscsic-$(CONFIG_PPC_ISERIES)	+= iseries_vscsi.o 
+ibmvscsic-$(CONFIG_PPC_PSERIES)	+= rpa_vscsi.o 
+ 
+

@@ -725,6 +725,15 @@
 	  To compile this driver as a module, choose M here: the
 	  module will be called ips.
 
+config SCSI_IBMVSCSI
+	tristate "IBM Virtual SCSI support"
+	depends on PPC_PSERIES || PPC_ISERIES
+	help
+	  This is the IBM Virtual SCSI Client
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ibmvscsic.
+
 config SCSI_INITIO
 	tristate "Initio 9100U(W) support"
 	depends on PCI && SCSI && BROKEN

@@ -114,6 +114,7 @@
 obj-$(CONFIG_SCSI_SATA_PROMISE)	+= libata.o sata_promise.o
 obj-$(CONFIG_SCSI_SATA_SIL)	+= libata.o sata_sil.o
 obj-$(CONFIG_SCSI_SATA_VIA)	+= libata.o sata_via.o
+obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsi/
 
 obj-$(CONFIG_ARM)		+= arm/
 

+static int map_sg_data(struct scsi_cmnd *cmd,
+ struct SRP_CMD *srp_cmd, struct device *dev)
+{
[...]
+ data->virtual_address = sg[0].dma_address;
+ data->length = sg[0].dma_length;

This is wrong, you must use the data accessors sg_dma_addres() and sg_dma_len().

+static int map_single_data(struct scsi_cmnd *cmd,
+ struct SRP_CMD *srp_cmd, struct device *dev)
+{
[...]
+ data->virtual_address =
+ (u64) (unsigned long)dma_map_single(dev, cmd->request_buffer,
+ cmd->request_bufflen,
+ DMA_BIDIRECTIONAL);
+ if (data->virtual_address == 0xFFFFFFFF) {
+ printk(KERN_ERR
+ "ibmvscsi: Unable to map request_buffer for command!\n");

dma_map_single() has no error return currently, what is this trying to do?

+
+ /* Block requests until we get the SRP login back */
+ scsi_block_requests(host);
+
+ if (!scsi_add_host(hostdata->host, hostdata->dev)) {
+
+ scsi_scan_host(host);
+ return hostdata;
+ }

Er, have you actually tried this ... the block requests will block all requests including the attempt to scan. I assume the unblock is coming from the schedule_work, but you're still hanging the rest of the system waiting for this on a bootup.

+static int ibmvscsi_send_srp_event(struct srp_event_struct *evt_struct,
+ struct ibmvscsi_host_data *hostdata)
+{
[...]
+ if (ibmvscsi_send_crq(hostdata, crq_as_u64[0], crq_as_u64[1]) != 0) {
+ list_del(&evt_struct->list);
+
+ cmnd = evt_struct->cmnd;
+ printk(KERN_ERR "ibmvscsi: failed to send event struct\n");
+ unmap_cmd_data(&evt_struct->evt->srp.cmd, hostdata->dev);
+ ibmvscsi_free_event_struct(&hostdata->pool, evt_struct);
+ cmnd->result = DID_ERROR << 16;
+ evt_struct->cmnd_done(cmnd);
+ }

This is still in the queuecommand path. Are you sure you want to end the command with DID_ERROR here, which will decrement the retry count in error handling rather than returning SCSI_MQUEUE_DEVICE_BUSY?

There seem to be other places in the driver where the return status of ibmvscsi_send_crq isn't being checked, should it be?

James

James,

Thanks for the comments, I'll send a revised patch. On a couple of the issues...

Well, actually in in arch/ppc64/pci_iommu.c it can return 0xFFFFFFFF (actually, it returns NO_TCE, which is ((dma_addr_t)-1)).

Yes, this works fine, but I don't like it. The scenario is that until we get the SRP_LOGIN response back from the adapter, we don't know what the queue_limit is (can_queue). If the mid-layer starts scanning and sending SCSI cmnds, we have to hold them up until we get that response back. I've tried a number of approaches....originally I kicked off scsi_scan from the bottom half that gets the SRP_LOGIN_RSP. However:
- The bottom half is executing in a work queue. Calling scsi_scan() causes more requests to be sent and responses returned, which would like to execute in the bottom_half workqueue, except that the workqueue is busy because it is executing scsi_scan().
- I created a second workqueue just to handle scsi_scan(), but that was getting byzantine and Christoph threw up on it.

The only other approach I have considered is not doing the scsi_block_requests(), but waiting on a completion before issuing scsi_scan() and posting the completion when the SRP_LOGIN_RSP is received.

Dave B

Well, OK, put a fixme in there, since eventually there will be a standard for dma_map_single errors, and it may not be -1.

I don't quite follow this. SCSI uses a softirq in 2.6, so there are no bottom half scheduling issues for us at all.

Hanging the boot sequence like this waiting for a login isn't acceptable.

As I see it, the preferred option would be simply to trigger a scan of the device from user land using the scsi_host class, so dispense with the scsi_scan_host() altogether.

James

James,

Here is a 2.6 patch for the ibmvscsi driver incorporating your comments. I was trying to be too clever in the initialization path...there are lots of drivers that need to delay during initialization, and they don't play games with scsi_block. Code in this case lifed almost directly from the qlogic driver. The diffs from the previous ibmvscsi.c are inline, but I've regenerated and attached the patch for the whole driver to this note.

By the way, one of your other comments was whether I really wanted to end commands with DID_ERROR when I can't send them to the adapter. I think the answer is yes, since if that happens we are getting into some ugly failure mode.

Please let me know if there are any other issues you would like me to work to get this accepted. This driver is currently in a beta version of the next SuSE code, and is currently part of our test cycle testing new platforms, and it seems to be holding together pretty well.

@@ -82,6 +82,7 @@
  */
 static int max_id = 64;
 static int max_channel = 3;
+static int init_timeout = 5;
 
 MODULE_DESCRIPTION("IBM Virtual SCSI");
 MODULE_AUTHOR("Dave Boutcher");

@@ -91,6 +92,8 @@
 MODULE_PARM_DESC(max_id, "Largest ID value for each channel");
 module_param_named(max_channel, max_channel, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(max_channel, "Largest channel value");
+module_param_named(init_timeout, init_timeout, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(init_timeout, "Initialization timeout in seconds");
 
 /* ------------------------------------------------------------
  * Routines for the event pool and event structs

@@ -342,8 +345,8 @@
                        srp_cmd->data_out_format = SRP_DIRECT_BUFFER;
                else
                        srp_cmd->data_in_format = SRP_DIRECT_BUFFER;
-               data->virtual_address = sg[0].dma_address;
-               data->length = sg[0].dma_length;
+               data->virtual_address = sg_dma_address(&sg[0]);
+               data->length = sg_dma_len(&sg[0]);
                data->memory_handle = 0;
                return 1;
        }

@@ -368,10 +371,10 @@
        for (i = 0; i < sg_mapped; ++i) {
                struct memory_descriptor *descr = &indirect->list[i];
                struct scatterlist *sg_entry = &sg[i];
-               descr->virtual_address = sg_entry->dma_address;
-               descr->length = sg_entry->dma_length;
+               descr->virtual_address = sg_dma_address(sg_entry);
+               descr->length = sg_dma_len(sg_entry);
                descr->memory_handle = 0;
-               total_length += sg_entry->dma_length;
+               total_length += sg_dma_len(sg_entry);
        }
        indirect->total_length = total_length;
 

@@ -397,6 +400,10 @@
            (u64) (unsigned long)dma_map_single(dev, cmd->request_buffer,
                                                cmd->request_bufflen,
                                                DMA_BIDIRECTIONAL);
+       /* FIXME: one day this should return something other than
+        * 0xFFFFFFFF in case of error. currently arch/ppc64 returns
+        * ((dma_addr_t)-1) in some cases
+        */
        if (data->virtual_address == 0xFFFFFFFF) {
                printk(KERN_ERR
                       "ibmvscsi: Unable to map request_buffer for 

command!\n");
@@ -646,14 +653,14 @@

                break;
        case SRP_LOGIN_REJ_TYPE:        /* refused! */
                printk(KERN_INFO "ibmvscsi: SRP_LOGIN_REQ rejected\n");
-               /* Login failed.  we are still scsi_blocked */
+               /* Login failed.  */
                atomic_set(&hostdata->request_limit, -1);
                return;
        default:
                printk(KERN_ERR
                       "ibmvscsi: Invalid login response typecode 
0x%02x!\n",
                       evt_struct->evt->srp.generic.type);
-               /* Login failed.  we are still scsi_blocked */
+               /* Login failed.  */
                atomic_set(&hostdata->request_limit, -1);
                return;
        }
@@ -667,9 +674,6 @@
        hostdata->host->can_queue =
            evt_struct->evt->srp.login_rsp.request_limit_delta;

- /* Now allow commands (such the queued up scans) to go */
- scsi_unblock_requests(hostdata->host);
-

        return;

}

@@ -841,10 +845,15 @@
case 0x01: /* Initialization message */
printk(KERN_INFO "ibmvscsi: partner

initialized\n");
                        /* Send back a response */
-                       ibmvscsi_send_crq(hostdata, 0xC002000000000000, 0);
-
-                       /* Now login */
-                       send_srp_login(hostdata);
+                       if (ibmvscsi_send_crq(hostdata,

+ 0xC002000000000000, 0) == 0) {
+ /* Now login */
+ send_srp_login(hostdata);
+ } else {
+ printk(KERN_ERR
+ "ibmvscsi: Unable to send init rsp\n");
+ }
+

                        break;
                case 0x02:      /* Initialization response */
                        printk(KERN_INFO
@@ -860,7 +869,6 @@
        case 0xFF:              /* Hypervisor telling us the connection is 
closed */
                printk(KERN_INFO "ibmvscsi: Virtual adapter failed!\n");

-               scsi_block_requests(hostdata->host);
                atomic_set(&hostdata->request_limit, -1);
                purge_requests(hostdata);
                return;

@@ -908,6 +916,7 @@
* SCSI driver registration
*/
static struct scsi_host_template driver_template = {
+ .module = THIS_MODULE,
.name = "SCSI host adapter emulator for RPA/iSeries Virtual I/O", .proc_name = "ibmvscsi",
.queuecommand = ibmvscsi_queue,
@@ -927,7 +936,7 @@

 {
        struct ibmvscsi_host_data *hostdata;
        struct Scsi_Host *host;

- int rc;
+ unsigned long wait_switch = 0;

        host = scsi_host_alloc(&driver_template, sizeof(*hostdata));
        if (!host) {
@@ -952,29 +961,40 @@
                goto init_pool_failed;
        }

- /* Try to send an initialization message. Note that this is allowed
- * to fail if the other end is not active.
- */
- rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000, 0);
-

        host->max_lun = 8;
        host->max_id = max_id;
        host->max_channel = max_channel;

-       /* Block requests until we get the SRP login back */
-       scsi_block_requests(host);
+       if (scsi_add_host(hostdata->host, hostdata->dev))
+               goto add_host_failed;
+
+       /* Try to send an initialization message.  Note that this is allowed
+        * to fail if the other end is not acive.  In that case we don't
+        * want to scan
+        */
+       if (ibmvscsi_send_crq(hostdata, 0xC001000000000000, 0) == 0) {
+               /*
+                * Wait around max init_timeout secs for the adapter to finish
+                * initializing. When we are done initializing, we will have a
+                * valid request_limit.  We don't want Linux scanning before
+                * we are ready.
+                */

+ for (wait_switch = jiffies + (init_timeout * HZ);
+ time_before(jiffies,wait_switch) &&
+ atomic_read(&hostdata->request_limit) < 0;) {
+
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(5);
+ }

- if (!scsi_add_host(hostdata->host, hostdata->dev)) {
- /* If we can communicate with our server, scan
- * for devices */

-               if (rc == 0) {
+               /* if we now have a valid request_limit, initiate a scan */
+               if (atomic_read(&hostdata->request_limit) > 0)
                        scsi_scan_host(host);
-               }
-               return hostdata;
        }

+
+ return hostdata;

- printk(KERN_ERR "ibmvscsi: couldn't add host\n");
-
+ add_host_failed:

        release_event_pool(&hostdata->pool, hostdata);
       init_pool_failed:
        ibmvscsi_release_crq_queue(&hostdata->queue, hostdata);

Dave B

@@ -0,0 +1,5 @@
+obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsic.o
+
+ibmvscsic-y			+= ibmvscsi.o
+ibmvscsic-$(CONFIG_PPC_ISERIES)	+= iseries_vscsi.o 
+ibmvscsic-$(CONFIG_PPC_PSERIES)	+= rpa_vscsi.o 

@@ -725,6 +725,15 @@
 	  To compile this driver as a module, choose M here: the
 	  module will be called ips.
 
+config SCSI_IBMVSCSI
+	tristate "IBM Virtual SCSI support"
+	depends on PPC_PSERIES || PPC_ISERIES
+	help
+	  This is the IBM Virtual SCSI Client
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ibmvscsic.
+
 config SCSI_INITIO
 	tristate "Initio 9100U(W) support"
 	depends on PCI && SCSI && BROKEN

@@ -114,6 +114,7 @@
 obj-$(CONFIG_SCSI_SATA_PROMISE)	+= libata.o sata_promise.o
 obj-$(CONFIG_SCSI_SATA_SIL)	+= libata.o sata_sil.o
 obj-$(CONFIG_SCSI_SATA_VIA)	+= libata.o sata_via.o
+obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsi/
 
 obj-$(CONFIG_ARM)		+= arm/
 

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.

Thanks,

Dave B

diff -uNr --exclude=SCCS linux-2.5/drivers/scsi/ibmvscsi/Makefile ppc64-2.5new/drivers/scsi/ibmvscsi/Makefile
@@ -0,0 +1,5 @@
+obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsic.o
+
+ibmvscsic-y			+= ibmvscsi.o
+ibmvscsic-$(CONFIG_PPC_ISERIES)	+= iseries_vscsi.o 
+ibmvscsic-$(CONFIG_PPC_PSERIES)	+= rpa_vscsi.o 

@@ -752,6 +752,15 @@
 	  To compile this driver as a module, choose M here: the
 	  module will be called ips.
 
+config SCSI_IBMVSCSI
+	tristate "IBM Virtual SCSI support"
+	depends on PPC_PSERIES || PPC_ISERIES
+	help
+	  This is the IBM Virtual SCSI Client
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ibmvscsic.
+
 config SCSI_INITIO
 	tristate "Initio 9100U(W) support"
 	depends on PCI && SCSI && BROKEN

@@ -123,6 +123,7 @@
 obj-$(CONFIG_SCSI_SATA_SIL)	+= libata.o sata_sil.o
 obj-$(CONFIG_SCSI_SATA_VIA)	+= libata.o sata_via.o
 obj-$(CONFIG_SCSI_SATA_VITESSE)	+= libata.o sata_vsc.o
+obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsi/
 
 obj-$(CONFIG_ARM)		+= arm/
 

Actually, this:

+ (u64) (unsigned long)dma_map_single(dev, cmd->request_buffer,
+ cmd->request_bufflen,
+ DMA_BIDIRECTIONAL);
+ if (pci_dma_mapping_error(data->virtual_address)) {
+ printk(KERN_ERR
+ "ibmvscsi: Unable to map request_buffer for command!\n");
+ return 0;

Should be

if(dma_mapping_error())

I have no idea why there are two identical APIs for the mapping error, but since you use the DMA API, you should use its version. You can also drop the #include <linux/pci.h> as well.

This:

+ sg_mapped = dma_map_sg(dev, sg, cmd->use_sg, DMA_BIDIRECTIONAL);
+
+ if (pci_dma_mapping_error(sg_dma_address(&sg[0])))
+ return 0;

Is wrong. dma_map_sg returns zero if there's a mapping error, you should check for that.

James

Well, that would be true if arch/ppc64 had dma_mapping_error implemented. Which it not. You would need something like the following patch, which will show up when we rationalize it with the rest of ppc64 and an appropriate bk pull happens...I'll work with my ppc64 bretheren and then re-submit the ibmvscsi patch.

===== dma-mapping.h 1.5 vs edited =====

@@ -15,6 +15,11 @@
 #include <asm/scatterlist.h>
 #include <asm/bug.h>
 
+#define DMA_ERROR_CODE      (~(dma_addr_t)0x0)
+static inline int dma_mapping_error(dma_addr_t dma_addr) {
+        return (dma_addr == DMA_ERROR_CODE);
+}
+
 extern int dma_supported(struct device *dev, u64 mask);
 extern int dma_set_mask(struct device *dev, u64 dma_mask);
 extern void *dma_alloc_coherent(struct device *dev, size_t size,

Yes, my bad. I was so delighted with pci_dma_mapping_error() that I got a little carried away. Thanks.

Dave B

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

Hmmm...well, I could collapse the two together, since commands_per_lun is not limited by anything specific for this adapter. I would prefer to leave them broken out to handle users who have extreme requirements.

The SRP spec has two different buffer formats: SRP_DIRECT_BUFFER for a single buffer, and SRP_INDIRECT_BUFFER for lists of buffers, and the layout of the buffers in the request is different for those two cases.

Yes....that is one of the things that would drive you down that path. There are a couple of places in the code where request_limit is explicitly set to -1 to force all requests to be rejected.

OK, two issues. There are a bunch of SCSI LLDDs that use this same logic in abort and reset handlers to wait for adapter events to complete, so I think the logic is OK. The issue of spin_lock_irq vs spin_lock is a good one...and points out that there are a bunch of LLDDs that are broke :-) I'll resubmit without the _irq

Ya, copied from less sane operating systems. I'll fix.

I'll fix.

Oops.

This whole driver has to do with adapter sharing....and this answers the question with whom you are sharing it. Which may in fact NOT be Linux.

Agreed...I'll walk either side of the street on that one.

Hmmm...above code copied from the qlogic driver...and it looked reasonable to me, but I'll tweak it.

Because for iseries it is implemented in the pci code. While it may look wrong, it is actually correct. Check out arch/ppc64/kernel/iSeries_iommu.c and arch/ppc64/kernel/dma.c. This device has to have dev->bus == &pci_bus_type otherwise the dma_mapping_foo functions won't work correctly.

Thanks for the comments!!!!!!!!!!!!!!

Dave B

Actually, no, with the irq is correct. Wait_for_completion will sleep, and sleeping with interrupts disabled is wrong.

The reason for this is that the error handler takes the host lock around calls to the driver error handler functions. The rationale was that "simple" drivers didn't want to bother with locking, but it's obviously causing more problems than it solves.

Erm, something is very wrong in the iSeries code then. This iseries_vio_device is a struct device. As such, it should contain all the information it needs for the DMA API to act on it without performing silly pci device tricks.

This looks like it's done because the iseries should be converted to the generic device infrastructure, but in fact it's not. Since the generic API has been around for over a year and was designed to solve precisely these very problems it needs fixing rather than trying to work around it in a driver.

James

There will always be 1 (no more, no less) of these struct devices in the system, so I'll move the definition of this into iSeries_iommu and then just reference it from the driver. I think that should abstract things sufficiently.

Dave B

Sounds like a small module declaring devices such as this would be more appropriate than unrelated iommu code?

In a regular PCI system, the PCI bus probing code creates devices. For platform-specific virtual devices, ppc64 needs "create my virtual devices" initialization code, it looks like.

Jeff

Attached is a patch which fixes a problem in qla2xxx_eh_abort() where we incorrect use spin_unlock() before calling qla2x00_abort_command()
- which can of course sleep.

Please apply.

Thanks,
Andrew Vasquez

===== drivers/scsi/qla2xxx/qla_os.c 1.15 vs edited =====

@@ -1227,7 +1227,7 @@
 			sp_get(ha, sp);
 
 			spin_unlock_irqrestore(&ha->hardware_lock, flags);
-			spin_unlock(ha->host->host_lock);
+			spin_unlock_irq(ha->host->host_lock);
 
 			if (qla2x00_abort_command(ha, sp)) {
 				DEBUG2(printk("qla2xxx_eh_abort: abort_command "

Well, actually, sleeping with interrupts disabled is only a minor bug. Assuming they'll be disabled when you return from a sleep is the major bug because if you take the host lock again without disabling interrupts, you'll deadlock on a SCSI interrupt.

Regardless, this patch doesn't look right. Flags contains interrupt state. If you actually go around the for loop, you set the state unconditionally to disabled because of the

			spin_lock_irq(ha->host->host_lock);
			spin_lock_irqsave(&ha->hardware_lock, flags);

James

"Do what you need to do, and no more."

Don't engineer code because users _might_ have some outlandish requirement.

My mistake here -- I was misreading the code as testing both DIRECT_BUFFER and INDIRECT_BUFFER in both the loop and non-sg case.

ewwww ;-)

Well, qlogic is wrong too. Do you want to submit a patch fixing qlogic while you're at it? ;-)

Jeff

Fair enough - the driver isn't checking the return value of schedule_timeout() in many cases. How about the attached patch against scsi-misc-2.6.

Thanks,
Andrew Vasquez

===== drivers/scsi/qla2xxx/qla_init.c 1.11 vs edited =====

@@ -946,7 +946,7 @@
 			break;
 
 		/* Delay for a while */
-		set_current_state(TASK_INTERRUPTIBLE);
+		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule_timeout(HZ / 2);
 
 		DEBUG3(printk("scsi(%ld): fw_state=%x curr time=%lx.\n",

===== drivers/scsi/qla2xxx/qla_os.c 1.15 vs edited =====

@@ -912,7 +912,7 @@
 
 		spin_unlock_irq(ha->host->host_lock);
 
-		set_current_state(TASK_INTERRUPTIBLE);
+		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule_timeout(2*HZ);
 
 		spin_lock_irq(ha->host->host_lock);

@@ -960,7 +960,7 @@
 	    test_bit(ISP_ABORT_RETRY, &ha->dpc_flags)) &&
 		time_before(jiffies, wait_online)) {
 
-		set_current_state(TASK_INTERRUPTIBLE);
+		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule_timeout(HZ);
 	}
 	if (ha->flags.online == TRUE) 

@@ -1003,7 +1003,7 @@
 	    atomic_read(&ha->loop_state) == LOOP_DOWN) ||
 	    test_bit(CFG_ACTIVE, &ha->cfg_flags) ||
 	    atomic_read(&ha->loop_state) != LOOP_READY) {
-		set_current_state(TASK_INTERRUPTIBLE);
+		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule_timeout(HZ);
 		if (time_after_eq(jiffies, loop_timeout)) {
 			return_status = QLA_FUNCTION_FAILED;

@@ -2123,8 +2123,8 @@
 
 		qla2x00_check_fabric_devices(ha);
 
-		set_current_state(TASK_INTERRUPTIBLE);
-		schedule_timeout(5);
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		schedule_timeout(HZ/100);
 	}
 
 	pci_set_drvdata(pdev, ha);

@@ -2866,7 +2866,7 @@
 			    "Memory Allocation failed - request_ring\n");
 
 			qla2x00_mem_free(ha);
-			set_current_state(TASK_INTERRUPTIBLE);
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			schedule_timeout(HZ/10);
 
 			continue;

@@ -2880,7 +2880,7 @@
 			    "Memory Allocation failed - response_ring\n");
 
 			qla2x00_mem_free(ha);
-			set_current_state(TASK_INTERRUPTIBLE);
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			schedule_timeout(HZ/10);
 
 			continue;

@@ -2894,7 +2894,7 @@
 			    "Memory Allocation failed - init_cb\n");
 
 			qla2x00_mem_free(ha);
-			set_current_state(TASK_INTERRUPTIBLE);
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			schedule_timeout(HZ/10);
 
 			continue;

@@ -2907,7 +2907,7 @@
 			    "Memory Allocation failed - ioctl_mem\n");
 
 			qla2x00_mem_free(ha);
-			set_current_state(TASK_INTERRUPTIBLE);
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			schedule_timeout(HZ/10);
 
 			continue;

@@ -2919,7 +2919,7 @@
 			    "qla2x00_allocate_sp_pool()\n");
 
 			qla2x00_mem_free(ha);
-			set_current_state(TASK_INTERRUPTIBLE);
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			schedule_timeout(HZ/10);
 
 			continue;

@@ -2936,7 +2936,7 @@
 				    "Memory Allocation failed - sns_cmd\n");
 
 				qla2x00_mem_free(ha);
-				set_current_state(TASK_INTERRUPTIBLE);
+				set_current_state(TASK_UNINTERRUPTIBLE);
 				schedule_timeout(HZ/10);
 
 				continue;

@@ -2952,7 +2952,7 @@
 				    "Memory Allocation failed - ms_iocb\n");
 
 				qla2x00_mem_free(ha);
-				set_current_state(TASK_INTERRUPTIBLE);
+				set_current_state(TASK_UNINTERRUPTIBLE);
 				schedule_timeout(HZ/10);
 
 				continue;

@@ -2971,7 +2971,7 @@
 				    "Memory Allocation failed - ct_sns\n");
 
 				qla2x00_mem_free(ha);
-				set_current_state(TASK_INTERRUPTIBLE);
+				set_current_state(TASK_UNINTERRUPTIBLE);
 				schedule_timeout(HZ/10);
 
 				continue;

@@ -2988,7 +2988,7 @@
 			    "Memory Allocation failed - iodesc_pd\n");
 
 			qla2x00_mem_free(ha);
-			set_current_state(TASK_INTERRUPTIBLE);
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			schedule_timeout(HZ/10);
 
 			continue;

Looks good to me...

but a question: in the cases you left TASK_INTERRUPTIBLE, do you care at all about signals interrupting the wait? What are the implications of signal delivery and a shorter schedule_timeout() on these remaining cases?

Jeff