From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x230.google.com (mail-it0-x230.google.com [IPv6:2607:f8b0:4001:c0b::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1CB6A81E9D for ; Tue, 15 Nov 2016 07:10:43 -0800 (PST) Received: by mail-it0-x230.google.com with SMTP id l8so7569009iti.1 for ; Tue, 15 Nov 2016 07:10:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=AGzhr9DN5GiX9qDJd2UmKdPpdk3wwttBaru+ay3DGxU=; b=bzi3JvYFjF22BY72Y6H6pJ6N0sPOiDZrx5OVwkQohGq2EaAAbczq8/kgfDO9Q+ujN/ 0CKqPfEXB3xitXUVNxq90LTfEPfs6QBf8jMiXeM6/NZpqGpNW0mKr8yn/ZJZqxpWElwg HjjHAf7XLLC1y8r2FU4Dl0e+k1YmChOiHrgrg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=AGzhr9DN5GiX9qDJd2UmKdPpdk3wwttBaru+ay3DGxU=; b=UYxPBlUbGRUoY9GTNAkRHO8Oj4D7pe7efLrb5Kike9gPz7QQNG6lltf5KaICrL/MXx uM6vwg1u4dGLWQDPAJHGiHSbgEQv7Ttxm7PUbnetIx/xVEGWip//Gp20qu6OXzdm8R5k rN8uvjUCfxzrLVeCleG3XmcSCPRrMhb7kyJ5eXGSlgjFvXzngfgn50q4AvOh8FWg+QFk MaQP7OqbcHHzreJQpGYNhL/nGE8FAgV7G2zOHnkN76Z3uczRYQ4ytRzfy5y5D7v1Gb5L QE9TGuxqgB3HYnn9yo6byyjOePtzx5u7Eq7NJJZ6ta18EDu5604C6JSEnlbUM/rONuzc SyeA== X-Gm-Message-State: ABUngve06tLxVkf1xOn+UJSGeD3DJ8lv5j1K0tSdqYpGbAvrd/r3MvGZhQ0N8QIZtpsSxUmWTaS3qgApOyPaPhZ1 X-Received: by 10.36.185.83 with SMTP id k19mr3117165iti.59.1479222646993; Tue, 15 Nov 2016 07:10:46 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.59.147 with HTTP; Tue, 15 Nov 2016 07:10:46 -0800 (PST) In-Reply-To: References: <1479157789-14674-1-git-send-email-jeremy.linton@arm.com> From: Ard Biesheuvel Date: Tue, 15 Nov 2016 15:10:46 +0000 Message-ID: To: Jeremy Linton Cc: edk2-devel-01 , Steve Capper , Leif Lindholm , "ryan.harkin@linaro.org" , linaro-uefi Subject: Re: [PATCH 0/8] ATAPI support on SiI SATA adapter X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Nov 2016 15:10:43 -0000 Content-Type: text/plain; charset=UTF-8 On 15 November 2016 at 15:05, Jeremy Linton wrote: > |-----Original Message----- > |From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > |Sent: Tuesday, November 15, 2016 8:58 AM > |To: Jeremy Linton > |Cc: edk2-devel-01; Steve Capper; Leif Lindholm; ryan.harkin@linaro.org; > |linaro-uefi > |Subject: Re: [edk2] [PATCH 0/8] ATAPI support on SiI SATA adapter > | > |On 15 November 2016 at 14:54, Jeremy Linton > |wrote: > |> On 11/15/2016 01:43 AM, Ard Biesheuvel wrote: > |>> > |>> Hi Jeremy, > |>> > |>> On 14 November 2016 at 21:09, Jeremy Linton > |wrote: > |>>> > |>>> The SiI isn't an AHCI compatible adapter so it implements the EFI > |>>> ATA pass-through protocol directly. This works for fixed hard > |>>> drives, but not ATAPI attached devices (CDROM, DVDROM, TAPE, etc). > |>>> > |>>> This patch adds read only ATAPI support via the EFI SCSI > |>>> pass-through protocol, allowing boot from attached CD/DVD. This > |>>> patch also cleans up, and tweaks recovery paths/etc in the original driver. > |>> > |>> > |>> Very nice! Thanks for getting to the bottom of this. > |>> > |>> However, looking at the patches, they are riddled with coding style > |>> violations. I am usually less strict than Leif when it comes to > |>> upholding those, but these patches really need to be cleaned up to be > |>> considered for merging. > |> > |> > |> > |> Is there a tool which can correct or at least point out the formatting > |> errors? > |> > | > |Yes, BaseTools/Scripts/PatchCheck.py > > I did use that before submission, the only thing it complained about was an error caused by the git submodule commit id not having a CR (which AFAIK is bogus) > > [jlinton@mammon-v1 edk2]$ python BaseTools/Scripts/PatchCheck.py -8 |more > Checking git commit: 4a9a9d7 > The commit message format passed all checks. > The code passed all checks. > > Checking git commit: 8537b6c > The commit message format passed all checks. > The code passed all checks. > > Checking git commit: 458452e > The commit message format passed all checks. > The code passed all checks. > > Checking git commit: ca0606f > The commit message format passed all checks. > Code format is not valid: > * Line ending ('\n') is not CRLF > File: OpenPlatformPkg > Line: Subproject commit a072474a3b05ec7f12f2d4db271cc07a0cf7a369 > > Checking git commit: 0d9942f > The commit message format passed all checks. > The code passed all checks. > > Checking git commit: b516f7a > The commit message format passed all checks. > The code passed all checks. > > Checking git commit: 2259641 > The commit message format passed all checks. > The code passed all checks. > > Checking git commit: 61be9c2 > The commit message format passed all checks. > The code passed all checks. > Hmm, ok, that is strange. What I spotted when going over the patches was lots of initialized stack variables, which the EDK2 coding style does not allow. Local variables should be initialized using assignments not initializers (for some reason, which I think may have to do with generated code size on some toolchains) Other things to check for is spaces around '=' and the use of STATIC for things that are only referenced locally. In any case, I will go through the patches again to comment in some more detail. Thanks, Ard.