From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C1456803B0 for ; Mon, 13 Mar 2017 14:32:57 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP; 13 Mar 2017 14:32:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,161,1486454400"; d="scan'208";a="235601590" Received: from yjiang5-mobl.amr.corp.intel.com (HELO localhost) ([10.254.127.228]) by fmsmga004.fm.intel.com with ESMTP; 13 Mar 2017 14:32:56 -0700 MIME-Version: 1.0 To: Laszlo Ersek , "Kinney, Michael D" , Message-ID: <148944077646.15816.5488638936147140185@jljusten-skl> From: Jordan Justen In-Reply-To: <55e1eade-1d06-4b04-3e07-5ceef61a5602@redhat.com> Cc: edk2-devel-01 References: <20170223014814.10937-1-lersek@redhat.com> <20170223014814.10937-12-lersek@redhat.com> <148914088572.26490.12215430240628550434@jljusten-skl> <55e1eade-1d06-4b04-3e07-5ceef61a5602@redhat.com> User-Agent: alot/0.5.1 Date: Mon, 13 Mar 2017 14:32:56 -0700 Subject: Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib 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: Mon, 13 Mar 2017 21:32:57 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-03-10 11:36:36, Laszlo Ersek wrote: > On 03/10/17 11:14, Jordan Justen wrote: > > On 2017-02-22 17:48:13, Laszlo Ersek wrote: > >> + // > >> + ScratchBuffer->Features =3D mSmiFeatures; > >> + Status =3D QemuFwCfgS3WriteBytes (mRequestedFeaturesItem, > > = > > I noticed a fair amount of function calls where the first args are > > appearing on the first line, and then the rest appear on subsequent > > lines indented two spaces. > = > Yes, that's intentional. The coding style nominally requires each > argument on a separate line (if they don't all fit on a single > line), but that wastes a huge amount of vertical space (which is bad > because less code fits in a screenful). So I frequently follow a > semi-compressed style where I place the arguments continuously, but > wherever I have to break the line (because of the 80 char limit) I > stick with the indentation required by the coding style. Also, when > I apply this style, I don't break the final closing paren off to a > separate line (because this style is primarily contiguous). > = > I mostly employ this style when the arguments are otherwise simple > (i.e., when the "one arg per line" style does not improve clarity, > just wastes a lot of space). > = > This is not new, I've been coding like this in edk2 virtually > forever. > I understand that I've given an r-b in the past for code like this. I have noticed it previously, but in some cases it doesn't look as strange. For example, with the DEBUG macro, the error level seems less important to move to the next line. I guess it is probably bad to make an exception there. I do think that lining up the arguments should be prioritized over saving vertical whitespace. I think we just have to accept that EDK II is not efficient in terms of vertical space. In fact, EDK II is just plain verbose in general. :) At least we don't follow the kernel's strict 8 char indent and 80 columns limit. (Although, there are some good arguments in favor of this in terms of forcing code to be broken down into smaller functions.) I would like to know if Mike has any opinion on whether this is a bug in the coding style spec to not address this more definitively. -Jordan > > = > >> + sizeof ScratchBuffer->Features); > >> + if (RETURN_ERROR (Status)) { > >> + goto FatalError; > >> + } > >> + > > = > > I can't say that the coding style prohibits this, but it does seem odd > > looking. I would expect to see: > > = > > [1] > > = > > Status =3D QemuFwCfgS3WriteBytes ( > > mRequestedFeaturesItem, > > sizeof ScratchBuffer->Features > > ); > > = > > The coding style examples look like this: > > = > > Status =3D QemuFwCfgS3WriteBytes ( > > mRequestedFeaturesItem, > > sizeof ScratchBuffer->Features > > ); > > = > > But this looks less like what I'm used to actually seeing in EDK II. > > (Look at DXE Core, for example. It looks like [1].) > > = > > I personally think this is okay to save a line, but it doesn't seem to > > follow the coding style doc: > > = > > Status =3D QemuFwCfgS3WriteBytes ( > > mRequestedFeaturesItem, > > sizeof ScratchBuffer->Features); > > = > > Now, we all know EDK II has a style of it's own, but outide EDK II, I > > might expect something like: > > = > > Status =3D QemuFwCfgS3WriteBytes(mRequestedFeaturesItem, > > sizeof ScratchBuffer->Features); > > = > > Which once again shows arguements on subsequent lines are lined up. > > Based on that, I think [1] is the best style for EDK II code. > = > Yes, [1] is the official style, and I stick with that when the arguments = are complex or long, or require separate comments. > = > However, as I said above, when the arguments are simple, it makes sense t= o place them contiguously (together with the final paren), while preserving= the edk2 indentation. > = > Again, I've been doing this for years, consistently, and you've never see= med to take issue with it. For example, some snippets from the virtio block= driver: > = > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 290) // > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 291) // virti= o-blk header in first desc > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 292) // > 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 293) VirtioAp= pendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, > e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 294) VRING_= DESC_F_NEXT, &Indices); > = > = > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 310) // > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 311) // VRI= NG_DESC_F_WRITE is interpreted from the host's point of view. > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 312) // > 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 313) Virtio= AppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize, > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 314) VRIN= G_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE), > e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 315) &Ind= ices); > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 316) } > = > = > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 318) // > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 319) // host = status in last (second or third) desc > fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 320) // > 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 321) VirtioAp= pendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus, > e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 322) VRING_= DESC_F_WRITE, &Indices); > = > = > 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 674) Status= =3D VIRTIO_CFG_READ (Dev, Topology.PhysicalBlockExp, > 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 675) = &PhysicalBlockExp); > = > = > More recent code from e.g. "SmmAccessPei.c": > = > = > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 186) return Smram= AccessGetCapabilities (This->LockState, This->OpenState, > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 187) Smr= amMapSize, SmramMap); > = > = > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 267) DEBUG ((EF= I_D_ERROR, "%a: no SMRAM with host bridge DID=3D0x%04x; only " > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 268) "DID=3D0= x%04x (Q35) is supported\n", __FUNCTION__, HostBridgeDevId, > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 269) INTEL_Q3= 5_MCH_DEVICE_ID)); > = > = > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 309) // > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 310) // Given the= zero graphics memory sizes configured above, set the > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 311) // graphics-= related stolen memory bases to the same as TOLUD. > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 312) // > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 313) PciWrite32 (= DRAMC_REGISTER_Q35 (MCH_GBSM), > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 314) TopOfLowRa= mMb << MCH_GBSM_MB_SHIFT); > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 315) PciWrite32 (= DRAMC_REGISTER_Q35 (MCH_BGSM), > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 316) TopOfLowRa= mMb << MCH_BGSM_MB_SHIFT); > = > = > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 348) Status =3D S= mramAccessGetCapabilities (mAccess.LockState, mAccess.OpenState, > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 349) &= SmramMapSize, SmramMap); > = > = > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 376) CopyMem (Gui= dHob, &SmramMap[DescIdxSmmS3ResumeState], > 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 377) sizeof Smr= amMap[DescIdxSmmS3ResumeState]); > = > = > Brand new code from "OvmfPkg/AcpiPlatformDxe/BootScript.c": > = > = > df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 107) Context->Wri= tePointers =3D AllocatePool (WritePointerCount * > df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 108) = sizeof *Context->WritePointers); > = > = > df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 191) DEBUG ((DEBU= G_VERBOSE, "%a: 0x%04x/[0x%08x+%d] :=3D 0x%Lx (%Lu)\n", > df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 192) __FUNCTION= __, PointerItem, PointerOffset, PointerSize, PointerValue, > df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 193) (UINT64)S3= Context->Used)); > = > = > df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 247) Status =3D g= BS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, > df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 248) = NULL /* Registration */, (VOID **)&S3SaveState); > = > I've been entirely consistent about this. > = > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel