Re: [PATCH 0/8] ATAPI support on SiI SATA adapter

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 15 November 2016 at 15:05, Jeremy Linton <Jeremy.Linton@arm.com> 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 <jeremy.linton@arm.com>
> |wrote:
> |> On 11/15/2016 01:43 AM, Ard Biesheuvel wrote:
> |>>
> |>> Hi Jeremy,
> |>>
> |>> On 14 November 2016 at 21:09, Jeremy Linton <jeremy.linton@arm.com>
> |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.