From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 04 Oct 2019 12:54:46 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 817193090FDD; Fri, 4 Oct 2019 19:54:45 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-253.rdu2.redhat.com [10.10.121.253]) by smtp.corp.redhat.com (Postfix) with ESMTP id 666AA5D6B7; Fri, 4 Oct 2019 19:54:43 +0000 (UTC) Subject: Re: [edk2-devel] [edk2-platforms: PATCH 1/1] DisplayLinkPkg: DisplayLinkGop: Added GOP driver for USB docking stations based on DisplayLink chips To: Andy Hayes Cc: devel@edk2.groups.io, leif.lindholm@linaro.org, Michael D Kinney , "Ni, Ray" , Michael A Rothman References: <20190819133200.14577-1-andy.hayes@displaylink.com> <20190819133200.14577-2-andy.hayes@displaylink.com> <20190830152717.GV29255@bivouac.eciton.net> From: "Laszlo Ersek" Message-ID: <985de369-7880-b6cc-46e7-5a2edca6582b@redhat.com> Date: Fri, 4 Oct 2019 21:54:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190830152717.GV29255@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.43]); Fri, 04 Oct 2019 19:54:45 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Andy, I've got a question about your edk2-platforms commit 9df63499ea01 (i.e., this patch): On 08/30/19 17:27, Leif Lindholm wrote: > On Mon, Aug 19, 2019 at 02:32:00PM +0100, Andy Hayes wrote: [...] >> diff --git a/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.c b/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.c [...] >> +EFI_STATUS >> +DlReadEdid ( >> + IN USB_DISPLAYLINK_DEV* UsbDisplayLinkDev >> + ) >> +{ [...] >> + Status = EdidOverride->GetEdid ( >> + EdidOverride, >> + &UsbDisplayLinkDev->Handle, >> + &EdidAttributes, >> + &EdidDataSize, >> + &EdidDataBlock); In UEFI v2.8, EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID's "ChildHandle" parameter is specified as follows, in *natural language*: ChildHandle -- A child handle that represents a possible video output device. Accordingly, the function *prototype* should specify the parameter as: IN EFI_HANDLE ChildHandle However, the spec in fact mandates: IN EFI_HANDLE *ChildHandle there. This is most likely a typo in the prototype, and should be fixed (so that the prototype match the natural language description). I've reported the following spec ticket about it: https://mantis.uefi.org/mantis/view.php?id=2018 Here's the problem. It looks like most GetEdid() calls in existence conform to the *spirit* of the spec, and not to the *letter* thereof; in other words, they pass an EFI_HANDLE for ChildHandle. Furthermore, the EFI_EDID_OVERRIDE_PROTOCOL implementations underneath also conform to the spirit, and not the letter of the spec, and they consume ChildHandle as an EFI_HANDLE. (They do not de-reference ChildHandle for getting the actual handle.) In other words, fixing the typo in the spec would simply adopt existing practice, and no code would have to be changed for spec conformance. Except... the code quoted above. The code above conforms to the *letter* of the spec, and passes an (EFI_HANDLE*), as ChildHandle. If the typo is fixed in the spec, the call site in DlReadEdid() would have to be modified. Can you please confirm wheter: - the EdidOverride->GetEdid() call is reached in practice (= it is not dead code), - the underlying EFI_EDID_OVERRIDE_PROTOCOL actually consumes ChildHandle (for which it first has to de-reference it)? My hope is that either the above call site is dead code en-bloc, or the underlying protocol implementation ignores the ChildHandle parameter. Then we can fix both the spec and the DlReadEdid() function. Thanks! Laszlo