Re: [edk2-devel] [PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols

["Ard Biesheuvel" <ardb@kernel.org>]

On Fri, 17 Feb 2023 at 15:55, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 17. Feb 2023, at 15:29, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Fri, 17 Feb 2023 at 15:05, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>
> >> Hi Ard,
> >>
> >> Thank you! Is "1/4" a mistake or did I miss the other 3? :)
> >
> > Oops.
> >
> > It was part of some RPi4 patches but I decided to send it out by itself.
> >
> >
> >> Comments inline.
> >>
> >> On 17. Feb 2023, at 12:12, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> The Unicode collation protocols, however, are different: loading the
> >> driver will fail if neither of those are present. So they are not
> >> TO_START protocols, and they need to be mentioned in the DEPEX so that
> >> the DXE core will not dispatch the driver before the producers of the
> >> prerequisite protocols have been dispatched. Also, mark them as
> >> SOMETIMES_CONSUMES, as only one of the two is required.
> >>
> >>
> >> Right. FatPkg solves this by probing for the protocol in Start() [1], which should guarantee that all entry points have been executed first, right? I'd prefer a universal and consistent solution to the issue and this looks fine, honestly.
> >>
> >> [1]
> >> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.c#L381
> >> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPkg/EnhancedFatDxe/Fat.inf#L19
> >>
> >
> > I'd prefer using existing features for this, rather than open coding it.
>
> It‘s not like we replicate a feature, we just move a function call to suit the control flow better. I‘d like a consistent solution and the FatPkg one looks fine to me.
>

So the FAT driver will happily load, but then fail in an obscure
manner when being started on a controller handle, in a way that is
indistinguishable (afict) from a partition that has not FAT file
system in the first place.

So it doesn't look fine to me at all, tbh. If the collation protocols
are required, we should check for them in supported(), and make a lot
of noise in a DEBUG build if the failure condition is an unexpected
one, i.e., some additional protocol we dependent on (even though we
pretend to be a UEFI driver)  does not exist.

Alternatively, we might defer installation of the driver binding
protocol implementation to a callback on the collation protocol
installation, but that seems like more work for the same result.


> >
> >
> >> -  MODULE_TYPE                    = UEFI_DRIVER
> >> +  MODULE_TYPE                    = DXE_DRIVER
> >>
> >>
> >> Is it not unidiomatic to use the UEFI Driver Binding model (UEFI) in a DXE driver (UEFI PI)?
> >>
> >
> > Perhaps. But having a hard dependency on protocols beyond the default
> > set defined for UEFI drivers is arguably even worse.
>
> This is still very much a UEFI driver in a logical sense, this is just abusing the types to utilise DXE concepts. I‘m not opposed to such things in general, but here it looks unnecessary. It doesn’t help I’m not a big fan of the DXE dispatcher to begin with. :)
>

I will +1 any solution that removes the DXE dispatcher entirely as a
side effect, but unfortunately, that is not going to happen :-)

> I agree the dependency is awkward, but I have to check the reason and alternatives later. In the end, it‘s Pedro‘s call anyway.
>

Indeed.

> >
> >
> >> +[Depex]
> >> +  gEfiUnicodeCollationProtocolGuid OR gEfiUnicodeCollation2ProtocolGuid
> >>
> >>
> >> Hmm, this will have the side effect that Ext4Dxe may load before (some of) the architectural protocols, right (modulo implicit dependencies via the UC protocols)? This would need some careful analysis, or we need to add all of the architectural protocols to preserve the old behaviour.
> >>
> >
> > The ext4 driver does nothing except install protocols in its
> > entrypoint, and everything else that happens is under the control of
> > the UEFI driver model, afaict.
>
> I guess. There also is a chance that libraries like DebugLib enable advanced features only as core services become available. But probably not a big deal.
>

We could depex on the UEFI driver prerequisites as well as the
collation protocol ones, arguably retaining the UEFI_DRIVER status.