Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Jun 15, 2018 at 03:26:54PM +0000, Evan Lloyd wrote:
> Hi Ard, Zeng
> 
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Sent: 15 June 2018 15:17
> > To: Sami Mujawar <Sami.Mujawar@arm.com>
> > Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Eric Dong
> > <eric.dong@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>; Leif Lindholm
> > <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>;
> > Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; Evan Lloyd
> > <Evan.Lloyd@arm.com>; Thomas Abraham <thomas.abraham@arm.com>;
> > nd <nd@arm.com>
> > Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem
> > space
> > 
> > On 15 June 2018 at 16:13, Sami Mujawar <sami.mujawar@arm.com> 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 <sami.mujawar@arm.com>
> > > ---
> > > 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)'
> > >
> > >