From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::243; helo=mail-wr0-x243.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x243.google.com (mail-wr0-x243.google.com [IPv6:2a00:1450:400c:c0c::243]) (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 5C2F8210F4BA9 for ; Fri, 15 Jun 2018 09:13:30 -0700 (PDT) Received: by mail-wr0-x243.google.com with SMTP id a12-v6so10482148wro.1 for ; Fri, 15 Jun 2018 09:13:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=QSUpz3fRZA3IYj6HkGVUFaqv6/YLVO1uiGo47xTPlw8=; b=OOzGhtC+WDTDwAVGDJv9VSsoICTeJwgae+Otj9amUqjCwxGfVe0fATrn0yP2iU5wUn /qmD0xZ0cVj6KZg0GJXgiybb8W8lE2ssdsBGxne2wrf959d8qzsleCCFvv1UIU7LjyfL AFuAjFDDPzP5mx/sRkRZujYrdAF17I7bJrDhA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=QSUpz3fRZA3IYj6HkGVUFaqv6/YLVO1uiGo47xTPlw8=; b=Z/0yRqamSrGspIBr7vKlIpG7QjKrt5r+eCwWme+PLHAriFrP2zCOzS2w0WOR1hicUG JZX+Jzxno2SUPfJREOIKyg7Go7epuHB5jhwUHwch3O3NavsHFYauE9VBX3hF4JZ3xEDW /MGOMC5lIrqpJ4eki3HFTKJix/kaX/V4Xw8KqSdvL/U5hlygDwQ6NC+Ex3a5cXyNhfRS lscproUO6DR37m3AjpP20CXLEdChrVk8cTovOK+ujGSqM399Rw/eYtQjANQm7FQPThAq +IQOXq/Ky1JMqUTS6tess3wdy+cXpc11GYoA4rr8axAo5jTBqcFAKXzxfV34xhKAWV52 FKeQ== X-Gm-Message-State: APt69E1VBrkx4SB9Go8Q+jagfciBrcsX92XSOE+1z9c4LNO8XAisNUrU rYBiVaxaQ4l4IeE1oibWf4wkAw== X-Google-Smtp-Source: ADUXVKICMBDEhQSDMtbx9MooP8mDTyHAOjZ8oIBa7620iwMDpKhDLvsSaQZ30KL+npuJXKVNr45ZxA== X-Received: by 2002:adf:fc05:: with SMTP id i5-v6mr2200326wrr.157.1529079209418; Fri, 15 Jun 2018 09:13:29 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 72-v6sm9149590wrb.22.2018.06.15.09.13.27 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 15 Jun 2018 09:13:28 -0700 (PDT) Date: Fri, 15 Jun 2018 17:13:26 +0100 From: Leif Lindholm To: Evan Lloyd Cc: Ard Biesheuvel , Sami Mujawar , "edk2-devel@lists.01.org" , "Zeng, Star" , Eric Dong , Ruiyu Ni , Matteo Carlini , Stephanie Hughes-Fitt , Thomas Abraham , nd Message-ID: <20180615161326.7rl74schpgor6h6c@bivouac.eciton.net> References: <20180615141348.13020-1-sami.mujawar@arm.com> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Jun 2018 16:13:31 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Jun 15, 2018 at 03:26:54PM +0000, Evan Lloyd wrote: > Hi Ard, Zeng > > > -----Original Message----- > > From: Ard Biesheuvel > > Sent: 15 June 2018 15:17 > > To: Sami Mujawar > > Cc: edk2-devel@lists.01.org; Zeng, Star ; Eric Dong > > ; Ruiyu Ni ; Leif Lindholm > > ; Matteo Carlini ; > > Stephanie Hughes-Fitt ; Evan Lloyd > > ; Thomas Abraham ; > > nd > > Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem > > space > > > > On 15 June 2018 at 16:13, Sami Mujawar wrote: > > > The SATA controller driver crashes while accessing the PCI memory > > > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled. > > > > > > Enable the PCI memory space access to prevent the SATA Controller > > > driver from crashing. > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Sami Mujawar > > > --- > > > The changes can be viewed at > > > > > https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f > > i > > > x_v2 > > > > > > Notes: > > > v2: > > > - Improved log message and code documentation based on feedback > > [SAMI] > > > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE [ZENG] > > > - This SATA Controller driver only uses the PCI BAR5 register > > > space which is the AHCI Base Address (ABAR). According to the > > > 'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1' > > > specification, section 2.1.11, 'This register allocates space > > > for the HBA memory registers'. > > > The section 2.1.10, allows provision for Optional BARs which > > > may support either memory or I/O spaces. However, in the context > > > of the current SATA controller driver, which only ever access > > > the ABAR, enabling I/O memory space is not required. [SAMI] > > > - Prefer to use // surrounding comments [ZENG] > > > - Doing this would violate the edk2 coding standard. See EDK2 > > > Coding Standard Specification, revision 2.20, section 6.2.3. [SAMI] > > > > > > > Please stop obsessing about the coding standard. The maintainer was quite > > clear what he wanted, and in the past, I have also indicated that for the ARM > > related packages, I prefer readability and consistency over adherence to a > > dubious standard. > > [[Evan Lloyd]] I'd like to make some points here: > 1. It is perfectly reasonable for Sami to explain why he has done > something a certain way - Zeng is then at liberty to respond > with his preference, but we do not (yet) know what that might > be. Yes we do. Zeng responded with that in the first instance. Sami then disputed that explicitly stated preference, referring to the coding standard. There was no lack of clarity in what Zeng wanted - so I remain unclear on what this is intending to achieve? > 2. "readability and consistency" is the very purpose of any > coding standard. If consistency is valuable, doesn't that > apply across modules? I understand that it may be > particularly valuable for maintainers within their modules, > but the rest of us benefit from a consistent style - > especially when looking outside our normal demesne. At which point the rule of thumb is: aligning with the immediately surrounding code always trumps adhering to the specified style. Which is in itself trumped by the maintainer explicitly stating another preference. (Like here.) > 3. Applying a consistent Coding Standard across the whole project > is a necessary pre-condition for automated CI checks on new > submissions. I'd like to aim e.g. for an improved > patchcheck.py, but that requires consistency across modules, > at least for new code. Such a system will _never_ be fully automatable. I will _always_ take a 80 < x < 90 line over one that breaks up an output string or one that reduces readability more than having to side-scroll does. That doesn't make it worthless, far from it. > Note: I am not disputing the dubiosity of the CCS. It could be > improved (a lot). However it is all we have right now. Then a more constructive approach is to recognise that not a single one of the maintainers are appearing to be respecting the horror vacui rule and submit a patch against https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification to have section 6.2.3 deleted. (Much like I should at some point get around to have 5.4.2.2.2 deleted, or at least conditionalised to only apply for !ARM.) Regards, Leif > Regards, > Evan > > > > > > > > v1: > > > - Fix SATA Controller driver crash [SAMI] > ... > > > -- > > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > > >