Re: [edk2-platforms][PATCH v2 2/3] Platform/RPi: Add PlatformPcdLib to set the Genet MAC address

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Fri, 31 Jan 2020 at 19:45, Pete Batard <pete@akeo.ie> wrote:
>
> On 2020.01.31 17:53, Ard Biesheuvel wrote:
> > On Fri, 31 Jan 2020 at 18:48, Pete Batard <pete@akeo.ie> wrote:
> >>
> >> Hi Ard,
> >>
> >> On 2020.01.31 17:05, Ard Biesheuvel wrote:
> >>> On Fri, 24 Jan 2020 at 12:54, Pete Batard <pete@akeo.ie> wrote:
> >>>>
> >>>> The Genet driver stub used by the Raspberry Pi 4 platform is
> >>>> designed to set the MAC address according to a PCD.
> >>>>
> >>>> To be able to set that PCD at runtime, by using the Raspberry
> >>>> Pi firmware interface, that has a dedicated call to retrieve
> >>>> the MAC address, and satisfy driver dependencies in a generic
> >>>> manner, we create a new PlatformPcdLib that can be referenced
> >>>> by the Genet driver, to set the MAC PCD before use there.
> >>>>
> >>>> While it is currently only tailored around MAC PCD population
> >>>> for Genet, we do expect this PCD library to be extended in the
> >>>> future, to provide additional PCD facilities for other drivers.
> >>>>
> >>>> Signed-off-by: Pete Batard <pete@akeo.ie>
> >>>> ---
> >>>>    Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c   | 51 ++++++++++++++++++++
> >>>>    Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf | 43 +++++++++++++++++
> >>>>    2 files changed, 94 insertions(+)
> >>>>
> >>>> diff --git a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c
> >>>> new file mode 100644
> >>>> index 000000000000..792073a903e9
> >>>> --- /dev/null
> >>>> +++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c
> >>>> @@ -0,0 +1,51 @@
> >>>> +/** @file
> >>>> + *
> >>>> + *  Copyright (c) 2020, Pete Batard <pete@akeo.ie>
> >>>> + *
> >>>> + *  SPDX-License-Identifier: BSD-2-Clause-Patent
> >>>> + *
> >>>> + **/
> >>>> +
> >>>> +#include <Library/DebugLib.h>
> >>>> +#include <Library/PcdLib.h>
> >>>> +#include <Library/PrintLib.h>
> >>>> +#include <Library/UefiBootServicesTableLib.h>
> >>>> +#include <Library/UefiLib.h>
> >>>> +#include <Library/UefiRuntimeServicesTableLib.h>
> >>>> +#include <Protocol/RpiFirmware.h>
> >>>> +
> >>>> +EFI_STATUS
> >>>> +EFIAPI
> >>>> +PlatformPcdLibConstructor (
> >>>> +  IN EFI_HANDLE ImageHandle,
> >>>> +  IN EFI_SYSTEM_TABLE *SystemTable
> >>>> +  )
> >>>> +{
> >>>> +
> >>>> +  //
> >>>> +  // If Genet is in use and a MAC address PCD has not been set, do it here.
> >>>> +  //
> >>>> +#if (FixedPcdGet64 (PcdBcmGenetRegistersAddress) != 0)
> >>>
> >>> I still don't see why we need this conditional. This is a fixed PCD,
> >>> so it must be set in the .DSC. If you are going to set it in the .DSC,
> >>> why include this driver in the first place?
> >>
> >> Let's consider this:
> >>
> >> 1. We expand on PlatformPcdLib to do more than set the MAC Address PCD
> >> for the Pi 4. For instance, we use it to set some other PCD, that is
> >> specific to the Pi 3, or for an upcoming Pi. In this usage, we would be
> >> using PlatformPcdLib with a .DSC where PcdBcmGenetRegistersAddress is
> >> not set and therefore defaults to 0.
> >>
> >
> > I don't think a single PlatformPcdLib is realistic, since you'd need
> > to link it into different drivers via NULL resolution, all of which
> > would inherited the conjunction of all the DEPEX clauses, potentially
> > causing circular dependencies.
> >
> > I'd much rather have either a named library class in BcmGenet.dec
> > scope that the platform can implement to provide the MAC, or one that
> > can be used via NULL resolution (in cases where it is imaginable that
> > the driver does not need the MAC to be provided at all)
>
> Yes, but the problem is, right now, I have no idea what the Genet driver
> is going to look like, so we need a solution that is unlikely to
> interfere with what's going to happen in terms of Genet driver
> expansion. And this is the least intrusive solution I identified (which
> means it will also be easiest to remove if we decide to supersede it).
>

If the genet stub driver morphs into something more elaborate, it will
still require a platform specific way to discover the MAC the
platforms wants it to use.

> I personally don't think a named library is the better solution because,
> ideally, we should have two independent drivers (not libraries), one for
> UMAC and one for Genet, considering that we really want is have the
> platform rely on a UMAC hardware service implementation since we are
> dealing with 2 separate hardware functions, that pertain to networking
> but that are not directly related. So if you want to go towards the
> "There has to be a better solution that this" route, I'm would actually
> push for a separate driver rather than a library. But then I expect that
> we're going to create all kinds of problems for Jeremy when he produces
> his Genet implementation, which is precisely why I steered away from
> doing that in the first place. There are also other alternatives, such
> as implementing Simple Network Driver protocol, since this appears to
> provide means to set the MAC, but this too is going to interfere with
> Jeremy's effort.
>
> So, and I hope this doesn't come as offensive because this is not my
> intention here, I'm afraid that, unless someone else takes that matter
> into their own hand, the current proposal is the best you're gonna get,
> unless you want to produce that named library class yourself (which,
> again, I still don't view as the better solution if that's what we are
> looking for).
>

OK

> What also bothers me is that you did state at the end of [1] that:
>
>  > I don't mind this approach, but in general, I'd be happier with a
>  > specific library class to discover the MAC address.
>
> which I took as a tacit approval that, even if you didn't exactly like
> it, you were still okay with applying this patchset as long as the other
> issues were addressed. So I hope you can appreciate that I can't go
> around with expecting approval on the general approach of a patchset,
> only to see it suddenly rejected by the same person one week later...
>

That was before you refused to remove the #if PcdGet64() == 0

> But more pragmatically, we need a way to get networking in Linux, which
> means UMAC setup in UEFI, now. Not in two weeks. Not in one month. And
> short of someone coming up with an alternative tomorrow, I'm still
> seeing PCDLib lib as the one means to achieve that, that is going to
> create the least trouble for all the parties involved.
>
> Regards,
>
> /Pete
>
> [1] https://edk2.groups.io/g/devel/message/53447
>
> >> 2. Since the whole PcdBcmGenetMacAddress setup section would not apply
> >> then, we prefer ensuring that it cannot interfere or create issues, by
> >> dropping it altogether at build time. Granted, the
> >>     if (PcdGet64 (PcdBcmGenetMacAddress) == 0)
> >> condition could also be seen as enough of a guard, but I don't see much
> >> of a reason not to go further, so that further alteration of the code
> >> does not risk impacting the Pi 3 or other Pi platforms with future
> >> developments.
> >>
> >> Does that make sense to you? Or do you see this as too far fetched a
> >> scenario?
> >>
> >
> > No, I don't think this is something we should care about.
> >
> >> I'm basically trying to make this whole library easier to maintain in
> >> the long run, by adding some pre-emptive provisions to drop code that
> >> may not apply (even if, in the current scenario, this library is only
> >> used for the Pi 4 platform, where PcdBcmGenetRegistersAddress is always
> >> set).
> >>
> >
> > I don't see a single library that sets all kinds of unrelated PCDs as
> > something highly useful tbh.
> >
>