Re: [PATCH edk2-platforms 08/14] Silicon/Socionext: add driver for NETSEC network controller

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

On Sat, Oct 28, 2017 at 02:06:42PM +0100, Ard Biesheuvel wrote:
> On 11 September 2017 at 17:12, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, Sep 08, 2017 at 07:23:09PM +0100, Ard Biesheuvel wrote:
> >> This adds the NetSecDxe driver provided by Socionext, but reworked
> >> extensively to improve compliance with the SimpleNetworkProtocol API,
> >> and to avoid uncached allocations for streaming DMA.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/NetsecDxe.c                                                     | 1000 ++++++++++++++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/NetsecDxe.dec                                                   |   47 +
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/NetsecDxe.h                                                     |   88 ++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/NetsecDxe.inf                                                   |   69 +
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/include/ogma_api.h                   |  736 ++++++++++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/include/ogma_basic_type.h            |   45 +
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/include/ogma_version.h               |   24 +
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_basic_access.c              |   88 ++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_basic_access.h              |   52 +
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_desc_ring_access.c          | 1391 +++++++++++++++++++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_desc_ring_access_internal.h |  111 ++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_gmac_access.c               | 1454 ++++++++++++++++++++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_internal.h                  |  210 +++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_misc.c                      | 1385 +++++++++++++++++++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_misc_internal.h             |   38 +
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_reg.h                       |  219 +++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_reg_f_gmac_4mt.h            |  222 +++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/netsec_sdk/src/ogma_reg_netsec.h                |  368 +++++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/ogma_config.h                                   |   25 +
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/pfdep.h                                         |  265 ++++
> >>  Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/netsec_for_uefi/pfdep_uefi.c                                    |  176 +++
> >>  21 files changed, 8013 insertions(+)
> >>
> >> diff --git a/Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/NetsecDxe.c b/Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/NetsecDxe.c
> >> new file mode 100644
> >> index 000000000000..7c3f12362f14
> >> --- /dev/null
> >> +++ b/Silicon/Socionext/Synquacer/Drivers/Net/NetsecDxe/NetsecDxe.c
> >> @@ -0,0 +1,1000 @@
> >> +/** @file
> >> +
> >> +  Copyright (c) 2016 Socionext Inc. All rights reserved.<BR>
> >> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
> >> +
> >> +  This program and the accompanying materials
> >> +  are licensed and made available under the terms and conditions of the BSD License
> >> +  which accompanies this distribution.  The full text of the license may be found at
> >> +  http://opensource.org/licenses/bsd-license.php
> >> +
> >> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >> +
> >> +**/
> >> +
> >> +#include <Library/DebugLib.h>
> >> +#include <Library/DmaLib.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> +#include <Library/MemoryAllocationLib.h>
> >> +#include <Library/IoLib.h>
> >> +#include <Library/NetLib.h>
> >
> > Sorted alphabetically, please?
> >
> >> +
> >> +#include "NetsecDxe.h"
> >> +#include "netsec_for_uefi/pfdep.h"
> >
> > Hmm, could that be folded into NetsecDxe.h?
> >
> 
> Yes, but I would have to modify all the 'platform independent' source
> files, and not having to modify them is kind of the point.
> ...

I was actually referring to just the #include statement.
But part of the reason I suggested that was that I was hoping to
reduce the amount on non-conforming style.
Further reading though the rest of the file showed this to be a drop
in the ocean really.

> >> +
> >> +  ogma_disable_desc_ring_irq (LanDriver->Handle, OGMA_DESC_RING_ID_NRM_TX,
> >> +                              OGMA_CH_IRQ_REG_EMPTY);
> >> +
> >> +  // ##### configure_mac
> >
> > In general, it feels like each of these comment headers indicate a
> > good place to break a block out into a helper function.
> >
> 
> Meh. This function is not complex at all, it just does a bunch of
> steps in sequence. Don't see the point really.
> ...

It's more a question of overviewability without familiarity with the
underlying implementation. But it's a preference rather than anything
stronger.

> Apologies for snipping the context - my edit window became intolerably
> slow due to the size of the email. I /think/ I incorporated all other
> feedback you gave to this patch.

Yes, I think so.

/
    Leif