Hi, Wenbo Here "queue handling" what I meant is s/w SQ/CQ handling in EDKII NvmExpressDxe driver, we don't check command ID. We just check PT to know if there is a new completion queue entry. So with current logic, if there is a timeout happen and the upper layer send another command again, the current command would wrongly think it's done if the completion event for last command just arrives. That's what I meant "mess up", it's only from s/w view rather h/w view.
For your case " if the driver moves head (head++), it corrupts the completion queue, the NVMe controller generates error", looks like we have to ensure the header is always less or equal than the tail for the completion queue. So I think we have to add two steps: 1. check PT to see if there is a new CQ event. if no, then don't advance to next CQ entry. 2. using abort command to stop the execution of specified command. Thanks Feng -----Original Message----- From: Wenbo Wang [mailto:[email protected]] Sent: Tuesday, September 08, 2015 10:31 To: Tian, Feng; [email protected] Subject: RE: [edk2] [NvmExpressDxe] Why update cq head even if command timeout Because each command has an unique command ID, queue handling shall not miss up even if the completion is returned after timeout. -----Original Message----- From: Wenbo Wang [mailto:[email protected]] Sent: Tuesday, September 8, 2015 10:23 AM To: 'Tian, Feng'; '[email protected]' Subject: RE: [edk2] [NvmExpressDxe] Why update cq head even if command timeout Thanks Feng. In our test, when the command timeout, the completion queue is empty (head == tail), in this case if the driver moves head (head++), it corrupts the completion queue, the NVMe controller generates error. I think the HW behavior is correct because the driver shall not move head if there is no completion there. line 568 to line 592 is not executed when no timeout happens. Yes, I don't see driver sending abort NVMe command when timeout happens. Thanks, -Wenbo -----Original Message----- From: Tian, Feng [mailto:[email protected]] Sent: Tuesday, September 8, 2015 9:47 AM To: Wenbo Wang; [email protected] Cc: Tian, Feng Subject: RE: [edk2] [NvmExpressDxe] Why update cq head even if command timeout Hi, Wenbo Why we advanced one CQ entry in line 592 whatever the execution result of SQ is because there are two cases: 1. If timeout happens and no new completion queue entry is added, the CQH get updated to point to next entry. Nvme controller would know a new entry of completion queue is available for use. 2. The SQ is completed between line 568 and line 592. If it's such case, timeout is return but a completion queue entry is filled. So I guess the problem is the SQ entry doesn't get aborted at timeout case, right? If there is another request, the queue handling is missed up. Thanks Feng -----Original Message----- From: edk2-devel [mailto:[email protected]] On Behalf Of Wenbo Wang Sent: Monday, September 07, 2015 17:26 To: [email protected] Cc: Tian, Feng Subject: [edk2] [NvmExpressDxe] Why update cq head even if command timeout Hi, The following code is from NvmExpressPassthru.c, at line 592, the cq head is updated even if the command timeout. Is there any reason for this? If completion queue is empty, this operation may corrupt the queue. Thanks, -Wenbo ======== Code Copied from the latest NvmExpressPassthru.c ============= // Command has been submitted 553 Status = gBS->SetTimer(TimerEvent, TimerRelative, Packet->CommandTimeout); 554 555 if (EFI_ERROR(Status)) { 556 goto EXIT; 557 } 558 559 // 560 // Wait for completion queue to get filled in. 561 // 562 Status = EFI_TIMEOUT; 563 while (EFI_ERROR (gBS->CheckEvent (TimerEvent))) { 564 if (Cq->Pt != Private->Pt[QueueType]) { 565 Status = EFI_SUCCESS; 566 break; 567 } 568 } 569 570 // 571 // Check the NVMe cmd execution result 572 // 573 if (Status != EFI_TIMEOUT) { 574 if ((Cq->Sct == 0) && (Cq->Sc == 0)) { 575 Status = EFI_SUCCESS; 576 } else { 577 Status = EFI_DEVICE_ERROR; 578 // 579 // Copy the Respose Queue entry for this command to the callers response buffer 580 // 581 CopyMem(Packet->NvmeCompletion, Cq, sizeof(EFI_NVM_EXPRESS_COMPLETION)); 582 583 // 584 // Dump every completion entry status for debugging. 585 // 586 DEBUG_CODE_BEGIN(); 587 NvmeDumpStatus(Cq); 588 DEBUG_CODE_END(); 589 } 590 } 591 592 if ((Private->CqHdbl[QueueType].Cqh ^= 1) == 0) { <- cq head is updated even if command timeout 593 Private->Pt[QueueType] ^= 1; 594 } 595 596 Data = ReadUnaligned32 ((UINT32*)&Private->CqHdbl[QueueType]); 597 PciIo->Mem.Write ( 598 PciIo, 599 EfiPciIoWidthUint32, 600 NVME_BAR, 601 NVME_CQHDBL_OFFSET(QueueType, Private->Cap.Dstrd), 602 1, 603 &Data 604 ); _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
