* Fix race condition between finishing a request and returning a busy status

when the channel is in use, which would cause the SCSI scheduler to stop
  sending requests.
* Indeed we need to return a status from the interrupt handler, as for PIO
  transfers there is no way of knowning whether or not the interrupt at hand
  was ours.
* Add some debug output.


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@30091 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/add-ons/kernel/bus_managers/ata/ATAChannel.cpp

@@ -222,15 +222,16 @@ status_t
 ATAChannel::ExecuteIO(scsi_ccb *ccb)
 {
 	TRACE_FUNCTION("%p\n", ccb);
-	if (mutex_trylock(&fExecutionLock) != B_OK)
+	if (mutex_trylock(&fExecutionLock) != B_OK) {
+		TRACE("channel is busy\n");
 		return B_BUSY;
-
+	}
-	MutexLocker _(&fExecutionLock, true);
 
 	fRequest->SetCCB(ccb);
 	if (ccb->cdb[0] == SCSI_OP_REQUEST_SENSE) {
+		TRACE("request sense\n");
 		fRequest->RequestSense();
-		fRequest->Finish(false);
+		fRequest->Finish(false, &fExecutionLock);
 		return B_OK;
 	}
 
@@ -240,7 +241,7 @@ ATAChannel::ExecuteIO(scsi_ccb *ccb)
 	if (ccb->target_id >= fDeviceCount) {
 		TRACE_ERROR("invalid target device\n");
 		fRequest->SetStatus(SCSI_SEL_TIMEOUT);
-		fRequest->Finish(false);
+		fRequest->Finish(false, &fExecutionLock);
 		return B_BAD_INDEX;
 	}
 
@@ -248,7 +249,7 @@ ATAChannel::ExecuteIO(scsi_ccb *ccb)
 	if (device == NULL) {
 		TRACE_ERROR("target device not present\n");
 		fRequest->SetStatus(SCSI_SEL_TIMEOUT);
-		fRequest->Finish(false);
+		fRequest->Finish(false, &fExecutionLock);
 		return B_BAD_INDEX;
 	}
 
@@ -256,7 +257,7 @@ ATAChannel::ExecuteIO(scsi_ccb *ccb)
 		: ATA_STANDARD_TIMEOUT);
 
 	status_t result = device->ExecuteIO(fRequest);
-	fRequest->Finish(false);
+	fRequest->Finish(false, &fExecutionLock);
 	return result;
 }
 
@@ -554,7 +555,8 @@ ATAChannel::FinishRequest(ATARequest *request, uint32 flags, uint8 errorMask)
 		return B_OK;
 
 	request->SetStatus(SCSI_SEQUENCE_FAIL);
-	TRACE_ERROR("command failed, error bit is set\n");
+	TRACE_ERROR("command failed, error bit is set: 0x%02x\n",
+		taskFile->read.error);
 	uint8 error = taskFile->read.error & errorMask;
 	if (error & ATA_ERROR_INTERFACE_CRC) {
 		TRACE_ERROR("interface crc error\n");
@@ -702,21 +704,22 @@ ATAChannel::WritePIO(uint8 *buffer, size_t length)
 }
 
 
-void
+status_t
 ATAChannel::Interrupt(uint8 status)
 {
 	SpinLocker locker(fInterruptLock);
 	if (!fExpectsInterrupt) {
-		TRACE_ERROR("interrupt when not expecting transfer\n");
+		TRACE("interrupt when not expecting transfer\n");
-		return;
+		return B_UNHANDLED_INTERRUPT;
 	}
 
 	if ((status & ATA_STATUS_BUSY) != 0) {
-		TRACE_ERROR(("interrupt while device is busy\n"));
+		TRACE(("interrupt while device is busy\n"));
-		return;
+		return B_UNHANDLED_INTERRUPT;
 	}
 
 	fInterruptCondition.NotifyAll();
+	return B_HANDLED_INTERRUPT;
 }
 
 

src/add-ons/kernel/bus_managers/ata/ATADevice.cpp

@@ -23,9 +23,6 @@ ATADevice::ATADevice(ATAChannel *channel, uint8 index)
 {
 	memset(&fInfoBlock, 0, sizeof(fInfoBlock));
 	memset(&fTaskFile, 0, sizeof(fTaskFile));
-
-	snprintf(fDebugContext, sizeof(fDebugContext), "%s %lu-%u",
-		IsATAPI() ? "pi " : "", channel->ChannelID(), index);
 }
 
 
@@ -464,6 +461,9 @@ ATADevice::Configure()
 status_t
 ATADevice::Identify()
 {
+	snprintf(fDebugContext, sizeof(fDebugContext), "%s %lu-%u",
+		IsATAPI() ? "pi " : "", fChannel->ChannelID(), fIndex);
+
 	ATARequest request;
 	request.SetDevice(this);
 	request.SetTimeout(20 * 1000 * 1000);
@@ -564,8 +564,10 @@ ATADevice::ExecuteReadWrite(ATARequest *request, uint64 address,
 			}
 		}
 	} else {
-		if (fChannel->ExecutePIOTransfer(request) != B_OK)
+		if (fChannel->ExecutePIOTransfer(request) != B_OK) {
+			TRACE_ERROR("executing pio transfer failed\n");
 			request->SetStatus(SCSI_SEQUENCE_FAIL);
+		}
 	}
 
 	return fChannel->FinishRequest(request, ATA_WAIT_FINISH

src/add-ons/kernel/bus_managers/ata/ATAModule.cpp

@@ -206,12 +206,7 @@ status_t
 ata_interrupt_handler(void *cookie, uint8 status)
 {
 	ATAChannel *channel = (ATAChannel *)cookie;
-	channel->Interrupt(status);
+	return channel->Interrupt(status);
-
-	// the controller driver already checks if its interrupt status indicates
-	// that the interrupt was for this device, so we are only here if that's
-	// the case.
-	return B_HANDLED_INTERRUPT;
 }
 
 

src/add-ons/kernel/bus_managers/ata/ATAPrivate.h

@@ -107,7 +107,7 @@ public:
 		status_t					ReadPIO(uint8 *buffer, size_t length);
 		status_t					WritePIO(uint8 *buffer, size_t length);
 
-		void						Interrupt(uint8 status);
+		status_t					Interrupt(uint8 status);
 
 private:
 		status_t					_ReadRegs(ata_task_file *taskFile,
@@ -266,7 +266,8 @@ public:
 		void						SetBlocksLeft(uint32 blocksLeft);
 		uint32 *					BlocksLeft() { return &fBlocksLeft; };
 
-		status_t					Finish(bool resubmit);
+		status_t					Finish(bool resubmit,
+										mutex *mutexToUnlock);
 
 		// SCSI stuff
 		void						SetCCB(scsi_ccb *ccb);

src/add-ons/kernel/bus_managers/ata/ATARequest.cpp

@@ -76,7 +76,7 @@ ATARequest::SetBlocksLeft(uint32 blocksLeft)
 
 
 status_t
-ATARequest::Finish(bool resubmit)
+ATARequest::Finish(bool resubmit, mutex *mutexToUnlock)
 {
 	// when the request completed and has set sense
     // data, report this to the scsi stack by setting
@@ -102,6 +102,8 @@ ATARequest::Finish(bool resubmit)
 	} else
 		fCCB->subsys_status = fStatus;
 
+	mutex_unlock(mutexToUnlock);
+
 	if (resubmit)
 		gSCSIModule->resubmit(fCCB);
 	else