Re: [patch] 3Ware 64 bit locking issues

Re: [patch] 3Ware 64 bit locking issues

[Jes Sorensen]

>>>>> "Adam" == Adam Radford <aradford@3WARE.com> writes:

Adam> Thanks for flags type fix and redundant pushf/popf fix.
Adam> Regarding your question about the error handling routines, the
Adam> 3ware driver uses the new style scsi error handling, so looking
Adam> through scsi_error.c, all calls to
Adam> hostt->eh_abort_handler() and hostt->eh_host_reset_handler() are
Adam> wrapped with a spin_lock_irqsave(&io_request_lock, flags) and
Adam> spin_unlock_irqrestore(&io_request_lock, flags) so they should
Adam> be okay.

Hmm ok. However, since Jens is working on killing the io_request_lock
so something will need to get done when that happens.

Jens, what is the strategy for that?

Cheers
Jes

Re: [patch] 3Ware 64 bit locking issues

[Jens Axboe]

On Wed, Aug 22 2001, Jes Sorensen wrote:
> >>>>> "Adam" == Adam Radford <aradford@3WARE.com> writes:
> 
> Adam> Thanks for flags type fix and redundant pushf/popf fix.
> Adam> Regarding your question about the error handling routines, the
> Adam> 3ware driver uses the new style scsi error handling, so looking
> Adam> through scsi_error.c, all calls to
> Adam> hostt->eh_abort_handler() and hostt->eh_host_reset_handler() are
> Adam> wrapped with a spin_lock_irqsave(&io_request_lock, flags) and
> Adam> spin_unlock_irqrestore(&io_request_lock, flags) so they should
> Adam> be okay.
> 
> Hmm ok. However, since Jens is working on killing the io_request_lock
> so something will need to get done when that happens.
> 
> Jens, what is the strategy for that?

Lots of the lower level hooks are done with io_request_lock required,
the only one I've killed so far is ->detect() and that was mainly
because it didn't fit with the new scheme (per-host locking, host not
inited at this time). IMO it was a mistake to ever grab io_request_lock
(or any other lock, for that matter) before calling into the lower
levels -- it's not very clean and it makes progressing to a better
locking structure harder.

Basically we want to move the locking down a notch, so the mid layer is
not responsible for providing reentrance protection for the low level
drivers. It will be painfull...

-- 
Jens Axboe

[patch] 3Ware 64 bit locking issues

[jes]

Hi

The 3Ware driver was using 32 bit data types for cpu flags arguments
to spin_lock_irqsave which isn't safe. Here is a patch, which also
gets rid of the redundant flag saving in one instance in the interrupt
handler where the flags had just been saved anyway.

I also noticed that the driver does spin_lock() in the error handling
functions, without disabling interrupts on the local cpu. Is this
safe? Ie. are the error handling functions guaranteed to be called
from interrupt context or with interrupts disabled? Note that I didn't
make any changes to this bit.

Jes

--- drivers/scsi/3w-xxxx.c~	Wed Jun 27 20:10:55 2001
+++ drivers/scsi/3w-xxxx.c	Thu Aug 16 00:17:00 2001
@@ -1202,15 +1202,14 @@
 	int do_attention_interrupt=0;
 	int do_host_interrupt=0;
 	int do_command_interrupt=0;
-	int flags = 0;
-	int flags2 = 0;
+	unsigned long flags = 0;
 	TW_Command *command_packet;
 	if (test_and_set_bit(TW_IN_INTR, &tw_dev->flags))
 		return;
 	spin_lock_irqsave(&io_request_lock, flags);
 
 	if (tw_dev->tw_pci_dev->irq == irq) {
-		spin_lock_irqsave(&tw_dev->tw_lock, flags2);
+		spin_lock(&tw_dev->tw_lock);
 		dprintk(KERN_NOTICE "3w-xxxx: tw_interrupt()\n");
 
 		/* Read the registers */
@@ -1349,7 +1348,7 @@
 				}
 			}
 		}
-		spin_unlock_irqrestore(&tw_dev->tw_lock, flags2);
+		spin_unlock(&tw_dev->tw_lock);
 	}
 	spin_unlock_irqrestore(&io_request_lock, flags);
 	clear_bit(TW_IN_INTR, &tw_dev->flags);
@@ -1918,7 +1917,7 @@
 	unsigned char *command = SCpnt->cmnd;
 	int request_id = 0;
 	int error = 0;
-	int flags = 0;
+	unsigned long flags = 0;
 	TW_Device_Extension *tw_dev = (TW_Device_Extension *)SCpnt->host->hostdata;
 
 	if (tw_dev == NULL) {