From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.8750.1676647072217022759 for ; Fri, 17 Feb 2023 07:17:52 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=S9LhH17e; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id AD2C361DD8 for ; Fri, 17 Feb 2023 15:17:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1F344C4339C for ; Fri, 17 Feb 2023 15:17:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1676647071; bh=SoZCAYcllUqMkvJkfJsZzcBhwBxJge8B354bQhXvft0=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=S9LhH17egcXq5uTX1NLNet7uPymXg4zjXzl4Bvp0IIcuyl3ndpByb0LNh0ptkQtxa G2Ghtxct3eYeb4TS5E1ozL8AvkPl3ta6rCvSjlCkwFSZjZu+dAfo3hf3+3PPEDjQph MLKgChN6WH7mXcT7dEU/Om8VmKD8krLlmbMF2EfHjs9/EWQhNkSbPe2LQno/wh33VG wZNzO/4p6KODrVsvxLQMpUPujFi+UsGA4yWrmhN0ivOPSiw18fpxQeP4nRagafFIa4 +KxIk4bU6SftEpyvXnyvWYjAjyEsAq2XftLWEF49S3Wwnj1A3tFBAyxJZk65tIQGBV Xb47v8uj/5LIw== Received: by mail-lj1-f173.google.com with SMTP id r28so1490416ljk.1 for ; Fri, 17 Feb 2023 07:17:50 -0800 (PST) X-Gm-Message-State: AO0yUKVjEfv5n4j4UscLX3mo3U/ZcxMBNEBow5zX/Ht7ywepbQh9jwIG AjB0q20HTHrv6Fpo5nvW4jjJ5Ppek8T5wXaLS0c= X-Google-Smtp-Source: AK7set87uZtBmqJFnmN1/N5NwKtBI/HXDWoV4muIBOYMCr8Kn4nkszAdS/pOsvxwnoZn7bIqJTWWSZUZ7x3atitQ7fo= X-Received: by 2002:a05:651c:205c:b0:28e:d4ae:90ab with SMTP id t28-20020a05651c205c00b0028ed4ae90abmr491555ljo.2.1676647069084; Fri, 17 Feb 2023 07:17:49 -0800 (PST) MIME-Version: 1.0 References: <2DE5E6BC-4AAE-4E31-91E2-B47B2F93DA43@posteo.de> In-Reply-To: <2DE5E6BC-4AAE-4E31-91E2-B47B2F93DA43@posteo.de> From: "Ard Biesheuvel" Date: Fri, 17 Feb 2023 16:17:38 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH edk2-platforms 1/4] Ext4Pkg: Use depex for unicode collation protocols To: =?UTF-8?Q?Marvin_H=C3=A4user?= Cc: devel@edk2.groups.io, Pedro Falcato Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 17 Feb 2023 at 15:55, Marvin H=C3=A4user wrote= : > > > > On 17. Feb 2023, at 15:29, Ard Biesheuvel wrote: > > > > =EF=BB=BFOn Fri, 17 Feb 2023 at 15:05, Marvin H=C3=A4user 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 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, righ= t? I'd prefer a universal and consistent solution to the issue and this loo= ks fine, honestly. > >> > >> [1] > >> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30= de5aa08be/FatPkg/EnhancedFatDxe/Fat.c#L381 > >> https://github.com/tianocore/edk2/blob/02fcfdce1e5ce86f1951191883e7e30= de5aa08be/FatPkg/EnhancedFatDxe/Fat.inf#L19 > >> > > > > I'd prefer using existing features for this, rather than open coding it= . > > It=E2=80=98s not like we replicate a feature, we just move a function cal= l to suit the control flow better. I=E2=80=98d like a consistent solution a= nd 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 =3D UEFI_DRIVER > >> + MODULE_TYPE =3D 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 ab= using the types to utilise DXE concepts. I=E2=80=98m not opposed to such th= ings in general, but here it looks unnecessary. It doesn=E2=80=99t help I= =E2=80=99m 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 alt= ernatives later. In the end, it=E2=80=98s Pedro=E2=80=98s call anyway. > Indeed. > > > > > >> +[Depex] > >> + gEfiUnicodeCollationProtocolGuid OR gEfiUnicodeCollation2ProtocolGu= id > >> > >> > >> Hmm, this will have the side effect that Ext4Dxe may load before (some= of) the architectural protocols, right (modulo implicit dependencies via t= he 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 advan= ced 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.