From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from userp2130.oracle.com (userp2130.oracle.com [156.151.31.86]) by mx.groups.io with SMTP id smtpd.web11.9476.1587052991514133488 for ; Thu, 16 Apr 2020 09:03:11 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=EGaap+q8; spf=pass (domain: oracle.com, ip: 156.151.31.86, mailfrom: nikita.leshchenko@oracle.com) Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 03GFvbfN132269; Thu, 16 Apr 2020 16:03:08 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=content-type : mime-version : subject : from : in-reply-to : date : cc : content-transfer-encoding : message-id : references : to; s=corp-2020-01-29; bh=qiHFhq4mksvmtZTOy43rC/QQwQTqcx6yNks8x6Wxl48=; b=EGaap+q8pDFbVM1E13SW7RpxFATyjUnM9aBWEbApdDel6CD1JRorHI0YS3GkFlOkAi/H pqshZ0uSG5oN1LXELibfb3960Pl8m8ovrNU3mNmefknZyDfIVf9oN9LEyc1nQ9mgF3LO PByKyG5MSaLJYuxS1oEvECm6rf8cECuVLhd99SNqU1MMIolulJ1o1V7PmagWl1yJVtUI X9wHlJxvJ0erJsFwJzpGybTKKGxsCZacqYanffzbxYQexWrGRrrQuissgN4Li6CPfJB2 dGWQi1TPL3HPpuh0TswSgNZNVovTR0Jf4LblqN+M/0T15zm9VK8XfcPWxNTFHT3JpY7K qw== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by userp2130.oracle.com with ESMTP id 30e0aa7vvk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 16 Apr 2020 16:03:07 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 03GFvsJh151715; Thu, 16 Apr 2020 16:01:06 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by userp3030.oracle.com with ESMTP id 30dyp03e8y-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 16 Apr 2020 16:01:06 +0000 Received: from abhmp0007.oracle.com (abhmp0007.oracle.com [141.146.116.13]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 03GG14Xo002007; Thu, 16 Apr 2020 16:01:04 GMT Received: from [10.30.3.6] (/213.57.127.2) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Thu, 16 Apr 2020 09:01:04 -0700 Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: Re: [edk2-devel] [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware From: "Nikita Leshenko" In-Reply-To: <034ffcb9-e11b-4b31-8d62-2eba4eaef08f@redhat.com> Date: Thu, 16 Apr 2020 19:00:58 +0300 Cc: devel@edk2.groups.io, liran.alon@oracle.com, aaron.young@oracle.com, Jordan Justen , Ard Biesheuvel Message-Id: References: <20200414173813.7715-1-nikita.leshchenko@oracle.com> <20200414173813.7715-11-nikita.leshchenko@oracle.com> <034ffcb9-e11b-4b31-8d62-2eba4eaef08f@redhat.com> To: Laszlo Ersek X-Mailer: Apple Mail (2.3445.9.1) X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9593 signatures=668686 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 suspectscore=0 malwarescore=0 phishscore=0 spamscore=0 adultscore=0 mlxscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2004160113 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9593 signatures=668686 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 clxscore=1015 impostorscore=0 mlxlogscore=999 mlxscore=0 lowpriorityscore=0 suspectscore=0 adultscore=0 spamscore=0 malwarescore=0 phishscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2004160113 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable > On 16 Apr 2020, at 12:53, Laszlo Ersek wrote: >=20 > On 04/14/20 19:38, Nikita Leshenko wrote: >> Reset and send the IO controller initialization request. The reply is >> read back to complete the doorbell function but it isn't useful to us >> because it doesn't contain relevant data or status codes. >>=20 >> See "LSI53C1030 PCI-X to Dual Channel Ultra320 SCSI Multifunction >> Controller" technical manual for more information. >>=20 >> Ref: = https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id= =3D2390__;!!GqivPVa7Brio!Ph-KBaVDeTP4F0oR9YHybLz2_YpCTUBsXM5rymNRMgRKVeUaR= sWXxogKvM9T1fsEfQg7pQ$=20 >> Signed-off-by: Nikita Leshenko >> --- >> .../Include/IndustryStandard/FusionMptScsi.h | 123 +++++++++++++ >> OvmfPkg/MptScsiDxe/MptScsi.c | 173 = +++++++++++++++++- >> 2 files changed, 295 insertions(+), 1 deletion(-) >>=20 >> diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h = b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h >> index df9bdc2f0348..d00a9e6db0bf 100644 >> --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h >> +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h >> @@ -20,4 +20,127 @@ >> #define LSI_SAS1068_PCI_DEVICE_ID 0x0054 >> #define LSI_SAS1068E_PCI_DEVICE_ID 0x0058 >>=20 >> +#define MPT_REG_DOORBELL 0x00 >> +#define MPT_REG_WRITE_SEQ 0x04 >> +#define MPT_REG_HOST_DIAG 0x08 >> +#define MPT_REG_TEST 0x0c >> +#define MPT_REG_DIAG_DATA 0x10 >> +#define MPT_REG_DIAG_ADDR 0x14 >> +#define MPT_REG_ISTATUS 0x30 >> +#define MPT_REG_IMASK 0x34 >> +#define MPT_REG_REQ_Q 0x40 >> +#define MPT_REG_REP_Q 0x44 >> + >> +#define MPT_DOORBELL_RESET 0x40 >> +#define MPT_DOORBELL_HANDSHAKE 0x42 >=20 > (1) I suggest (but don't insist) that we adjust the whitespace after > MPT_DOORBELL_RESET so that 0x40 line up with 0x42 below it. Just an = idea. Good idea, will do in v5 >=20 >> + >> +#define MPT_IMASK_DOORBELL 0x01 >> +#define MPT_IMASK_REPLY 0x08 >> + >> +#define MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST 0x00 >> +#define MPT_MESSAGE_HDR_FUNCTION_IOC_INIT 0x02 >> + >> +#define MPT_SG_ENTRY_TYPE_SIMPLE 0x01 >> + >> +#define MPT_IOC_WHOINIT_ROM_BIOS 0x02 >> + >> +// >> +// Device structures >> +// >> + >> +typedef union { >> +#pragma pack (1) >> + struct { >> + UINT8 WhoInit; >> + UINT8 Reserved1; >> + UINT8 ChainOffset; >> + UINT8 Function; >> + UINT8 Flags; >> + UINT8 MaxDevices; >> + UINT8 MaxBuses; >> + UINT8 MessageFlags; >> + UINT32 MessageContext; >> + UINT16 ReplyFrameSize; >> + UINT16 Reserved2; >> + UINT32 HostMfaHighAddr; >> + UINT32 SenseBufferHighAddr; >> + } Data; >> +#pragma pack () >> + UINT64 Uint64; // 8 byte alignment required by HW >> +} MPT_IO_CONTROLLER_INIT_REQUEST; >> + >> +#pragma pack (1) >> +typedef struct { >> + UINT8 WhoInit; >> + UINT8 Reserved1; >> + UINT8 MessageLength; >> + UINT8 Function; >> + UINT8 Flags; >> + UINT8 MaxDevices; >> + UINT8 MaxBuses; >> + UINT8 MessageFlags; >> + UINT32 MessageContext; >> + UINT16 Reserved2; >> + UINT16 IOCStatus; >> + UINT32 IOCLogInfo; >> +} MPT_IO_CONTROLLER_INIT_REPLY; >> + >> +typedef struct { >> + UINT8 TargetID; >> + UINT8 Bus; >> + UINT8 ChainOffset; >> + UINT8 Function; >> + UINT8 CDBLength; >> + UINT8 SenseBufferLength; >> + UINT8 Reserved; >> + UINT8 MessageFlags; >> + UINT32 MessageContext; >> + UINT8 LUN[8]; >> + UINT32 Control; >> + UINT8 CDB[16]; >> + UINT32 DataLength; >> + UINT32 SenseBufferLowAddress; >> +} MPT_SCSI_IO_REQUEST; >> + >> +typedef struct { >> + UINT32 Length: 24; >> + UINT32 EndOfList: 1; >> + UINT32 Is64BitAddress: 1; >> + // >> + // True when the buffer contains data to be transfered. Otherwise = it's the >> + // destination buffer >> + // >> + UINT32 BufferContainsData: 1; >> + UINT32 LocalAddress: 1; >> + UINT32 ElementType: 2; >> + UINT32 EndOfBuffer: 1; >> + UINT32 LastElement: 1; >> + UINT64 DataBufferAddress; >> +} MPT_SG_ENTRY_SIMPLE; >> +#pragma pack () >> + >> +typedef union { >> +#pragma pack (1) >> + struct { >> + UINT8 TargetID; >> + UINT8 Bus; >> + UINT8 MessageLength; >> + UINT8 Function; >> + UINT8 CDBLength; >> + UINT8 SenseBufferLength; >> + UINT8 Reserved; >> + UINT8 MessageFlags; >> + UINT32 MessageContext; >> + UINT8 SCSIStatus; >> + UINT8 SCSIState; >> + UINT16 IOCStatus; >> + UINT32 IOCLogInfo; >> + UINT32 TransferCount; >> + UINT32 SenseCount; >> + UINT32 ResponseInfo; >> + } Data; >> +#pragma pack () >> + UINT64 Uint64; // 8 byte alignment required by HW >> +} MPT_SCSI_IO_ERROR_REPLY; >> + >> #endif // __FUSION_MPT_SCSI_H__ >=20 > (2) If you really want to sink the pragmas into the unions, so that = they > only surround the embedded structs, I'm OK with that. If you feel > flexible about them, I'd suggest wrapping the entire sequence of > typedefs into a single #pragma pack (1) / pack () pair, which is = easier > to read and feels more idiomatic in edk2. But, again, if you feel it's > important to express the packing goals with this specific syntax, I'm = OK > that way too. I don't have a strong opinion about it, I prefer to do what feels more = idiomatic in EDK2 (although I don't always know what is idiomatic in EDK2 so feel = free to point out such things :) ). My intent is to have the structs packed but still aligned to 8 bytes = (for some reason the hardware aligns the descriptor address to 8). I couldn't find = any code in EDK2 that does so I came up with my own pattern. However, if I understand your suggestion correctly, if I wrap the entire typedef in #pragma pack (1) / pack (), I would lose the = alignment. That's why I wrapped only the inner structs. Are are you suggesting that I break out the inner structs into separate typedefs, wrap all of = them in #pragma pack (1) / pack () and then create the wrapping aligning unions? >=20 >> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c = b/OvmfPkg/MptScsiDxe/MptScsi.c >> index 4bfd03d2acb0..9c3bdc430e1a 100644 >> --- a/OvmfPkg/MptScsiDxe/MptScsi.c >> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c >> @@ -42,6 +42,167 @@ typedef struct { >> #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \ >> CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE) >>=20 >> +// >> +// Hardware functions >> +// >> + >> +STATIC >> +EFI_STATUS >> +Out32 ( >> + IN MPT_SCSI_DEV *Dev, >> + IN UINT32 Addr, >> + IN UINT32 Data >> + ) >> +{ >> + return Dev->PciIo->Io.Write ( >> + Dev->PciIo, >> + EfiPciIoWidthUint32, >> + PCI_BAR_IDX0, >> + Addr, >> + 1, >> + &Data >> + ); >> +} >> + >> +STATIC >> +EFI_STATUS >> +In32 ( >> + IN MPT_SCSI_DEV *Dev, >> + IN UINT32 Addr, >> + OUT UINT32 *Data >> + ) >> +{ >> + return Dev->PciIo->Io.Read ( >> + Dev->PciIo, >> + EfiPciIoWidthUint32, >> + PCI_BAR_IDX0, >> + Addr, >> + 1, >> + Data >> + ); >> +} >> + >> +STATIC >> +EFI_STATUS >> +MptDoorbell ( >> + IN MPT_SCSI_DEV *Dev, >> + IN UINT8 DoorbellFunc, >> + IN UINT8 DoorbellArg >> + ) >> +{ >> + return Out32 ( >> + Dev, >> + MPT_REG_DOORBELL, >> + (((UINT32)DoorbellFunc) << 24) | (DoorbellArg << 16) >> + ); >> +} >> + >> +STATIC >> +EFI_STATUS >> +MptScsiReset ( >> + IN MPT_SCSI_DEV *Dev >> + ) >> +{ >> + EFI_STATUS Status; >> + >> + // >> + // Reset hardware >> + // >> + Status =3D MptDoorbell (Dev, MPT_DOORBELL_RESET, 0); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + // >> + // Mask interrupts >> + // >> + Status =3D Out32 (Dev, MPT_REG_IMASK, MPT_IMASK_DOORBELL | = MPT_IMASK_REPLY); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + // >> + // Clear interrupt status >> + // >> + Status =3D Out32 (Dev, MPT_REG_ISTATUS, 0); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + return EFI_SUCCESS; >> +} >> + >> +STATIC >> +EFI_STATUS >> +MptScsiInit ( >> + IN MPT_SCSI_DEV *Dev >> + ) >> +{ >> + EFI_STATUS Status; >> + MPT_IO_CONTROLLER_INIT_REQUEST Req; >> + MPT_IO_CONTROLLER_INIT_REPLY Reply; >> + UINT8 *ReplyBytes; >> + UINT32 ReplyWord; >> + >> + Status =3D MptScsiReset (Dev); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + ZeroMem (&Req, sizeof (Req)); >> + ZeroMem (&Reply, sizeof (Reply)); >> + Req.Data.WhoInit =3D MPT_IOC_WHOINIT_ROM_BIOS; >> + Req.Data.Function =3D MPT_MESSAGE_HDR_FUNCTION_IOC_INIT; >> + Req.Data.MaxDevices =3D 1; >> + Req.Data.MaxBuses =3D 1; >> + Req.Data.ReplyFrameSize =3D sizeof (MPT_SCSI_IO_ERROR_REPLY); >=20 > (3) The local variable that you're going to read the reply into has = type > MPT_IO_CONTROLLER_INIT_REPLY. But the number of bytes you announce for > reading back matches MPT_SCSI_IO_ERROR_REPLY. >=20 > I haven't checked the technical manual that the commit message > references, so I don't know which reply format is the one that's = needed > here -- but whichever is needed, the number of bytes that we read back > should actually match the structure that we populate. >=20 > (General remark: for this reason, I avoid "sizeof (type)" on = principle, > whenever I can, and use "sizeof object" instead. With the latter, it's > more difficult to get "out of sync" if I change the type of the object > later on.) Good idea, I will change it to `sizeof Dev->Dma->IoReply.Data` >=20 > Hmmm, wait a second!... Is it possible that "ReplyFrameSize" is for > configuring *future* replies, and the = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT > request code guarantees that the device will produce an > MPT_IO_CONTROLLER_INIT_REPLY object in response, regardless of > "ReplyFrameSize"? >=20 > In that case, please ignore my comment (3). Exactly, it's for future replies >=20 >=20 > (4) We don't align MPT_IO_CONTROLLER_INIT_REPLY to an 8-byte boundary, > unlike MPT_IO_CONTROLLER_INIT_REQUEST. >=20 > Can you please confirm that it's because we read > MPT_IO_CONTROLLER_INIT_REPLY gradually (dword-wise), via "ReplyWord" = -- > which is automatically naturally aligned? (I.e., I'm not suggesting a > code change, just asking for a confirmation.) Yes. Now that you mention it, I see that it's enough to align MPT_IO_CONTROLLER_INIT_REQUEST to 4 bytes since it's read in 32-bit chunks. I will add a comment to the code. >=20 >=20 >> + >> + // >> + // Send controller init through doorbell >> + // >> + Status =3D MptDoorbell ( >> + Dev, >> + MPT_DOORBELL_HANDSHAKE, >> + sizeof (Req) / sizeof (UINT32) >=20 > (5) Please use a STATIC_ASSERT here, to express that this division > produces a 0 remainder. Please see the similar STATIC_ASSERT in the > PvScsi driver. >=20 >=20 > (6) Furthermore, I'd suggest another STATIC_ASSERT to express that the > quotient fits in a UINT8, and then maybe explicitly casting the = quotient > to UINT8. >=20 > I'm asking for the explicit cast only because I'm concerned that the = VS > compiler will whine about "possible loss of precision". (I realize the > quotient is a constant that's known at compile time -- but I'm afraid = VS > might still whine about the implicit UINTN->UINT8 conversion, for the > "DoorbellArg" parameter.) >=20 >=20 > (7) Note that the union pattern you use for alignment is *slightly* > different from the one used in PvScsi. And this difference could hide = a > trap -- an obscure trap, admittedly, but I'd like to be clear about = it. >=20 > In PvScsi we have a local variable called "AlignedCmd", which is of = the > union type. The "AlignedCmd.Cmd" union member, which is itself a = packed > structure, is the actual datum that we serialize and send to the = device. >=20 > Here however, we seem to send the full union to the device. >=20 > Now, per ISO C99 "6.7.2.1 Structure and union specifiers", paragraph = 15, > "[t]here may be unnamed padding at the end of a structure or union". = And > see my comment (2) above -- you specifically do *not* pack the unions, > only the internals of their "Data" members. (The gcc documentation > confirms that #pragma pack affects unions too, so packing a union = *can* > make a difference.) >=20 > Thus, when you send the whole MPT_IO_CONTROLLER_INIT_REQUEST union to > the device, and not just its "Data" member (which is itself packed), = you > could be writing from unnamed padding at the end of the union. >=20 > Therefore I would: >=20 > (7a) either send "Req.Data" (rather than "Req") to the device in this > function, >=20 > (7b) or else address my remark (2), and pack the unions too. >=20 >=20 > (8) I believe same observation as (7) holds for the >=20 > Req.Data.ReplyFrameSize =3D sizeof (MPT_SCSI_IO_ERROR_REPLY); >=20 > assignment. Even if my comment (3) falls away, and "ReplyFrameSize" > configures the size of *future* responses, those responses should = likely > not include any (potential) unnamed padding at the end of the > MPT_SCSI_IO_ERROR_REPLY union. I wasn't aware of this padding, so I will update the code to use = suggestion (7a). We can't do (7b) since packing the union would discard the alignment. >=20 >=20 >> + ); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + Status =3D Dev->PciIo->Io.Write ( >> + Dev->PciIo, >> + EfiPciIoWidthFifoUint32, >> + 0, >=20 > (9) Please use PCI_BAR_IDX0 here too. >=20 >> + MPT_REG_DOORBELL, >> + sizeof (Req) / sizeof (UINT32), >> + &Req >> + ); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + // >> + // Read reply through doorbell >> + // Each 32bit (Dword) read produces 16bit (Word) of data >> + // >=20 > (10) Please use another STATIC_ASSERT here for expressing that the > response structure size is an integer multiple of sizeof (UINT16). >=20 > (11) Please repeat the statement from the commit message here -- as a > code comment -- that the reply is only read to complete the doorbell > function of the device, and that we intentionally ignore the contents. >=20 > (BTW, if we do not parse the response even at the end of this series, > then saving the response into the "Reply" variable looks useless -- I > guess we could remove the "Reply" variable altogether, in that case. = But > I'll have to see the rest of the patches.) We must read the reply fully to initialise the device (the device would = reset the interrupt status only after the host has fetched the entire reply). We = could discard the reply, but I thought it would be nicer to keep it on the stack = temporary in case someone would like to use it in later changes or in debugging = (since the reply is very small and should be hot in the cache, I don't this has any performance impact). >=20 >> + ReplyBytes =3D (UINT8 *)&Reply; >> + while (ReplyBytes !=3D (UINT8 *)(&Reply + 1)) { >> + Status =3D In32 (Dev, MPT_REG_DOORBELL, &ReplyWord); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + CopyMem (ReplyBytes, &ReplyWord, sizeof (UINT16)); >> + ReplyBytes +=3D sizeof (UINT16); >> + } >> + >> + // >> + // Clear interrupts generated by doorbell reply >> + // >> + Status =3D Out32 (Dev, MPT_REG_ISTATUS, 0); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + return EFI_SUCCESS; >> +} >=20 > (12) So in general I would have suggested that, in case the > initialization fails and we take an early "return" branch in this > function, we should first jump to an error handling label, and reset = the > device. (Because, when we return an error code from the function, we > shouldn't leave the device in half-initialized state.) >=20 > *But*, it seems like the reset operation *itself* occurs through the > doorbell register and the MPT_REG_ISTATUS register -- and if any step = in > MptScsiInit() fails, then it's exactly those registers that are > (apparently) in unknown / unreliable state. In other words, if > MptScsiInit() fails, then we *can't* (expect to) reset the device > (successfully). >=20 > Can you please confirm my understanding? (Again, just a question, not = a > code change request.) Your understanding it correct. In VirtualBox it looks like you can't = write a reset command to the doorbell while a handshake is in progress. In any = case, wonder what could realistically cause the initialization to fail. On EDK2 side, = we're just talking about a bunch of "in/out" opcodes (the API for which could fail, = but I would assume that if it fails then further "in/out"s will fail as well). On = the hypervisor side, the QEMU and VirtualBox don't even have a failure path for the = init call. So failure seems very theoretical here and I couldn't find anything = realistically helpful to do if the initialization fails in the middle. >=20 >=20 >> + >> // >> // Ext SCSI Pass Thru >> // >> @@ -333,6 +494,11 @@ MptScsiControllerStart ( >> goto CloseProtocol; >> } >>=20 >> + Status =3D MptScsiInit (Dev); >> + if (EFI_ERROR (Status)) { >> + goto RestorePciAttributes; >> + } >> + >> // >> // Host adapter channel, doesn't exist >> // >> @@ -357,11 +523,14 @@ MptScsiControllerStart ( >> &Dev->PassThru >> ); >> if (EFI_ERROR (Status)) { >> - goto RestoreAttributes; >> + goto UninitDev; >> } >>=20 >> return EFI_SUCCESS; >>=20 >> +UninitDev: >> + MptScsiReset (Dev); >> + >> RestoreAttributes: >> Dev->PciIo->Attributes ( >> Dev->PciIo, >> @@ -421,6 +590,8 @@ MptScsiControllerStop ( >> return Status; >> } >>=20 >> + MptScsiReset (Dev); >> + >> Dev->PciIo->Attributes ( >> Dev->PciIo, >> EfiPciIoAttributeOperationEnable, >>=20 >=20 > These parts look fine to me. >=20 > Before proceeding to patch#11 in this series, I'll wait for your = answer; > I think I need to read & understand it in order to continue the = review. ACK on the rest of the points that I didn't leave a comment under, I = will change in v5. Thanks for the quick review! Nikita >=20 > Thanks! > Laszlo >=20